| Message ID | 20200728113712.22966-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Multiple fixes to XENMEM_acquire_resource | expand |
On 28.07.2020 13:37, Andrew Cooper wrote: > New architectures shouldn't be forced to implement no-op stubs for unused > functionality. > > Introduce CONFIG_ARCH_ACQUIRE_RESOURCE which can be opted in to, and provide > compatibility logic in xen/mm.h > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Jan Beulich <jbeulich@suse.com>
> -----Original Message----- > From: Andrew Cooper <andrew.cooper3@citrix.com> > Sent: 28 July 2020 12:37 > To: Xen-devel <xen-devel@lists.xenproject.org> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>; Wei Liu <wl@xen.org>; > Roger Pau Monné <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Julien Grall > <julien@xen.org>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Paul Durrant <paul@xen.org>; Michał > Leszczyński <michal.leszczynski@cert.pl>; Hubert Jasudowicz <hubert.jasudowicz@cert.pl> > Subject: [PATCH 1/5] xen/memory: Introduce CONFIG_ARCH_ACQUIRE_RESOURCE > > New architectures shouldn't be forced to implement no-op stubs for unused > functionality. > > Introduce CONFIG_ARCH_ACQUIRE_RESOURCE which can be opted in to, and provide > compatibility logic in xen/mm.h > > No functional change. Code-wise, it looks fine, so... Reviewed-by: Paul Durrant <paul@xen.org> ...but ... > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Wei Liu <wl@xen.org> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien@xen.org> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > CC: Paul Durrant <paul@xen.org> > CC: Michał Leszczyński <michal.leszczynski@cert.pl> > CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl> > --- > xen/arch/x86/Kconfig | 1 + > xen/common/Kconfig | 3 +++ > xen/include/asm-arm/mm.h | 8 -------- > xen/include/xen/mm.h | 9 +++++++++ > 4 files changed, 13 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig > index a636a4bb1e..e7644a0a9d 100644 > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -6,6 +6,7 @@ config X86 > select ACPI > select ACPI_LEGACY_TABLES_LOOKUP > select ARCH_SUPPORTS_INT128 > + select ARCH_ACQUIRE_RESOURCE ... I do wonder whether 'HAS_ACQUIRE_RESOURCE' is a better and more descriptive name. > select COMPAT > select CORE_PARKING > select HAS_ALTERNATIVE > diff --git a/xen/common/Kconfig b/xen/common/Kconfig > index 15e3b79ff5..593459ea6e 100644 > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -22,6 +22,9 @@ config GRANT_TABLE > > If unsure, say Y. > > +config ARCH_ACQUIRE_RESOURCE > + bool > + > config HAS_ALTERNATIVE > bool > > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h > index f8ba49b118..0b7de3102e 100644 > --- a/xen/include/asm-arm/mm.h > +++ b/xen/include/asm-arm/mm.h > @@ -358,14 +358,6 @@ static inline void put_page_and_type(struct page_info *page) > > void clear_and_clean_page(struct page_info *page); > > -static inline > -int arch_acquire_resource(struct domain *d, unsigned int type, unsigned int id, > - unsigned long frame, unsigned int nr_frames, > - xen_pfn_t mfn_list[]) > -{ > - return -EOPNOTSUPP; > -} > - > unsigned int arch_get_dma_bitsize(void); > > #endif /* __ARCH_ARM_MM__ */ > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h > index 1061765bcd..1b2c1f6b32 100644 > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -685,4 +685,13 @@ static inline void put_page_alloc_ref(struct page_info *page) > } > } > > +#ifndef CONFIG_ARCH_ACQUIRE_RESOURCE > +static inline int arch_acquire_resource( > + struct domain *d, unsigned int type, unsigned int id, unsigned long frame, > + unsigned int nr_frames, xen_pfn_t mfn_list[]) > +{ > + return -EOPNOTSUPP; > +} > +#endif /* !CONFIG_ARCH_ACQUIRE_RESOURCE */ > + > #endif /* __XEN_MM_H__ */ > -- > 2.11.0
Hi Andrew, On 28/07/2020 12:37, Andrew Cooper wrote: > New architectures shouldn't be forced to implement no-op stubs for unused > functionality. > > Introduce CONFIG_ARCH_ACQUIRE_RESOURCE which can be opted in to, and provide > compatibility logic in xen/mm.h > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> With one question below: Acked-by: Julien Grall <jgrall@amazon.com> > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h > index 1061765bcd..1b2c1f6b32 100644 > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -685,4 +685,13 @@ static inline void put_page_alloc_ref(struct page_info *page) > } > } > > +#ifndef CONFIG_ARCH_ACQUIRE_RESOURCE > +static inline int arch_acquire_resource( > + struct domain *d, unsigned int type, unsigned int id, unsigned long frame, > + unsigned int nr_frames, xen_pfn_t mfn_list[]) Any reason to change the way we indent the arguments? > +{ > + return -EOPNOTSUPP; > +} > +#endif /* !CONFIG_ARCH_ACQUIRE_RESOURCE */ > + > #endif /* __XEN_MM_H__ */ > Cheers,
On 30/07/2020 10:50, Julien Grall wrote: > Hi Andrew, > > On 28/07/2020 12:37, Andrew Cooper wrote: >> New architectures shouldn't be forced to implement no-op stubs for >> unused >> functionality. >> >> Introduce CONFIG_ARCH_ACQUIRE_RESOURCE which can be opted in to, and >> provide >> compatibility logic in xen/mm.h >> >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > With one question below: > > Acked-by: Julien Grall <jgrall@amazon.com> Thanks, > > >> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h >> index 1061765bcd..1b2c1f6b32 100644 >> --- a/xen/include/xen/mm.h >> +++ b/xen/include/xen/mm.h >> @@ -685,4 +685,13 @@ static inline void put_page_alloc_ref(struct >> page_info *page) >> } >> } >> +#ifndef CONFIG_ARCH_ACQUIRE_RESOURCE >> +static inline int arch_acquire_resource( >> + struct domain *d, unsigned int type, unsigned int id, unsigned >> long frame, >> + unsigned int nr_frames, xen_pfn_t mfn_list[]) > > Any reason to change the way we indent the arguments? So its not all squashed on the right hand side. ~Andrew
On 30/07/2020 09:02, Paul Durrant wrote: >> -----Original Message----- >> From: Andrew Cooper <andrew.cooper3@citrix.com> >> Sent: 28 July 2020 12:37 >> To: Xen-devel <xen-devel@lists.xenproject.org> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>; Wei Liu <wl@xen.org>; >> Roger Pau Monné <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Julien Grall >> <julien@xen.org>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Paul Durrant <paul@xen.org>; Michał >> Leszczyński <michal.leszczynski@cert.pl>; Hubert Jasudowicz <hubert.jasudowicz@cert.pl> >> Subject: [PATCH 1/5] xen/memory: Introduce CONFIG_ARCH_ACQUIRE_RESOURCE >> >> New architectures shouldn't be forced to implement no-op stubs for unused >> functionality. >> >> Introduce CONFIG_ARCH_ACQUIRE_RESOURCE which can be opted in to, and provide >> compatibility logic in xen/mm.h >> >> No functional change. > Code-wise, it looks fine, so... > > Reviewed-by: Paul Durrant <paul@xen.org> Thanks, > > ...but ... > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Wei Liu <wl@xen.org> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> CC: Stefano Stabellini <sstabellini@kernel.org> >> CC: Julien Grall <julien@xen.org> >> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >> CC: Paul Durrant <paul@xen.org> >> CC: Michał Leszczyński <michal.leszczynski@cert.pl> >> CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl> >> --- >> xen/arch/x86/Kconfig | 1 + >> xen/common/Kconfig | 3 +++ >> xen/include/asm-arm/mm.h | 8 -------- >> xen/include/xen/mm.h | 9 +++++++++ >> 4 files changed, 13 insertions(+), 8 deletions(-) >> >> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig >> index a636a4bb1e..e7644a0a9d 100644 >> --- a/xen/arch/x86/Kconfig >> +++ b/xen/arch/x86/Kconfig >> @@ -6,6 +6,7 @@ config X86 >> select ACPI >> select ACPI_LEGACY_TABLES_LOOKUP >> select ARCH_SUPPORTS_INT128 >> + select ARCH_ACQUIRE_RESOURCE > ... I do wonder whether 'HAS_ACQUIRE_RESOURCE' is a better and more descriptive name. We don't have a coherent policy for how to categorise these things. I can change the name if you insist, but I'm not sure it makes a useful difference. ~Andrew
> -----Original Message----- > From: Andrew Cooper <andrew.cooper3@citrix.com> > Sent: 30 July 2020 18:34 > To: paul@xen.org; 'Xen-devel' <xen-devel@lists.xenproject.org> > Cc: 'Jan Beulich' <JBeulich@suse.com>; 'Wei Liu' <wl@xen.org>; 'Roger Pau Monné' > <roger.pau@citrix.com>; 'Stefano Stabellini' <sstabellini@kernel.org>; 'Julien Grall' > <julien@xen.org>; 'Volodymyr Babchuk' <Volodymyr_Babchuk@epam.com>; 'Michał Leszczyński' > <michal.leszczynski@cert.pl>; 'Hubert Jasudowicz' <hubert.jasudowicz@cert.pl> > Subject: Re: [PATCH 1/5] xen/memory: Introduce CONFIG_ARCH_ACQUIRE_RESOURCE > > On 30/07/2020 09:02, Paul Durrant wrote: > >> -----Original Message----- > >> From: Andrew Cooper <andrew.cooper3@citrix.com> > >> Sent: 28 July 2020 12:37 > >> To: Xen-devel <xen-devel@lists.xenproject.org> > >> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>; Wei Liu > <wl@xen.org>; > >> Roger Pau Monné <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Julien Grall > >> <julien@xen.org>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Paul Durrant <paul@xen.org>; > Michał > >> Leszczyński <michal.leszczynski@cert.pl>; Hubert Jasudowicz <hubert.jasudowicz@cert.pl> > >> Subject: [PATCH 1/5] xen/memory: Introduce CONFIG_ARCH_ACQUIRE_RESOURCE > >> > >> New architectures shouldn't be forced to implement no-op stubs for unused > >> functionality. > >> > >> Introduce CONFIG_ARCH_ACQUIRE_RESOURCE which can be opted in to, and provide > >> compatibility logic in xen/mm.h > >> > >> No functional change. > > Code-wise, it looks fine, so... > > > > Reviewed-by: Paul Durrant <paul@xen.org> > > Thanks, > > > > > ...but ... > > > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > >> --- > >> CC: Jan Beulich <JBeulich@suse.com> > >> CC: Wei Liu <wl@xen.org> > >> CC: Roger Pau Monné <roger.pau@citrix.com> > >> CC: Stefano Stabellini <sstabellini@kernel.org> > >> CC: Julien Grall <julien@xen.org> > >> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > >> CC: Paul Durrant <paul@xen.org> > >> CC: Michał Leszczyński <michal.leszczynski@cert.pl> > >> CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl> > >> --- > >> xen/arch/x86/Kconfig | 1 + > >> xen/common/Kconfig | 3 +++ > >> xen/include/asm-arm/mm.h | 8 -------- > >> xen/include/xen/mm.h | 9 +++++++++ > >> 4 files changed, 13 insertions(+), 8 deletions(-) > >> > >> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig > >> index a636a4bb1e..e7644a0a9d 100644 > >> --- a/xen/arch/x86/Kconfig > >> +++ b/xen/arch/x86/Kconfig > >> @@ -6,6 +6,7 @@ config X86 > >> select ACPI > >> select ACPI_LEGACY_TABLES_LOOKUP > >> select ARCH_SUPPORTS_INT128 > >> + select ARCH_ACQUIRE_RESOURCE > > ... I do wonder whether 'HAS_ACQUIRE_RESOURCE' is a better and more descriptive name. > > We don't have a coherent policy for how to categorise these things. I > can change the name if you insist, but I'm not sure it makes a useful > difference. > Ok, it's fine. My R-b stands. Paul > ~Andrew
On 30/07/2020 18:28, Andrew Cooper wrote: > On 30/07/2020 10:50, Julien Grall wrote: >> Hi Andrew, >> >> On 28/07/2020 12:37, Andrew Cooper wrote: >>> New architectures shouldn't be forced to implement no-op stubs for >>> unused >>> functionality. >>> >>> Introduce CONFIG_ARCH_ACQUIRE_RESOURCE which can be opted in to, and >>> provide >>> compatibility logic in xen/mm.h >>> >>> No functional change. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> >> With one question below: >> >> Acked-by: Julien Grall <jgrall@amazon.com> > > Thanks, > >> >> >>> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h >>> index 1061765bcd..1b2c1f6b32 100644 >>> --- a/xen/include/xen/mm.h >>> +++ b/xen/include/xen/mm.h >>> @@ -685,4 +685,13 @@ static inline void put_page_alloc_ref(struct >>> page_info *page) >>> } >>> } >>> +#ifndef CONFIG_ARCH_ACQUIRE_RESOURCE >>> +static inline int arch_acquire_resource( >>> + struct domain *d, unsigned int type, unsigned int id, unsigned >>> long frame, >>> + unsigned int nr_frames, xen_pfn_t mfn_list[]) >> >> Any reason to change the way we indent the arguments? > > So its not all squashed on the right hand side. Fair enough. I have asked the same question on a follow-up patch. Feel free to ignore it :). Cheers,
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index a636a4bb1e..e7644a0a9d 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -6,6 +6,7 @@ config X86 select ACPI select ACPI_LEGACY_TABLES_LOOKUP select ARCH_SUPPORTS_INT128 + select ARCH_ACQUIRE_RESOURCE select COMPAT select CORE_PARKING select HAS_ALTERNATIVE diff --git a/xen/common/Kconfig b/xen/common/Kconfig index 15e3b79ff5..593459ea6e 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -22,6 +22,9 @@ config GRANT_TABLE If unsure, say Y. +config ARCH_ACQUIRE_RESOURCE + bool + config HAS_ALTERNATIVE bool diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h index f8ba49b118..0b7de3102e 100644 --- a/xen/include/asm-arm/mm.h +++ b/xen/include/asm-arm/mm.h @@ -358,14 +358,6 @@ static inline void put_page_and_type(struct page_info *page) void clear_and_clean_page(struct page_info *page); -static inline -int arch_acquire_resource(struct domain *d, unsigned int type, unsigned int id, - unsigned long frame, unsigned int nr_frames, - xen_pfn_t mfn_list[]) -{ - return -EOPNOTSUPP; -} - unsigned int arch_get_dma_bitsize(void); #endif /* __ARCH_ARM_MM__ */ diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index 1061765bcd..1b2c1f6b32 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -685,4 +685,13 @@ static inline void put_page_alloc_ref(struct page_info *page) } } +#ifndef CONFIG_ARCH_ACQUIRE_RESOURCE +static inline int arch_acquire_resource( + struct domain *d, unsigned int type, unsigned int id, unsigned long frame, + unsigned int nr_frames, xen_pfn_t mfn_list[]) +{ + return -EOPNOTSUPP; +} +#endif /* !CONFIG_ARCH_ACQUIRE_RESOURCE */ + #endif /* __XEN_MM_H__ */
New architectures shouldn't be forced to implement no-op stubs for unused functionality. Introduce CONFIG_ARCH_ACQUIRE_RESOURCE which can be opted in to, and provide compatibility logic in xen/mm.h No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Paul Durrant <paul@xen.org> CC: Michał Leszczyński <michal.leszczynski@cert.pl> CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl> --- xen/arch/x86/Kconfig | 1 + xen/common/Kconfig | 3 +++ xen/include/asm-arm/mm.h | 8 -------- xen/include/xen/mm.h | 9 +++++++++ 4 files changed, 13 insertions(+), 8 deletions(-)