1. Patchew
  2. Xen
  3. [PATCH 0/5] Multiple fixes to XENMEM_acquire_resource
  4. View patch

[PATCH 1/5] xen/memory: Introduce CONFIG_ARCH_ACQUIRE_RESOURCE

Andrew Cooper posted 5 patches 5 years, 6 months ago
There is a newer version of this series
[PATCH 1/5] xen/memory: Introduce CONFIG_ARCH_ACQUIRE_RESOURCE
Posted by Andrew Cooper 5 years, 6 months ago 
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(-)

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__ */
-- 
2.11.0
Re: [PATCH 1/5] xen/memory: Introduce CONFIG_ARCH_ACQUIRE_RESOURCE
Posted by Julien Grall 5 years, 6 months ago 
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,

-- 
Julien Grall
Re: [PATCH 1/5] xen/memory: Introduce CONFIG_ARCH_ACQUIRE_RESOURCE
Posted by Andrew Cooper 5 years, 6 months ago 
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
Re: [PATCH 1/5] xen/memory: Introduce CONFIG_ARCH_ACQUIRE_RESOURCE
Posted by Julien Grall 5 years, 6 months ago 

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,

-- 
Julien Grall
RE: [PATCH 1/5] xen/memory: Introduce CONFIG_ARCH_ACQUIRE_RESOURCE
Posted by Paul Durrant 5 years, 6 months ago 
> -----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
Re: [PATCH 1/5] xen/memory: Introduce CONFIG_ARCH_ACQUIRE_RESOURCE
Posted by Andrew Cooper 5 years, 6 months ago 
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
RE: [PATCH 1/5] xen/memory: Introduce CONFIG_ARCH_ACQUIRE_RESOURCE
Posted by Paul Durrant 5 years, 6 months ago 
> -----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
Re: [PATCH 1/5] xen/memory: Introduce CONFIG_ARCH_ACQUIRE_RESOURCE
Posted by Jan Beulich 5 years, 6 months ago 
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>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.