| Message ID | 20200804074930.13104-2-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | accel/xen: Fix xen_enabled() behavior on target-agnostic objects | expand |
> -----Original Message----- > From: Philippe Mathieu-Daudé <philmd@redhat.com> > Sent: 04 August 2020 08:50 > To: qemu-devel@nongnu.org > Cc: Peter Maydell <peter.maydell@linaro.org>; Anthony Perard <anthony.perard@citrix.com>; Paolo > Bonzini <pbonzini@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>; xen- > devel@lists.xenproject.org; Paul Durrant <paul@xen.org>; Philippe Mathieu-Daudé <philmd@redhat.com>; > Paul Durrant <pdurrant@amazon.com> > Subject: [PATCH-for-5.1 v2 1/1] accel/xen: Fix xen_enabled() behavior on target-agnostic objects > > CONFIG_XEN is generated by configure and stored in "config-target.h", > which is (obviously) only include for target-specific objects. > This is a problem for target-agnostic objects as CONFIG_XEN is never > defined and xen_enabled() is always inlined as 'false'. > > Fix by following the KVM schema, defining CONFIG_XEN_IS_POSSIBLE > when we don't know to force the call of the non-inlined function, > returning the xen_allowed boolean. > > Fixes: da278d58a092 ("accel: Move Xen accelerator code under accel/xen/") > Reported-by: Paul Durrant <pdurrant@amazon.com> > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > include/sysemu/xen.h | 18 ++++++++++++++---- > accel/stubs/xen-stub.c | 2 ++ > accel/xen/xen-all.c | 7 +------ > 3 files changed, 17 insertions(+), 10 deletions(-) > > diff --git a/include/sysemu/xen.h b/include/sysemu/xen.h > index 1ca292715e..2c2c429ea8 100644 > --- a/include/sysemu/xen.h > +++ b/include/sysemu/xen.h > @@ -8,9 +8,19 @@ > #ifndef SYSEMU_XEN_H > #define SYSEMU_XEN_H > > -#ifdef CONFIG_XEN > +#ifdef NEED_CPU_H > +# ifdef CONFIG_XEN > +# define CONFIG_XEN_IS_POSSIBLE > +# endif > +#else > +# define CONFIG_XEN_IS_POSSIBLE > +#endif > > -bool xen_enabled(void); > +#ifdef CONFIG_XEN_IS_POSSIBLE > + > +extern bool xen_allowed; > + > +#define xen_enabled() (xen_allowed) Can this not move ahead of the #ifdef now (since xen_allowed is present in both xen-stub and xen-all)? I think this is what Peter was saying in his option '(2)'. Paul > > #ifndef CONFIG_USER_ONLY > void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length); > @@ -18,7 +28,7 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, > struct MemoryRegion *mr, Error **errp); > #endif > > -#else /* !CONFIG_XEN */ > +#else /* !CONFIG_XEN_IS_POSSIBLE */ > > #define xen_enabled() 0 > #ifndef CONFIG_USER_ONLY > @@ -33,6 +43,6 @@ static inline void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, > } > #endif > > -#endif /* CONFIG_XEN */ > +#endif /* CONFIG_XEN_IS_POSSIBLE */ > > #endif > diff --git a/accel/stubs/xen-stub.c b/accel/stubs/xen-stub.c > index dcca4e678a..8ae658acff 100644 > --- a/accel/stubs/xen-stub.c > +++ b/accel/stubs/xen-stub.c > @@ -9,6 +9,8 @@ > #include "hw/xen/xen.h" > #include "qapi/qapi-commands-misc.h" > > +bool xen_allowed; > + > void xenstore_store_pv_console_info(int i, Chardev *chr) > { > } > diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c > index 0c24d4b191..60b971d0a8 100644 > --- a/accel/xen/xen-all.c > +++ b/accel/xen/xen-all.c > @@ -32,12 +32,7 @@ > do { } while (0) > #endif > > -static bool xen_allowed; > - > -bool xen_enabled(void) > -{ > - return xen_allowed; > -} > +bool xen_allowed; > > xc_interface *xen_xc; > xenforeignmemory_handle *xen_fmem; > -- > 2.21.3
Hi Paul, On 8/4/20 9:57 AM, Paul Durrant wrote: >> -----Original Message----- >> From: Philippe Mathieu-Daudé <philmd@redhat.com> >> Sent: 04 August 2020 08:50 >> To: qemu-devel@nongnu.org >> Cc: Peter Maydell <peter.maydell@linaro.org>; Anthony Perard <anthony.perard@citrix.com>; Paolo >> Bonzini <pbonzini@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>; xen- >> devel@lists.xenproject.org; Paul Durrant <paul@xen.org>; Philippe Mathieu-Daudé <philmd@redhat.com>; >> Paul Durrant <pdurrant@amazon.com> >> Subject: [PATCH-for-5.1 v2 1/1] accel/xen: Fix xen_enabled() behavior on target-agnostic objects >> >> CONFIG_XEN is generated by configure and stored in "config-target.h", >> which is (obviously) only include for target-specific objects. >> This is a problem for target-agnostic objects as CONFIG_XEN is never >> defined and xen_enabled() is always inlined as 'false'. >> >> Fix by following the KVM schema, defining CONFIG_XEN_IS_POSSIBLE >> when we don't know to force the call of the non-inlined function, >> returning the xen_allowed boolean. >> >> Fixes: da278d58a092 ("accel: Move Xen accelerator code under accel/xen/") >> Reported-by: Paul Durrant <pdurrant@amazon.com> >> Suggested-by: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> include/sysemu/xen.h | 18 ++++++++++++++---- >> accel/stubs/xen-stub.c | 2 ++ >> accel/xen/xen-all.c | 7 +------ >> 3 files changed, 17 insertions(+), 10 deletions(-) >> >> diff --git a/include/sysemu/xen.h b/include/sysemu/xen.h >> index 1ca292715e..2c2c429ea8 100644 >> --- a/include/sysemu/xen.h >> +++ b/include/sysemu/xen.h >> @@ -8,9 +8,19 @@ >> #ifndef SYSEMU_XEN_H >> #define SYSEMU_XEN_H >> >> -#ifdef CONFIG_XEN >> +#ifdef NEED_CPU_H >> +# ifdef CONFIG_XEN >> +# define CONFIG_XEN_IS_POSSIBLE >> +# endif >> +#else >> +# define CONFIG_XEN_IS_POSSIBLE >> +#endif >> >> -bool xen_enabled(void); >> +#ifdef CONFIG_XEN_IS_POSSIBLE >> + >> +extern bool xen_allowed; >> + >> +#define xen_enabled() (xen_allowed) > > Can this not move ahead of the #ifdef now (since xen_allowed is present in both xen-stub and xen-all)? I think this is what Peter was saying in his option '(2)'. I think I respected Peter's option '(2)', following how KVM does, this is the case with stub, > > Paul > >> >> #ifndef CONFIG_USER_ONLY >> void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length); >> @@ -18,7 +28,7 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, >> struct MemoryRegion *mr, Error **errp); >> #endif >> >> -#else /* !CONFIG_XEN */ >> +#else /* !CONFIG_XEN_IS_POSSIBLE */ >> >> #define xen_enabled() 0 ^^^ here is the other case, >> #ifndef CONFIG_USER_ONLY >> @@ -33,6 +43,6 @@ static inline void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, >> } >> #endif >> >> -#endif /* CONFIG_XEN */ >> +#endif /* CONFIG_XEN_IS_POSSIBLE */ >> >> #endif >> diff --git a/accel/stubs/xen-stub.c b/accel/stubs/xen-stub.c >> index dcca4e678a..8ae658acff 100644 >> --- a/accel/stubs/xen-stub.c >> +++ b/accel/stubs/xen-stub.c >> @@ -9,6 +9,8 @@ >> #include "hw/xen/xen.h" >> #include "qapi/qapi-commands-misc.h" >> >> +bool xen_allowed; here is the stub, >> + >> void xenstore_store_pv_console_info(int i, Chardev *chr) >> { >> } >> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c >> index 0c24d4b191..60b971d0a8 100644 >> --- a/accel/xen/xen-all.c >> +++ b/accel/xen/xen-all.c >> @@ -32,12 +32,7 @@ >> do { } while (0) >> #endif >> >> -static bool xen_allowed; >> - >> -bool xen_enabled(void) >> -{ >> - return xen_allowed; >> -} >> +bool xen_allowed; here is the real variable. >> >> xc_interface *xen_xc; >> xenforeignmemory_handle *xen_fmem; >> -- >> 2.21.3 > >
> -----Original Message----- > From: Philippe Mathieu-Daudé <philmd@redhat.com> > Sent: 04 August 2020 09:35 > To: paul@xen.org; qemu-devel@nongnu.org > Cc: 'Peter Maydell' <peter.maydell@linaro.org>; 'Anthony Perard' <anthony.perard@citrix.com>; 'Paolo > Bonzini' <pbonzini@redhat.com>; 'Stefano Stabellini' <sstabellini@kernel.org>; xen- > devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com> > Subject: Re: [PATCH-for-5.1 v2 1/1] accel/xen: Fix xen_enabled() behavior on target-agnostic objects > > Hi Paul, > > On 8/4/20 9:57 AM, Paul Durrant wrote: > >> -----Original Message----- > >> From: Philippe Mathieu-Daudé <philmd@redhat.com> > >> Sent: 04 August 2020 08:50 > >> To: qemu-devel@nongnu.org > >> Cc: Peter Maydell <peter.maydell@linaro.org>; Anthony Perard <anthony.perard@citrix.com>; Paolo > >> Bonzini <pbonzini@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>; xen- > >> devel@lists.xenproject.org; Paul Durrant <paul@xen.org>; Philippe Mathieu-Daudé > <philmd@redhat.com>; > >> Paul Durrant <pdurrant@amazon.com> > >> Subject: [PATCH-for-5.1 v2 1/1] accel/xen: Fix xen_enabled() behavior on target-agnostic objects > >> > >> CONFIG_XEN is generated by configure and stored in "config-target.h", > >> which is (obviously) only include for target-specific objects. > >> This is a problem for target-agnostic objects as CONFIG_XEN is never > >> defined and xen_enabled() is always inlined as 'false'. > >> > >> Fix by following the KVM schema, defining CONFIG_XEN_IS_POSSIBLE > >> when we don't know to force the call of the non-inlined function, > >> returning the xen_allowed boolean. > >> > >> Fixes: da278d58a092 ("accel: Move Xen accelerator code under accel/xen/") > >> Reported-by: Paul Durrant <pdurrant@amazon.com> > >> Suggested-by: Peter Maydell <peter.maydell@linaro.org> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> --- > >> include/sysemu/xen.h | 18 ++++++++++++++---- > >> accel/stubs/xen-stub.c | 2 ++ > >> accel/xen/xen-all.c | 7 +------ > >> 3 files changed, 17 insertions(+), 10 deletions(-) > >> > >> diff --git a/include/sysemu/xen.h b/include/sysemu/xen.h > >> index 1ca292715e..2c2c429ea8 100644 > >> --- a/include/sysemu/xen.h > >> +++ b/include/sysemu/xen.h > >> @@ -8,9 +8,19 @@ > >> #ifndef SYSEMU_XEN_H > >> #define SYSEMU_XEN_H > >> > >> -#ifdef CONFIG_XEN > >> +#ifdef NEED_CPU_H > >> +# ifdef CONFIG_XEN > >> +# define CONFIG_XEN_IS_POSSIBLE > >> +# endif > >> +#else > >> +# define CONFIG_XEN_IS_POSSIBLE > >> +#endif > >> > >> -bool xen_enabled(void); > >> +#ifdef CONFIG_XEN_IS_POSSIBLE > >> + > >> +extern bool xen_allowed; > >> + > >> +#define xen_enabled() (xen_allowed) > > > > Can this not move ahead of the #ifdef now (since xen_allowed is present in both xen-stub and xen- > all)? I think this is what Peter was saying in his option '(2)'. > > I think I respected Peter's option '(2)', following how KVM does, this > is the case with stub, Ok, if it follows the KVM pattern then that's fine. Reviewed-by: Paul Durrant <paul@xen.org>
On Tue, Aug 04, 2020 at 09:49:30AM +0200, Philippe Mathieu-Daudé wrote: > CONFIG_XEN is generated by configure and stored in "config-target.h", > which is (obviously) only include for target-specific objects. > This is a problem for target-agnostic objects as CONFIG_XEN is never > defined and xen_enabled() is always inlined as 'false'. > > Fix by following the KVM schema, defining CONFIG_XEN_IS_POSSIBLE > when we don't know to force the call of the non-inlined function, > returning the xen_allowed boolean. > > Fixes: da278d58a092 ("accel: Move Xen accelerator code under accel/xen/") > Reported-by: Paul Durrant <pdurrant@amazon.com> > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com> This time, I've also check that build all targets works... I'll prepare the pull request right away. Thank you,
diff --git a/include/sysemu/xen.h b/include/sysemu/xen.h index 1ca292715e..2c2c429ea8 100644 --- a/include/sysemu/xen.h +++ b/include/sysemu/xen.h @@ -8,9 +8,19 @@ #ifndef SYSEMU_XEN_H #define SYSEMU_XEN_H -#ifdef CONFIG_XEN +#ifdef NEED_CPU_H +# ifdef CONFIG_XEN +# define CONFIG_XEN_IS_POSSIBLE +# endif +#else +# define CONFIG_XEN_IS_POSSIBLE +#endif -bool xen_enabled(void); +#ifdef CONFIG_XEN_IS_POSSIBLE + +extern bool xen_allowed; + +#define xen_enabled() (xen_allowed) #ifndef CONFIG_USER_ONLY void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length); @@ -18,7 +28,7 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, struct MemoryRegion *mr, Error **errp); #endif -#else /* !CONFIG_XEN */ +#else /* !CONFIG_XEN_IS_POSSIBLE */ #define xen_enabled() 0 #ifndef CONFIG_USER_ONLY @@ -33,6 +43,6 @@ static inline void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, } #endif -#endif /* CONFIG_XEN */ +#endif /* CONFIG_XEN_IS_POSSIBLE */ #endif diff --git a/accel/stubs/xen-stub.c b/accel/stubs/xen-stub.c index dcca4e678a..8ae658acff 100644 --- a/accel/stubs/xen-stub.c +++ b/accel/stubs/xen-stub.c @@ -9,6 +9,8 @@ #include "hw/xen/xen.h" #include "qapi/qapi-commands-misc.h" +bool xen_allowed; + void xenstore_store_pv_console_info(int i, Chardev *chr) { } diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c index 0c24d4b191..60b971d0a8 100644 --- a/accel/xen/xen-all.c +++ b/accel/xen/xen-all.c @@ -32,12 +32,7 @@ do { } while (0) #endif -static bool xen_allowed; - -bool xen_enabled(void) -{ - return xen_allowed; -} +bool xen_allowed; xc_interface *xen_xc; xenforeignmemory_handle *xen_fmem;
CONFIG_XEN is generated by configure and stored in "config-target.h", which is (obviously) only include for target-specific objects. This is a problem for target-agnostic objects as CONFIG_XEN is never defined and xen_enabled() is always inlined as 'false'. Fix by following the KVM schema, defining CONFIG_XEN_IS_POSSIBLE when we don't know to force the call of the non-inlined function, returning the xen_allowed boolean. Fixes: da278d58a092 ("accel: Move Xen accelerator code under accel/xen/") Reported-by: Paul Durrant <pdurrant@amazon.com> Suggested-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- include/sysemu/xen.h | 18 ++++++++++++++---- accel/stubs/xen-stub.c | 2 ++ accel/xen/xen-all.c | 7 +------ 3 files changed, 17 insertions(+), 10 deletions(-)