* Re: [PATCH] xen: fix build without pci passthrough
2020-05-04 10:14 [PATCH] xen: fix build without pci passthrough Roger Pau Monne
@ 2020-05-04 10:35 ` Philippe Mathieu-Daudé
2020-05-04 10:45 ` Roger Pau Monné
2020-05-11 8:14 ` Roger Pau Monné
2020-05-11 13:40 ` Anthony PERARD
2 siblings, 1 reply; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-04 10:35 UTC (permalink / raw)
To: Roger Pau Monne, qemu-devel
Cc: Anthony Perard, xen-devel, Stefano Stabellini, Paul Durrant
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] xen: fix build without pci passthrough
2020-05-04 10:35 ` Philippe Mathieu-Daudé
@ 2020-05-04 10:45 ` Roger Pau Monné
0 siblings, 0 replies; 8+ messages in thread
From: Roger Pau Monné @ 2020-05-04 10:45 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Anthony Perard, xen-devel, Stefano Stabellini, qemu-devel, Paul Durrant
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] xen: fix build without pci passthrough
2020-05-04 10:14 [PATCH] xen: fix build without pci passthrough Roger Pau Monne
2020-05-04 10:35 ` Philippe Mathieu-Daudé
@ 2020-05-11 8:14 ` Roger Pau Monné
2020-05-11 13:40 ` Anthony PERARD
2 siblings, 0 replies; 8+ messages in thread
From: Roger Pau Monné @ 2020-05-11 8:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Anthony Perard, xen-devel, Stefano Stabellini, Paul Durrant
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] xen: fix build without pci passthrough
2020-05-04 10:14 [PATCH] xen: fix build without pci passthrough Roger Pau Monne
2020-05-04 10:35 ` Philippe Mathieu-Daudé
2020-05-11 8:14 ` Roger Pau Monné
@ 2020-05-11 13:40 ` Anthony PERARD
2020-05-19 11:28 ` Roger Pau Monné
2 siblings, 1 reply; 8+ messages in thread
From: Anthony PERARD @ 2020-05-11 13:40 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel, Stefano Stabellini, qemu-devel, Paul Durrant
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] xen: fix build without pci passthrough
2020-05-11 13:40 ` Anthony PERARD
@ 2020-05-19 11:28 ` Roger Pau Monné
2020-05-19 12:20 ` Peter Maydell
0 siblings, 1 reply; 8+ messages in thread
From: Roger Pau Monné @ 2020-05-19 11:28 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Stefano Stabellini, qemu-devel, Paul Durrant
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] xen: fix build without pci passthrough
2020-05-19 11:28 ` Roger Pau Monné
@ 2020-05-19 12:20 ` Peter Maydell
2020-05-19 14:15 ` Roger Pau Monné
0 siblings, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2020-05-19 12:20 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Anthony PERARD, open list:X86, Stefano Stabellini,
QEMU Developers, Paul Durrant
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] xen: fix build without pci passthrough
2020-05-19 12:20 ` Peter Maydell
@ 2020-05-19 14:15 ` Roger Pau Monné
0 siblings, 0 replies; 8+ messages in thread
From: Roger Pau Monné @ 2020-05-19 14:15 UTC (permalink / raw)
To: Peter Maydell
Cc: Anthony PERARD, open list:X86, Stefano Stabellini,
QEMU Developers, Paul Durrant
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.
^ permalink raw reply [flat|nested] 8+ messages in thread