* [PATCH] configure: define CONFIG_XEN when Xen is enabled
@ 2020-07-28 9:18 Paul Durrant
2020-07-28 9:27 ` Peter Maydell
0 siblings, 1 reply; 7+ messages in thread
From: Paul Durrant @ 2020-07-28 9:18 UTC (permalink / raw)
To: qemu-devel, xen-devel
Cc: Anthony Perard, Paul Durrant, Stefano Stabellini,
Philippe Mathieu-Daudé,
Laurent Vivier
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
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] configure: define CONFIG_XEN when Xen is enabled
2020-07-28 9:18 [PATCH] configure: define CONFIG_XEN when Xen is enabled Paul Durrant
@ 2020-07-28 9:27 ` Peter Maydell
2020-07-28 9:51 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2020-07-28 9:27 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Paul Durrant, QEMU Developers,
Laurent Vivier, Anthony Perard, open list:X86,
Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] configure: define CONFIG_XEN when Xen is enabled
2020-07-28 9:27 ` Peter Maydell
@ 2020-07-28 9:51 ` Philippe Mathieu-Daudé
2020-07-28 9:53 ` Peter Maydell
0 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-28 9:51 UTC (permalink / raw)
To: Peter Maydell, Paul Durrant, Paolo Bonzini
Cc: Stefano Stabellini, Paul Durrant, QEMU Developers,
Laurent Vivier, Anthony Perard, open list:X86
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] configure: define CONFIG_XEN when Xen is enabled
2020-07-28 9:51 ` Philippe Mathieu-Daudé
@ 2020-07-28 9:53 ` Peter Maydell
2020-07-28 9:56 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2020-07-28 9:53 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Stefano Stabellini, Paul Durrant, Paul Durrant, QEMU Developers,
Laurent Vivier, open list:X86, Anthony Perard, Paolo Bonzini
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] configure: define CONFIG_XEN when Xen is enabled
2020-07-28 9:53 ` Peter Maydell
@ 2020-07-28 9:56 ` Philippe Mathieu-Daudé
2020-07-28 10:00 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-28 9:56 UTC (permalink / raw)
To: Peter Maydell
Cc: Stefano Stabellini, Paul Durrant, Paul Durrant, QEMU Developers,
Laurent Vivier, open list:X86, Anthony Perard, Paolo Bonzini
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] configure: define CONFIG_XEN when Xen is enabled
2020-07-28 9:56 ` Philippe Mathieu-Daudé
@ 2020-07-28 10:00 ` Philippe Mathieu-Daudé
2020-07-28 10:13 ` Peter Maydell
0 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-28 10:00 UTC (permalink / raw)
To: Peter Maydell
Cc: Stefano Stabellini, Paul Durrant, Paul Durrant, QEMU Developers,
Laurent Vivier, open list:X86, Anthony Perard, Paolo Bonzini
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.
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] configure: define CONFIG_XEN when Xen is enabled
2020-07-28 10:00 ` Philippe Mathieu-Daudé
@ 2020-07-28 10:13 ` Peter Maydell
0 siblings, 0 replies; 7+ messages in thread
From: Peter Maydell @ 2020-07-28 10:13 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Stefano Stabellini, Paul Durrant, Paul Durrant, QEMU Developers,
Laurent Vivier, open list:X86, Anthony Perard, Paolo Bonzini
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-07-28 10:14 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-28 9:18 [PATCH] configure: define CONFIG_XEN when Xen is enabled Paul Durrant
2020-07-28 9:27 ` Peter Maydell
2020-07-28 9:51 ` Philippe Mathieu-Daudé
2020-07-28 9:53 ` Peter Maydell
2020-07-28 9:56 ` Philippe Mathieu-Daudé
2020-07-28 10:00 ` Philippe Mathieu-Daudé
2020-07-28 10:13 ` Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).