Re: [PATCH] fix qemu build with xen-4.18.0

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Stefan,

Stefan Hajnoczi <stefanha@xxxxxxxxx> writes:

> On Tue, 12 Dec 2023 at 10:36, Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx> wrote:
>>
>> Hi Anthony
>>
>> Anthony PERARD <anthony.perard@xxxxxxxxxx> writes:
>>
>> > On Fri, Dec 08, 2023 at 02:49:27PM -0800, Stefano Stabellini wrote:
>> >> On Fri, 8 Dec 2023, Daniel P. Berrangé wrote:
>> >> > On Thu, Dec 07, 2023 at 11:12:48PM +0000, Michael Young wrote:
>> >> > > Builds of qemu-8.2.0rc2 with xen-4.18.0 are currently failing
>> >> > > with errors like
>> >> > > ../hw/arm/xen_arm.c:74:5: error: ‘GUEST_VIRTIO_MMIO_SPI_LAST’ 
>> >> > > undeclared (first use in this function)
>> >> > >    74 |    (GUEST_VIRTIO_MMIO_SPI_LAST - GUEST_VIRTIO_MMIO_SPI_FIRST)
>> >> > >       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~
>> >> > >
>> >> > > as there is an incorrect comparision in include/hw/xen/xen_native.h
>> >> > > which means that settings like GUEST_VIRTIO_MMIO_SPI_LAST
>> >> > > aren't being defined for xen-4.18.0
>> >> >
>> >> > The conditions in arch-arm.h for xen 4.18 show:
>> >> >
>> >> > $ cppi arch-arm.h | grep -E '(#.*if)|MMIO'
>> >> > #ifndef __XEN_PUBLIC_ARCH_ARM_H__
>> >> > # if defined(__XEN__) || defined(__XEN_TOOLS__) || defined(__GNUC__)
>> >> > # endif
>> >> > # ifndef __ASSEMBLY__
>> >> > #  if defined(__XEN__) || defined(__XEN_TOOLS__)
>> >> > #   if defined(__GNUC__) && !defined(__STRICT_ANSI__)
>> >> > #   endif
>> >> > #  endif /* __XEN__ || __XEN_TOOLS__ */
>> >> > # endif
>> >> > # if defined(__XEN__) || defined(__XEN_TOOLS__)
>> >> > #  define PSR_MODE_BIT  0x10U /* Set iff AArch32 */
>> >> > /* Virtio MMIO mappings */
>> >> > #  define GUEST_VIRTIO_MMIO_BASE   xen_mk_ullong(0x02000000)
>> >> > #  define GUEST_VIRTIO_MMIO_SIZE   xen_mk_ullong(0x00100000)
>> >> > #  define GUEST_VIRTIO_MMIO_SPI_FIRST   33
>> >> > #  define GUEST_VIRTIO_MMIO_SPI_LAST    43
>> >> > # endif
>> >> > # ifndef __ASSEMBLY__
>> >> > # endif
>> >> > #endif /*  __XEN_PUBLIC_ARCH_ARM_H__ */
>> >> >
>> >> > So the MMIO constants are available if __XEN__ or __XEN_TOOLS__
>> >> > are defined. This is no different to the condition that was
>> >> > present in Xen 4.17.
>> >> >
>> >> > What you didn't mention was that the Fedora build failure is
>> >> > seen on an x86_64 host, when building the aarch64 target QEMU,
>> >> > and I think this is the key issue.
>> >>
>> >> Hi Daniel, thanks for looking into it.
>> >>
>> >> - you are building on a x86_64 host
>> >> - the target is aarch64
>> >> - the target is the aarch64 Xen PVH machine (xen_arm.c)
>> >>
>> >> But is the resulting QEMU binary expected to be an x86 binary? Or are
>> >> you cross compiling ARM binaries on a x86 host?
>> >>
>> >> In other word, is the resulting QEMU binary expected to run on ARM or
>> >> x86?
>> >>
>> >>
>> >> > Are we expecting to build Xen support for non-arch native QEMU
>> >> > system binaries or not ?
>> >>
>> >> The ARM xenpvh machine (xen_arm.c) is meant to work with Xen on ARM, not
>> >> Xen on x86.  So this is only expected to work if you are
>> >> cross-compiling. But you can cross-compile both Xen and QEMU, and I am
>> >> pretty sure that Yocto is able to build Xen, Xen userspace tools, and
>> >> QEMU for Xen/ARM on an x86 host today.
>> >>
>> >>
>> >> > The constants are defined in arch-arm.h, which is only included
>> >> > under:
>> >> >
>> >> >   #if defined(__i386__) || defined(__x86_64__)
>> >> >   #include "arch-x86/xen.h"
>> >> >   #elif defined(__arm__) || defined (__aarch64__)
>> >> >   #include "arch-arm.h"
>> >> >   #else
>> >> >   #error "Unsupported architecture"
>> >> >   #endif
>> >> >
>> >> >
>> >> > When we are building on an x86_64 host, we not going to get
>> >> > arch-arm.h included, even if we're trying to build the aarch64
>> >> > system emulator.
>> >> >
>> >> > I don't know how this is supposed to work ?
>> >>
>> >> It looks like a host vs. target architecture mismatch: the #if defined
>> >> (__aarch64__) check should pass I think.
>> >
>> >
>> > Building qemu with something like:
>> >     ./configure --enable-xen --cpu=x86_64
>> > used to work. Can we fix that? It still works with v8.1.0.
>> > At least, it works on x86, I never really try to build qemu for arm.
>> > Notice that there's no "--target-list" on the configure command line.
>> > I don't know if --cpu is useful here.
>> >
>> > Looks like the first commit where the build doesn't work is
>> > 7899f6589b78 ("xen_arm: Add virtual PCIe host bridge support").
>>
>> I am currently trying to upstream this patch. It is in the QEMU mailing
>> list but it was never accepted. It is not reviewed in fact. I'll take a
>> look at it, but I don't understand how did you get in the first place.
>
> Hi Volodymyr,
> Paolo Bonzini sent a pull request with similar code changes this
> morning and I have merged it into the qemu.git/staging branch:
> https://urldefense.com/v3/__https://gitlab.com/qemu-project/qemu/-/commit/eaae59af4035770975b0ce9364b587223a909501__;!!GF_29dbcQIUBPA!yFgSxAEgXPjckF8piSt0T77bbeggSgwC-6-xDuZmzq4a8U7HEP8XxGnxwIhgA9iyFVie-fdVgAVA5wVipnewbLNp$
> [gitlab[.]com]
>
> If you spot something that is not correct, please reply here.
>

No, it is all fine in that pull request. I was talking about patch
"xen_arm: Add virtual PCIe host bridge support" which is still on
review:
https://patchwork.kernel.org/project/qemu-devel/patch/20231202014108.2017803-7-volodymyr_babchuk@xxxxxxxx/

I was surprised when Anthony mentioned that this patch breaks the
build, because the patch is not included in QEMU tree.

-- 
WBR, Volodymyr