| Message ID | 20201028174406.23424-1-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | meson.build: fix building of Xen support for aarch64 | expand |
On Wed, 28 Oct 2020, Alex Bennée wrote: > Xen is supported on aarch64 although weirdly using the i386-softmmu > model. Checking based on the host CPU meant we never enabled Xen > support. It would be nice to enable CONFIG_XEN for aarch64-softmmu to > make it not seem weird but that will require further build surgery. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Cc: Masami Hiramatsu <masami.hiramatsu@linaro.org> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Anthony Perard <anthony.perard@citrix.com> > Cc: Paul Durrant <paul@xen.org> > Fixes: 8a19980e3f ("configure: move accelerator logic to meson") > --- > meson.build | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/meson.build b/meson.build > index 835424999d..f1fcbfed4c 100644 > --- a/meson.build > +++ b/meson.build > @@ -81,6 +81,8 @@ if cpu in ['x86', 'x86_64'] > 'CONFIG_HVF': ['x86_64-softmmu'], > 'CONFIG_WHPX': ['i386-softmmu', 'x86_64-softmmu'], > } > +elif cpu in [ 'arm', 'aarch64' ] > + accelerator_targets += { 'CONFIG_XEN': ['i386-softmmu'] } > endif This looks very reasonable -- the patch makes sense. However I have two questions, mostly for my own understanding. I tried to repro the aarch64 build problem but it works at my end, even without this patch. I wonder why. I suspect it works thanks to these lines in meson.build: if not get_option('xen').disabled() and 'CONFIG_XEN_BACKEND' in config_host accelerators += 'CONFIG_XEN' have_xen_pci_passthrough = not get_option('xen_pci_passthrough').disabled() and targetos == 'linux' else have_xen_pci_passthrough = false endif But I am not entirely sure who is adding CONFIG_XEN_BACKEND to config_host. The other question is: does it make sense to print the value of CONFIG_XEN as part of the summary? Something like: diff --git a/meson.build b/meson.build index 47e32e1fcb..c6e7832dc9 100644 --- a/meson.build +++ b/meson.build @@ -2070,6 +2070,7 @@ summary_info += {'KVM support': config_all.has_key('CONFIG_KVM')} summary_info += {'HAX support': config_all.has_key('CONFIG_HAX')} summary_info += {'HVF support': config_all.has_key('CONFIG_HVF')} summary_info += {'WHPX support': config_all.has_key('CONFIG_WHPX')} +summary_info += {'XEN support': config_all.has_key('CONFIG_XEN')} summary_info += {'TCG support': config_all.has_key('CONFIG_TCG')} if config_all.has_key('CONFIG_TCG') summary_info += {'TCG debug enabled': config_host.has_key('CONFIG_DEBUG_TCG')} But I realize there is already: summary_info += {'xen support': config_host.has_key('CONFIG_XEN_BACKEND')} so it would be a bit of a duplicate
Hi Alex, 2020年10月29日(木) 2:44 Alex Bennée <alex.bennee@linaro.org>: > > Xen is supported on aarch64 although weirdly using the i386-softmmu > model. Checking based on the host CPU meant we never enabled Xen > support. It would be nice to enable CONFIG_XEN for aarch64-softmmu to > make it not seem weird but that will require further build surgery. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Cc: Masami Hiramatsu <masami.hiramatsu@linaro.org> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Anthony Perard <anthony.perard@citrix.com> > Cc: Paul Durrant <paul@xen.org> > Fixes: 8a19980e3f ("configure: move accelerator logic to meson") Thanks for the fix, I confirmed that the CONFIG_XEN=1 on arm64 build. Reviewed-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> Thank you, > --- > meson.build | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/meson.build b/meson.build > index 835424999d..f1fcbfed4c 100644 > --- a/meson.build > +++ b/meson.build > @@ -81,6 +81,8 @@ if cpu in ['x86', 'x86_64'] > 'CONFIG_HVF': ['x86_64-softmmu'], > 'CONFIG_WHPX': ['i386-softmmu', 'x86_64-softmmu'], > } > +elif cpu in [ 'arm', 'aarch64' ] > + accelerator_targets += { 'CONFIG_XEN': ['i386-softmmu'] } > endif > > ################## > -- > 2.20.1 > -- Masami Hiramatsu
Stefano Stabellini <sstabellini@kernel.org> writes: > On Wed, 28 Oct 2020, Alex Bennée wrote: >> Xen is supported on aarch64 although weirdly using the i386-softmmu >> model. Checking based on the host CPU meant we never enabled Xen >> support. It would be nice to enable CONFIG_XEN for aarch64-softmmu to >> make it not seem weird but that will require further build surgery. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> Cc: Masami Hiramatsu <masami.hiramatsu@linaro.org> >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> Cc: Anthony Perard <anthony.perard@citrix.com> >> Cc: Paul Durrant <paul@xen.org> >> Fixes: 8a19980e3f ("configure: move accelerator logic to meson") >> --- >> meson.build | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/meson.build b/meson.build >> index 835424999d..f1fcbfed4c 100644 >> --- a/meson.build >> +++ b/meson.build >> @@ -81,6 +81,8 @@ if cpu in ['x86', 'x86_64'] >> 'CONFIG_HVF': ['x86_64-softmmu'], >> 'CONFIG_WHPX': ['i386-softmmu', 'x86_64-softmmu'], >> } >> +elif cpu in [ 'arm', 'aarch64' ] >> + accelerator_targets += { 'CONFIG_XEN': ['i386-softmmu'] } >> endif > > This looks very reasonable -- the patch makes sense. > > > However I have two questions, mostly for my own understanding. I tried > to repro the aarch64 build problem but it works at my end, even without > this patch. Building on a x86 host or with cross compiler? > I wonder why. I suspect it works thanks to these lines in > meson.build: > > if not get_option('xen').disabled() and 'CONFIG_XEN_BACKEND' in config_host > accelerators += 'CONFIG_XEN' > have_xen_pci_passthrough = not get_option('xen_pci_passthrough').disabled() and targetos == 'linux' > else > have_xen_pci_passthrough = false > endif > > But I am not entirely sure who is adding CONFIG_XEN_BACKEND to > config_host. The is part of the top level configure check - which basically checks for --enable-xen or autodetects the presence of the userspace libraries. I'm not sure if we have a slight over proliferation of symbols for Xen support (although I'm about to add more). > The other question is: does it make sense to print the value of > CONFIG_XEN as part of the summary? Something like: > > diff --git a/meson.build b/meson.build > index 47e32e1fcb..c6e7832dc9 100644 > --- a/meson.build > +++ b/meson.build > @@ -2070,6 +2070,7 @@ summary_info += {'KVM support': config_all.has_key('CONFIG_KVM')} > summary_info += {'HAX support': config_all.has_key('CONFIG_HAX')} > summary_info += {'HVF support': config_all.has_key('CONFIG_HVF')} > summary_info += {'WHPX support': config_all.has_key('CONFIG_WHPX')} > +summary_info += {'XEN support': config_all.has_key('CONFIG_XEN')} > summary_info += {'TCG support': config_all.has_key('CONFIG_TCG')} > if config_all.has_key('CONFIG_TCG') > summary_info += {'TCG debug enabled': config_host.has_key('CONFIG_DEBUG_TCG')} > > > But I realize there is already: > > summary_info += {'xen support': config_host.has_key('CONFIG_XEN_BACKEND')} > > so it would be a bit of a duplicate Hmm so what we have is: CONFIG_XEN_BACKEND - ensures that appropriate compiler flags are added - pegs RAM_ADDR_MAX at UINT64_MAX (instead of UINTPTR_MAX) CONFIG_XEN - which controls a bunch of build objects, some of which may be i386 only? ./accel/meson.build:15:specific_ss.add_all(when: ['CONFIG_XEN'], if_true: dummy_ss) ./accel/stubs/meson.build:2:specific_ss.add(when: 'CONFIG_XEN', if_false: files('xen-stub.c')) ./accel/xen/meson.build:1:specific_ss.add(when: 'CONFIG_XEN', if_true: files('xen-all.c')) ./hw/9pfs/meson.build:17:fs_ss.add(when: 'CONFIG_XEN', if_true: files('xen-9p-backend.c')) ./hw/block/dataplane/meson.build:2:specific_ss.add(when: 'CONFIG_XEN', if_true: files('xen-block.c')) ./hw/block/meson.build:14:softmmu_ss.add(when: 'CONFIG_XEN', if_true: files('xen-block.c')) ./hw/char/meson.build:23:softmmu_ss.add(when: 'CONFIG_XEN', if_true: files('xen_console.c')) ./hw/display/meson.build:18:softmmu_ss.add(when: 'CONFIG_XEN', if_true: files('xenfb.c')) ./hw/i386/xen/meson.build:1:i386_ss.add(when: 'CONFIG_XEN', if_true: files('xen-hvm.c', 'xen-mapcache.c', 'xen_apic.c', 'xen_platform.c', 'xen_pvdevice.c') ./hw/net/meson.build:2:softmmu_ss.add(when: 'CONFIG_XEN', if_true: files('xen_nic.c')) ./hw/usb/meson.build:76:softmmu_ss.add(when: ['CONFIG_USB', 'CONFIG_XEN', libusb], if_true: files('xen-usb.c')) ./hw/xen/meson.build:1:softmmu_ss.add(when: ['CONFIG_XEN', xen], if_true: files( ./hw/xen/meson.build:20:specific_ss.add_all(when: ['CONFIG_XEN', xen], if_true: xen_specific_ss) ./hw/xenpv/meson.build:3:xenpv_ss.add(when: 'CONFIG_XEN', if_true: files('xen_machine_pv.c')) - there are also some stubbed inline functions controlled by it CONFIG_XEN_IGD_PASSTHROUGH - specific x86 PC only feature via Kconfig rule CONFIG_XEN_PCI_PASSTHROUGH - control Linux specific specific feature (via meson rule) First obvious question - is everything in hw/i386/xen actually i386 only? APIC seems pretty PC orientated but what about xen_platform and pvdevice? There seem to be some dependancies on xen-mapcache across the code. -- Alex Bennée
On Thu, Oct 29, 2020 at 6:01 AM Alex Bennée <alex.bennee@linaro.org> wrote: > > > Stefano Stabellini <sstabellini@kernel.org> writes: > > > On Wed, 28 Oct 2020, Alex Bennée wrote: > >> Xen is supported on aarch64 although weirdly using the i386-softmmu > >> model. Checking based on the host CPU meant we never enabled Xen > >> support. It would be nice to enable CONFIG_XEN for aarch64-softmmu to > >> make it not seem weird but that will require further build surgery. > >> > >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > >> Cc: Masami Hiramatsu <masami.hiramatsu@linaro.org> > >> Cc: Stefano Stabellini <sstabellini@kernel.org> > >> Cc: Anthony Perard <anthony.perard@citrix.com> > >> Cc: Paul Durrant <paul@xen.org> > >> Fixes: 8a19980e3f ("configure: move accelerator logic to meson") > >> --- > >> meson.build | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/meson.build b/meson.build > >> index 835424999d..f1fcbfed4c 100644 > >> --- a/meson.build > >> +++ b/meson.build > >> @@ -81,6 +81,8 @@ if cpu in ['x86', 'x86_64'] > >> 'CONFIG_HVF': ['x86_64-softmmu'], > >> 'CONFIG_WHPX': ['i386-softmmu', 'x86_64-softmmu'], > >> } > >> +elif cpu in [ 'arm', 'aarch64' ] > >> + accelerator_targets += { 'CONFIG_XEN': ['i386-softmmu'] } > >> endif > > > > This looks very reasonable -- the patch makes sense. A comment would be useful to explain that Arm needs i386-softmmu for the xenpv machine. > > > > However I have two questions, mostly for my own understanding. I tried > > to repro the aarch64 build problem but it works at my end, even without > > this patch. > > Building on a x86 host or with cross compiler? > > > I wonder why. I suspect it works thanks to these lines in > > meson.build: I think it's a runtime and not a build problem. In osstest, Xen support was detected and configured, but CONFIG_XEN wasn't set for Arm. So at runtime xen_available() returns 0, and QEMU doesn't start with "qemu-system-i386: -xen-domid 1: Option not supported for this target" I posted my investigation here: https://lore.kernel.org/xen-devel/CAKf6xpss8KpGOvZrKiTPz63bhBVbjxRTYWdHEkzUo2q1KEMjhg@mail.gmail.com/ Regards, Jason
On Thu, 29 Oct 2020, Jason Andryuk wrote: > On Thu, Oct 29, 2020 at 6:01 AM Alex Bennée <alex.bennee@linaro.org> wrote: > > > > > > Stefano Stabellini <sstabellini@kernel.org> writes: > > > > > On Wed, 28 Oct 2020, Alex Bennée wrote: > > >> Xen is supported on aarch64 although weirdly using the i386-softmmu > > >> model. Checking based on the host CPU meant we never enabled Xen > > >> support. It would be nice to enable CONFIG_XEN for aarch64-softmmu to > > >> make it not seem weird but that will require further build surgery. > > >> > > >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > >> Cc: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > >> Cc: Stefano Stabellini <sstabellini@kernel.org> > > >> Cc: Anthony Perard <anthony.perard@citrix.com> > > >> Cc: Paul Durrant <paul@xen.org> > > >> Fixes: 8a19980e3f ("configure: move accelerator logic to meson") > > >> --- > > >> meson.build | 2 ++ > > >> 1 file changed, 2 insertions(+) > > >> > > >> diff --git a/meson.build b/meson.build > > >> index 835424999d..f1fcbfed4c 100644 > > >> --- a/meson.build > > >> +++ b/meson.build > > >> @@ -81,6 +81,8 @@ if cpu in ['x86', 'x86_64'] > > >> 'CONFIG_HVF': ['x86_64-softmmu'], > > >> 'CONFIG_WHPX': ['i386-softmmu', 'x86_64-softmmu'], > > >> } > > >> +elif cpu in [ 'arm', 'aarch64' ] > > >> + accelerator_targets += { 'CONFIG_XEN': ['i386-softmmu'] } > > >> endif > > > > > > This looks very reasonable -- the patch makes sense. > > A comment would be useful to explain that Arm needs i386-softmmu for > the xenpv machine. > > > > > > > However I have two questions, mostly for my own understanding. I tried > > > to repro the aarch64 build problem but it works at my end, even without > > > this patch. > > > > Building on a x86 host or with cross compiler? > > > > > I wonder why. I suspect it works thanks to these lines in > > > meson.build: > > I think it's a runtime and not a build problem. In osstest, Xen > support was detected and configured, but CONFIG_XEN wasn't set for > Arm. So at runtime xen_available() returns 0, and QEMU doesn't start > with "qemu-system-i386: -xen-domid 1: Option not supported for this > target" > > I posted my investigation here: > https://lore.kernel.org/xen-devel/CAKf6xpss8KpGOvZrKiTPz63bhBVbjxRTYWdHEkzUo2q1KEMjhg@mail.gmail.com/ Right, but strangely I cannot reproduce it here. I am natively building (qemu-user) on aarch64 and for me CONFIG_XEN gets set.
On Thu, 29 Oct 2020, Alex Bennée wrote: > > On Wed, 28 Oct 2020, Alex Bennée wrote: > >> Xen is supported on aarch64 although weirdly using the i386-softmmu > >> model. Checking based on the host CPU meant we never enabled Xen > >> support. It would be nice to enable CONFIG_XEN for aarch64-softmmu to > >> make it not seem weird but that will require further build surgery. > >> > >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > >> Cc: Masami Hiramatsu <masami.hiramatsu@linaro.org> > >> Cc: Stefano Stabellini <sstabellini@kernel.org> > >> Cc: Anthony Perard <anthony.perard@citrix.com> > >> Cc: Paul Durrant <paul@xen.org> > >> Fixes: 8a19980e3f ("configure: move accelerator logic to meson") > >> --- > >> meson.build | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/meson.build b/meson.build > >> index 835424999d..f1fcbfed4c 100644 > >> --- a/meson.build > >> +++ b/meson.build > >> @@ -81,6 +81,8 @@ if cpu in ['x86', 'x86_64'] > >> 'CONFIG_HVF': ['x86_64-softmmu'], > >> 'CONFIG_WHPX': ['i386-softmmu', 'x86_64-softmmu'], > >> } > >> +elif cpu in [ 'arm', 'aarch64' ] > >> + accelerator_targets += { 'CONFIG_XEN': ['i386-softmmu'] } > >> endif > > > > This looks very reasonable -- the patch makes sense. > > > > > > However I have two questions, mostly for my own understanding. I tried > > to repro the aarch64 build problem but it works at my end, even without > > this patch. > > Building on a x86 host or with cross compiler? native build (qemu-user) > > I wonder why. I suspect it works thanks to these lines in > > meson.build: > > > > if not get_option('xen').disabled() and 'CONFIG_XEN_BACKEND' in config_host > > accelerators += 'CONFIG_XEN' > > have_xen_pci_passthrough = not get_option('xen_pci_passthrough').disabled() and targetos == 'linux' > > else > > have_xen_pci_passthrough = false > > endif > > > > But I am not entirely sure who is adding CONFIG_XEN_BACKEND to > > config_host. > > The is part of the top level configure check - which basically checks > for --enable-xen or autodetects the presence of the userspace libraries. > I'm not sure if we have a slight over proliferation of symbols for Xen > support (although I'm about to add more). > > > The other question is: does it make sense to print the value of > > CONFIG_XEN as part of the summary? Something like: > > > > diff --git a/meson.build b/meson.build > > index 47e32e1fcb..c6e7832dc9 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -2070,6 +2070,7 @@ summary_info += {'KVM support': config_all.has_key('CONFIG_KVM')} > > summary_info += {'HAX support': config_all.has_key('CONFIG_HAX')} > > summary_info += {'HVF support': config_all.has_key('CONFIG_HVF')} > > summary_info += {'WHPX support': config_all.has_key('CONFIG_WHPX')} > > +summary_info += {'XEN support': config_all.has_key('CONFIG_XEN')} > > summary_info += {'TCG support': config_all.has_key('CONFIG_TCG')} > > if config_all.has_key('CONFIG_TCG') > > summary_info += {'TCG debug enabled': config_host.has_key('CONFIG_DEBUG_TCG')} > > > > > > But I realize there is already: > > > > summary_info += {'xen support': config_host.has_key('CONFIG_XEN_BACKEND')} > > > > so it would be a bit of a duplicate > > Hmm so what we have is: > > CONFIG_XEN_BACKEND > - ensures that appropriate compiler flags are added > - pegs RAM_ADDR_MAX at UINT64_MAX (instead of UINTPTR_MAX) > CONFIG_XEN > - which controls a bunch of build objects, some of which may be i386 only? > ./accel/meson.build:15:specific_ss.add_all(when: ['CONFIG_XEN'], if_true: dummy_ss) > ./accel/stubs/meson.build:2:specific_ss.add(when: 'CONFIG_XEN', if_false: files('xen-stub.c')) > ./accel/xen/meson.build:1:specific_ss.add(when: 'CONFIG_XEN', if_true: files('xen-all.c')) > ./hw/9pfs/meson.build:17:fs_ss.add(when: 'CONFIG_XEN', if_true: files('xen-9p-backend.c')) > ./hw/block/dataplane/meson.build:2:specific_ss.add(when: 'CONFIG_XEN', if_true: files('xen-block.c')) > ./hw/block/meson.build:14:softmmu_ss.add(when: 'CONFIG_XEN', if_true: files('xen-block.c')) > ./hw/char/meson.build:23:softmmu_ss.add(when: 'CONFIG_XEN', if_true: files('xen_console.c')) > ./hw/display/meson.build:18:softmmu_ss.add(when: 'CONFIG_XEN', if_true: files('xenfb.c')) > ./hw/i386/xen/meson.build:1:i386_ss.add(when: 'CONFIG_XEN', if_true: files('xen-hvm.c', > 'xen-mapcache.c', > 'xen_apic.c', > 'xen_platform.c', > 'xen_pvdevice.c') > ./hw/net/meson.build:2:softmmu_ss.add(when: 'CONFIG_XEN', if_true: files('xen_nic.c')) > ./hw/usb/meson.build:76:softmmu_ss.add(when: ['CONFIG_USB', 'CONFIG_XEN', libusb], if_true: files('xen-usb.c')) > ./hw/xen/meson.build:1:softmmu_ss.add(when: ['CONFIG_XEN', xen], if_true: files( > ./hw/xen/meson.build:20:specific_ss.add_all(when: ['CONFIG_XEN', xen], if_true: xen_specific_ss) > ./hw/xenpv/meson.build:3:xenpv_ss.add(when: 'CONFIG_XEN', if_true: files('xen_machine_pv.c')) > - there are also some stubbed inline functions controlled by it > CONFIG_XEN_IGD_PASSTHROUGH > - specific x86 PC only feature via Kconfig rule > CONFIG_XEN_PCI_PASSTHROUGH > - control Linux specific specific feature (via meson rule) > > > First obvious question - is everything in hw/i386/xen actually i386 > only? APIC seems pretty PC orientated but what about xen_platform and > pvdevice? There seem to be some dependancies on xen-mapcache across the > code. All files under hw/i386/xen are only used on x86 today, because they are either x86 specific, or ioreq specific (required to handle ioreqs.) Given that today ioreqs are not used on ARM, they are not used. In the near future when we are going to handle ioreqs on ARM, they'll become useful. xen_apic.c: x86 specific xen-hvm.c: There are some x86 things there, but mostly it is about handling ioreqs which is going to be used on ARM soon as part of the virtio enablement xen-mapcache.c: The mapcache is used when ioreqs are used, it will get useful xen_platform.c: x86 specific xen_pvdevice.c: x86 specific
diff --git a/meson.build b/meson.build index 835424999d..f1fcbfed4c 100644 --- a/meson.build +++ b/meson.build @@ -81,6 +81,8 @@ if cpu in ['x86', 'x86_64'] 'CONFIG_HVF': ['x86_64-softmmu'], 'CONFIG_WHPX': ['i386-softmmu', 'x86_64-softmmu'], } +elif cpu in [ 'arm', 'aarch64' ] + accelerator_targets += { 'CONFIG_XEN': ['i386-softmmu'] } endif ##################
Xen is supported on aarch64 although weirdly using the i386-softmmu model. Checking based on the host CPU meant we never enabled Xen support. It would be nice to enable CONFIG_XEN for aarch64-softmmu to make it not seem weird but that will require further build surgery. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Cc: Masami Hiramatsu <masami.hiramatsu@linaro.org> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Anthony Perard <anthony.perard@citrix.com> Cc: Paul Durrant <paul@xen.org> Fixes: 8a19980e3f ("configure: move accelerator logic to meson") --- meson.build | 2 ++ 1 file changed, 2 insertions(+)