| Message ID | 20200504101443.3165-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: fix build without pci passthrough | expand |
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 >
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.
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 >
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,
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.
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
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.
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
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(+)