| Message ID | 20200728091828.21702-1-paul@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | configure: define CONFIG_XEN when Xen is enabled | expand |
On Tue, 28 Jul 2020 at 10:19, Paul Durrant <paul@xen.org> wrote: > > From: Paul Durrant <pdurrant@amazon.com> > > The recent commit da278d58a092 "accel: Move Xen accelerator code under > accel/xen/" introduced a subtle semantic change, making xen_enabled() always > return false unless CONFIG_XEN is defined prior to inclusion of sysemu/xen.h, > which appears to be the normal case. This causes various use-cases of QEMU > with Xen to break. > > This patch makes sure that CONFIG_XEN is defined if --enable-xen is passed > to configure. > > Fixes: da278d58a092 ("accel: Move Xen accelerator code under accel/xen/") > Signed-off-by: Paul Durrant <pdurrant@amazon.com> > --- > Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com> > Cc: Laurent Vivier <laurent@vivier.eu> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Anthony Perard <anthony.perard@citrix.com> > --- > configure | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/configure b/configure > index 2acc4d1465..f1b9d129fd 100755 > --- a/configure > +++ b/configure > @@ -7434,6 +7434,7 @@ if test "$virglrenderer" = "yes" ; then > echo "VIRGL_LIBS=$virgl_libs" >> $config_host_mak > fi > if test "$xen" = "yes" ; then > + echo "CONFIG_XEN=y" >> $config_host_mak > echo "CONFIG_XEN_BACKEND=y" >> $config_host_mak > echo "CONFIG_XEN_CTRL_INTERFACE_VERSION=$xen_ctrl_version" >> $config_host_mak > fi Configure already defines CONFIG_XEN as a target-specific config define in config-target.mak for the specific targets that Xen will work for (ie if you build --enable-xen for x86_64-softmmu and ppc64-softmmu then CONFIG_XEN is set for the former and not the latter). This patch makes it a build-wide config setting by putting it in config-host.mak. We should figure out which of those two is correct and do just one of them, not do both at the same time. Since CONFIG_HAX, CONFIG_KVM and other accelerator-type config defines are also per-target, I suspect that the correct fix for this bug is not in configure but elsewhere. thanks -- PMM
On 7/28/20 11:27 AM, Peter Maydell wrote: > On Tue, 28 Jul 2020 at 10:19, Paul Durrant <paul@xen.org> wrote: >> >> From: Paul Durrant <pdurrant@amazon.com> >> >> The recent commit da278d58a092 "accel: Move Xen accelerator code under >> accel/xen/" introduced a subtle semantic change, making xen_enabled() always >> return false unless CONFIG_XEN is defined prior to inclusion of sysemu/xen.h, >> which appears to be the normal case. This causes various use-cases of QEMU >> with Xen to break. >> >> This patch makes sure that CONFIG_XEN is defined if --enable-xen is passed >> to configure. >> >> Fixes: da278d58a092 ("accel: Move Xen accelerator code under accel/xen/") >> Signed-off-by: Paul Durrant <pdurrant@amazon.com> >> --- >> Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com> >> Cc: Laurent Vivier <laurent@vivier.eu> >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> Cc: Anthony Perard <anthony.perard@citrix.com> >> --- >> configure | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/configure b/configure >> index 2acc4d1465..f1b9d129fd 100755 >> --- a/configure >> +++ b/configure >> @@ -7434,6 +7434,7 @@ if test "$virglrenderer" = "yes" ; then >> echo "VIRGL_LIBS=$virgl_libs" >> $config_host_mak >> fi >> if test "$xen" = "yes" ; then >> + echo "CONFIG_XEN=y" >> $config_host_mak >> echo "CONFIG_XEN_BACKEND=y" >> $config_host_mak >> echo "CONFIG_XEN_CTRL_INTERFACE_VERSION=$xen_ctrl_version" >> $config_host_mak >> fi > > Configure already defines CONFIG_XEN as a target-specific > config define in config-target.mak for the specific targets > that Xen will work for (ie if you build --enable-xen for > x86_64-softmmu and ppc64-softmmu then CONFIG_XEN is set for > the former and not the latter). This patch makes it a > build-wide config setting by putting it in config-host.mak. > > We should figure out which of those two is correct and do > just one of them, not do both at the same time. > > Since CONFIG_HAX, CONFIG_KVM and other accelerator-type > config defines are also per-target, I suspect that the > correct fix for this bug is not in configure but elsewhere. This might come from this change: -#include "hw/xen/xen.h" +#include "sysemu/xen.h" Before Xen was target-specific, now it is a target-agnostic accelerator. However in include/qemu/osdep.h we have: 30 #include "config-host.h" 31 #ifdef NEED_CPU_H 32 #include "config-target.h" 33 #else 34 #include "exec/poison.h" 35 #endif CONFIG_XEN is generated in "config-target.h" (target-specific), so target-agnostic code always has it undefined. I'd rather uninline xen_enabled() but I'm not sure this has perf penalties. Paolo is that OK to uninline it? Phil.
On Tue, 28 Jul 2020 at 10:51, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > I'd rather uninline xen_enabled() but I'm not sure this has perf > penalties. Paolo is that OK to uninline it? Can we just follow the same working pattern we already have for kvm_enabled() etc ? thanks -- PMM
On 7/28/20 11:53 AM, Peter Maydell wrote: > On Tue, 28 Jul 2020 at 10:51, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> I'd rather uninline xen_enabled() but I'm not sure this has perf >> penalties. Paolo is that OK to uninline it? I suppose no because it is in various hot paths: exec.c:588: if (xen_enabled() && memory_access_is_direct(mr, is_write)) { exec.c:2243: if (xen_enabled()) { exec.c:2326: if (xen_enabled()) { exec.c:2478: } else if (xen_enabled()) { exec.c:2525: } else if (xen_enabled()) { exec.c:2576: if (xen_enabled() && block->host == NULL) { exec.c:2609: if (xen_enabled() && block->host == NULL) { exec.c:2657: if (xen_enabled()) { exec.c:3625: if (xen_enabled()) { exec.c:3717: if (xen_enabled()) { include/exec/ram_addr.h:295: if (!mask && !xen_enabled()) { > > Can we just follow the same working pattern we already have > for kvm_enabled() etc ? This was the idea... I'll look at what I missed. Phil.
On 7/28/20 11:56 AM, Philippe Mathieu-Daudé wrote: > On 7/28/20 11:53 AM, Peter Maydell wrote: >> On Tue, 28 Jul 2020 at 10:51, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >>> I'd rather uninline xen_enabled() but I'm not sure this has perf >>> penalties. Paolo is that OK to uninline it? > > I suppose no because it is in various hot paths: > > exec.c:588: if (xen_enabled() && memory_access_is_direct(mr, is_write)) { > exec.c:2243: if (xen_enabled()) { > exec.c:2326: if (xen_enabled()) { > exec.c:2478: } else if (xen_enabled()) { > exec.c:2525: } else if (xen_enabled()) { > exec.c:2576: if (xen_enabled() && block->host == NULL) { > exec.c:2609: if (xen_enabled() && block->host == NULL) { > exec.c:2657: if (xen_enabled()) { > exec.c:3625: if (xen_enabled()) { > exec.c:3717: if (xen_enabled()) { > include/exec/ram_addr.h:295: if (!mask && !xen_enabled()) { > >> >> Can we just follow the same working pattern we already have >> for kvm_enabled() etc ? > > This was the idea... I'll look at what I missed. Apparently kvm_enabled() checks CONFIG_KVM_IS_POSSIBLE instead of CONFIG_KVM, I suppose to bypass this limitation (from osdep.h): 21 #ifdef NEED_CPU_H 22 # ifdef CONFIG_KVM 24 # define CONFIG_KVM_IS_POSSIBLE 25 # endif 26 #else 27 # define CONFIG_KVM_IS_POSSIBLE 28 #endif 29 30 #ifdef CONFIG_KVM_IS_POSSIBLE ... Paolo do you confirm this is the reason? I'll prepare a similar patch. > > Phil. >
On Tue, 28 Jul 2020 at 11:00, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > Apparently kvm_enabled() checks CONFIG_KVM_IS_POSSIBLE instead > of CONFIG_KVM, I suppose to bypass this limitation (from osdep.h): > > 21 #ifdef NEED_CPU_H > 22 # ifdef CONFIG_KVM > 24 # define CONFIG_KVM_IS_POSSIBLE > 25 # endif > 26 #else > 27 # define CONFIG_KVM_IS_POSSIBLE > 28 #endif > 29 > 30 #ifdef CONFIG_KVM_IS_POSSIBLE > ... Interesting. We don't have CONFIG_WHPX_IS_POSSIBLE, CONFIG_HVF_IS_POSSIBLE, etc -- also bugs, or do we avoid them by happening not to check whpx_enabled(), hvf_enabled(), etc in obj-common-compiled source files? thanks -- PMM
diff --git a/configure b/configure index 2acc4d1465..f1b9d129fd 100755 --- a/configure +++ b/configure @@ -7434,6 +7434,7 @@ if test "$virglrenderer" = "yes" ; then echo "VIRGL_LIBS=$virgl_libs" >> $config_host_mak fi if test "$xen" = "yes" ; then + echo "CONFIG_XEN=y" >> $config_host_mak echo "CONFIG_XEN_BACKEND=y" >> $config_host_mak echo "CONFIG_XEN_CTRL_INTERFACE_VERSION=$xen_ctrl_version" >> $config_host_mak fi