[PATCH] xen: fix build without pci passthrough

[PATCH] xen: fix build without pci passthrough

[Roger Pau Monne]

has_igd_gfx_passthru is only available when QEMU is built with
CONFIG_XEN_PCI_PASSTHROUGH, and hence shouldn't be used in common
code without checking if it's available.

Fixes: 46472d82322d0 ('xen: convert "-machine igd-passthru" to an accelerator property')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Paul Durrant <paul@xen.org>
Cc: xen-devel@lists.xenproject.org
---
 hw/xen/xen-common.c | 4 ++++
 hw/xen/xen_pt.h     | 7 +++++++
 2 files changed, 11 insertions(+)

diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index a15070f7f6..c800862419 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -127,6 +127,7 @@ static void xen_change_state_handler(void *opaque, int running,
     }
 }
 
+#ifdef CONFIG_XEN_PCI_PASSTHROUGH
 static bool xen_get_igd_gfx_passthru(Object *obj, Error **errp)
 {
     return has_igd_gfx_passthru;
@@ -136,6 +137,7 @@ static void xen_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
 {
     has_igd_gfx_passthru = value;
 }
+#endif
 
 static void xen_setup_post(MachineState *ms, AccelState *accel)
 {
@@ -197,11 +199,13 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
 
     compat_props_add(ac->compat_props, compat, G_N_ELEMENTS(compat));
 
+#ifdef CONFIG_XEN_PCI_PASSTHROUGH
     object_class_property_add_bool(oc, "igd-passthru",
         xen_get_igd_gfx_passthru, xen_set_igd_gfx_passthru,
         &error_abort);
     object_class_property_set_description(oc, "igd-passthru",
         "Set on/off to enable/disable igd passthrou", &error_abort);
+#endif
 }
 
 #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 179775db7b..660dd8a008 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -1,6 +1,7 @@
 #ifndef XEN_PT_H
 #define XEN_PT_H
 
+#include "qemu/osdep.h"
 #include "hw/xen/xen_common.h"
 #include "hw/pci/pci.h"
 #include "xen-host-pci-device.h"
@@ -322,7 +323,13 @@ extern void *pci_assign_dev_load_option_rom(PCIDevice *dev,
                                             unsigned int domain,
                                             unsigned int bus, unsigned int slot,
                                             unsigned int function);
+
+#ifdef CONFIG_XEN_PCI_PASSTHROUGH
 extern bool has_igd_gfx_passthru;
+#else
+# define has_igd_gfx_passthru false
+#endif
+
 static inline bool is_igd_vga_passthrough(XenHostPCIDevice *dev)
 {
     return (has_igd_gfx_passthru
-- 
2.26.2

Re: [PATCH] xen: fix build without pci passthrough

[Philippe Mathieu-Daudé]

Hi Roger,

On 5/4/20 12:14 PM, Roger Pau Monne wrote:
> has_igd_gfx_passthru is only available when QEMU is built with
> CONFIG_XEN_PCI_PASSTHROUGH, and hence shouldn't be used in common
> code without checking if it's available.
> 
> Fixes: 46472d82322d0 ('xen: convert "-machine igd-passthru" to an accelerator property')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

See Kconfig fix suggested here:
https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg61844.html

> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org
> ---
>   hw/xen/xen-common.c | 4 ++++
>   hw/xen/xen_pt.h     | 7 +++++++
>   2 files changed, 11 insertions(+)
> 
> diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> index a15070f7f6..c800862419 100644
> --- a/hw/xen/xen-common.c
> +++ b/hw/xen/xen-common.c
> @@ -127,6 +127,7 @@ static void xen_change_state_handler(void *opaque, int running,
>       }
>   }
>   
> +#ifdef CONFIG_XEN_PCI_PASSTHROUGH
>   static bool xen_get_igd_gfx_passthru(Object *obj, Error **errp)
>   {
>       return has_igd_gfx_passthru;
> @@ -136,6 +137,7 @@ static void xen_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
>   {
>       has_igd_gfx_passthru = value;
>   }
> +#endif
>   
>   static void xen_setup_post(MachineState *ms, AccelState *accel)
>   {
> @@ -197,11 +199,13 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
>   
>       compat_props_add(ac->compat_props, compat, G_N_ELEMENTS(compat));
>   
> +#ifdef CONFIG_XEN_PCI_PASSTHROUGH
>       object_class_property_add_bool(oc, "igd-passthru",
>           xen_get_igd_gfx_passthru, xen_set_igd_gfx_passthru,
>           &error_abort);
>       object_class_property_set_description(oc, "igd-passthru",
>           "Set on/off to enable/disable igd passthrou", &error_abort);
> +#endif
>   }
>   
>   #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> index 179775db7b..660dd8a008 100644
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -1,6 +1,7 @@
>   #ifndef XEN_PT_H
>   #define XEN_PT_H
>   
> +#include "qemu/osdep.h"
>   #include "hw/xen/xen_common.h"
>   #include "hw/pci/pci.h"
>   #include "xen-host-pci-device.h"
> @@ -322,7 +323,13 @@ extern void *pci_assign_dev_load_option_rom(PCIDevice *dev,
>                                               unsigned int domain,
>                                               unsigned int bus, unsigned int slot,
>                                               unsigned int function);
> +
> +#ifdef CONFIG_XEN_PCI_PASSTHROUGH
>   extern bool has_igd_gfx_passthru;
> +#else
> +# define has_igd_gfx_passthru false
> +#endif
> +
>   static inline bool is_igd_vga_passthrough(XenHostPCIDevice *dev)
>   {
>       return (has_igd_gfx_passthru
> 

Re: [PATCH] xen: fix build without pci passthrough

[Roger Pau Monné]

On Mon, May 04, 2020 at 12:35:39PM +0200, Philippe Mathieu-Daudé wrote:
> Hi Roger,
> 
> On 5/4/20 12:14 PM, Roger Pau Monne wrote:
> > has_igd_gfx_passthru is only available when QEMU is built with
> > CONFIG_XEN_PCI_PASSTHROUGH, and hence shouldn't be used in common
> > code without checking if it's available.
> > 
> > Fixes: 46472d82322d0 ('xen: convert "-machine igd-passthru" to an accelerator property')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> See Kconfig fix suggested here:
> https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg61844.html

Having it available on Kconfig is indeed fine, but this still needs
some kind of configure checks AFAIK, as it's only available on Linux.

I'm certainly missing some context, but whether XEN_IGD_PASSTHROUGH
gets defined on Kconfig or not shouldn't really matter for this patch,
as we would still need to gate the code properly so it's not build
when PCI passthrough (or whatever name the option has) is not enabled?

Thanks, Roger.

Re: [PATCH] xen: fix build without pci passthrough

[Roger Pau Monné]

Ping?

On Mon, May 04, 2020 at 12:14:43PM +0200, Roger Pau Monne wrote:
> has_igd_gfx_passthru is only available when QEMU is built with
> CONFIG_XEN_PCI_PASSTHROUGH, and hence shouldn't be used in common
> code without checking if it's available.
> 
> Fixes: 46472d82322d0 ('xen: convert "-machine igd-passthru" to an accelerator property')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org
> ---
>  hw/xen/xen-common.c | 4 ++++
>  hw/xen/xen_pt.h     | 7 +++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> index a15070f7f6..c800862419 100644
> --- a/hw/xen/xen-common.c
> +++ b/hw/xen/xen-common.c
> @@ -127,6 +127,7 @@ static void xen_change_state_handler(void *opaque, int running,
>      }
>  }
>  
> +#ifdef CONFIG_XEN_PCI_PASSTHROUGH
>  static bool xen_get_igd_gfx_passthru(Object *obj, Error **errp)
>  {
>      return has_igd_gfx_passthru;
> @@ -136,6 +137,7 @@ static void xen_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
>  {
>      has_igd_gfx_passthru = value;
>  }
> +#endif
>  
>  static void xen_setup_post(MachineState *ms, AccelState *accel)
>  {
> @@ -197,11 +199,13 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
>  
>      compat_props_add(ac->compat_props, compat, G_N_ELEMENTS(compat));
>  
> +#ifdef CONFIG_XEN_PCI_PASSTHROUGH
>      object_class_property_add_bool(oc, "igd-passthru",
>          xen_get_igd_gfx_passthru, xen_set_igd_gfx_passthru,
>          &error_abort);
>      object_class_property_set_description(oc, "igd-passthru",
>          "Set on/off to enable/disable igd passthrou", &error_abort);
> +#endif
>  }
>  
>  #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> index 179775db7b..660dd8a008 100644
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -1,6 +1,7 @@
>  #ifndef XEN_PT_H
>  #define XEN_PT_H
>  
> +#include "qemu/osdep.h"
>  #include "hw/xen/xen_common.h"
>  #include "hw/pci/pci.h"
>  #include "xen-host-pci-device.h"
> @@ -322,7 +323,13 @@ extern void *pci_assign_dev_load_option_rom(PCIDevice *dev,
>                                              unsigned int domain,
>                                              unsigned int bus, unsigned int slot,
>                                              unsigned int function);
> +
> +#ifdef CONFIG_XEN_PCI_PASSTHROUGH
>  extern bool has_igd_gfx_passthru;
> +#else
> +# define has_igd_gfx_passthru false
> +#endif
> +
>  static inline bool is_igd_vga_passthrough(XenHostPCIDevice *dev)
>  {
>      return (has_igd_gfx_passthru
> -- 
> 2.26.2
> 

Re: [PATCH] xen: fix build without pci passthrough

[Anthony PERARD]

On Mon, May 04, 2020 at 12:14:43PM +0200, Roger Pau Monne wrote:
> has_igd_gfx_passthru is only available when QEMU is built with
> CONFIG_XEN_PCI_PASSTHROUGH, and hence shouldn't be used in common
> code without checking if it's available.
> 
> Fixes: 46472d82322d0 ('xen: convert "-machine igd-passthru" to an accelerator property')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org
> ---
>  hw/xen/xen-common.c | 4 ++++
>  hw/xen/xen_pt.h     | 7 +++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> index a15070f7f6..c800862419 100644
> --- a/hw/xen/xen-common.c
> +++ b/hw/xen/xen-common.c
> @@ -127,6 +127,7 @@ static void xen_change_state_handler(void *opaque, int running,
>      }
>  }
>  
> +#ifdef CONFIG_XEN_PCI_PASSTHROUGH
>  static bool xen_get_igd_gfx_passthru(Object *obj, Error **errp)
>  {
>      return has_igd_gfx_passthru;
> @@ -136,6 +137,7 @@ static void xen_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
>  {
>      has_igd_gfx_passthru = value;
>  }
> +#endif
>  
>  static void xen_setup_post(MachineState *ms, AccelState *accel)
>  {
> @@ -197,11 +199,13 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
>  
>      compat_props_add(ac->compat_props, compat, G_N_ELEMENTS(compat));
>  
> +#ifdef CONFIG_XEN_PCI_PASSTHROUGH
>      object_class_property_add_bool(oc, "igd-passthru",
>          xen_get_igd_gfx_passthru, xen_set_igd_gfx_passthru,
>          &error_abort);
>      object_class_property_set_description(oc, "igd-passthru",
>          "Set on/off to enable/disable igd passthrou", &error_abort);
> +#endif

It might not be a good idea to have the presence of class property
depending on what is built-in. Instead, I think it would be better to
return an error when QEMU is built without xen_pt and a user is trying
to enable it. That can be done by calling by calling error_setg(errp,
"nop") in xen_set_igd_gfx_passthru().

>  }
>  
>  #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> index 179775db7b..660dd8a008 100644
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -1,6 +1,7 @@
>  #ifndef XEN_PT_H
>  #define XEN_PT_H
>  
> +#include "qemu/osdep.h"

Why do you need osdep?

>  #include "hw/xen/xen_common.h"
>  #include "hw/pci/pci.h"
>  #include "xen-host-pci-device.h"
> @@ -322,7 +323,13 @@ extern void *pci_assign_dev_load_option_rom(PCIDevice *dev,
>                                              unsigned int domain,
>                                              unsigned int bus, unsigned int slot,
>                                              unsigned int function);
> +
> +#ifdef CONFIG_XEN_PCI_PASSTHROUGH
>  extern bool has_igd_gfx_passthru;
> +#else
> +# define has_igd_gfx_passthru false
> +#endif

I don't quite like the use of define here. Could you introduce a
function that return a bool instead? And defining that function in
hw/xen/xen.h like xen_enabled() would be fine I think.

Thanks,

-- 
Anthony PERARD

Re: [PATCH] xen: fix build without pci passthrough

[Roger Pau Monné]

On Mon, May 11, 2020 at 02:40:43PM +0100, Anthony PERARD wrote:
> On Mon, May 04, 2020 at 12:14:43PM +0200, Roger Pau Monne wrote:
> > diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> > index 179775db7b..660dd8a008 100644
> > --- a/hw/xen/xen_pt.h
> > +++ b/hw/xen/xen_pt.h
> > @@ -1,6 +1,7 @@
> >  #ifndef XEN_PT_H
> >  #define XEN_PT_H
> >  
> > +#include "qemu/osdep.h"
> 
> Why do you need osdep?

For CONFIG_XEN_PCI_PASSTHROUGH IIRC.

> 
> >  #include "hw/xen/xen_common.h"
> >  #include "hw/pci/pci.h"
> >  #include "xen-host-pci-device.h"
> > @@ -322,7 +323,13 @@ extern void *pci_assign_dev_load_option_rom(PCIDevice *dev,
> >                                              unsigned int domain,
> >                                              unsigned int bus, unsigned int slot,
> >                                              unsigned int function);
> > +
> > +#ifdef CONFIG_XEN_PCI_PASSTHROUGH
> >  extern bool has_igd_gfx_passthru;
> > +#else
> > +# define has_igd_gfx_passthru false
> > +#endif
> 
> I don't quite like the use of define here. Could you introduce a
> function that return a bool instead? And defining that function in
> hw/xen/xen.h like xen_enabled() would be fine I think.

But has_igd_gfx_passthru is defined in xen_pt.c which is only compiled
if CONFIG_XEN_PCI_PASSTHROUGH is defined, yet the variable is set from
xen-common.c. I think the former is fine, an any attempt to set
has_igd_gfx_passthru without CONFIG_XEN_PCI_PASSTHROUGH will result in
a compile error which is easier to catch?

Thanks, Roger.

Re: [PATCH] xen: fix build without pci passthrough

[Peter Maydell]

On Tue, 19 May 2020 at 12:28, Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Mon, May 11, 2020 at 02:40:43PM +0100, Anthony PERARD wrote:
> > On Mon, May 04, 2020 at 12:14:43PM +0200, Roger Pau Monne wrote:
> > > diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> > > index 179775db7b..660dd8a008 100644
> > > --- a/hw/xen/xen_pt.h
> > > +++ b/hw/xen/xen_pt.h
> > > @@ -1,6 +1,7 @@
> > >  #ifndef XEN_PT_H
> > >  #define XEN_PT_H
> > >
> > > +#include "qemu/osdep.h"
> >
> > Why do you need osdep?
>
> For CONFIG_XEN_PCI_PASSTHROUGH IIRC.

All .c files should always include osdep as the first include
in the file, and .h files should never include osdep (we note
this in CODING_STYLE.rst).

If you added this #include to fix a compile issue that would
suggest that there's a .c file somewhere that's missing the
mandatory osdep include. I did a quick eyeball of all the files
that include xen_pt.h, though, and none of them are missing the
osdep include. So I think you should be able to simply drop the
osdep include here. If that produces an error, let us know what
fails and we can work out what's gone wrong.

thanks
-- PMM

Re: [PATCH] xen: fix build without pci passthrough

[Roger Pau Monné]

On Tue, May 19, 2020 at 01:20:51PM +0100, Peter Maydell wrote:
> On Tue, 19 May 2020 at 12:28, Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> > On Mon, May 11, 2020 at 02:40:43PM +0100, Anthony PERARD wrote:
> > > On Mon, May 04, 2020 at 12:14:43PM +0200, Roger Pau Monne wrote:
> > > > diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> > > > index 179775db7b..660dd8a008 100644
> > > > --- a/hw/xen/xen_pt.h
> > > > +++ b/hw/xen/xen_pt.h
> > > > @@ -1,6 +1,7 @@
> > > >  #ifndef XEN_PT_H
> > > >  #define XEN_PT_H
> > > >
> > > > +#include "qemu/osdep.h"
> > >
> > > Why do you need osdep?
> >
> > For CONFIG_XEN_PCI_PASSTHROUGH IIRC.
> 
> All .c files should always include osdep as the first include
> in the file, and .h files should never include osdep (we note
> this in CODING_STYLE.rst).
> 
> If you added this #include to fix a compile issue that would
> suggest that there's a .c file somewhere that's missing the
> mandatory osdep include. I did a quick eyeball of all the files
> that include xen_pt.h, though, and none of them are missing the
> osdep include. So I think you should be able to simply drop the
> osdep include here. If that produces an error, let us know what
> fails and we can work out what's gone wrong.

My bad, didn't know about this rule and just looked up where
CONFIG_XEN_PCI_PASSTHROUGH was defined in order to include it. Will
remove in v2.

Thanks, Roger.