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

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