[PATCH 0/5] Multiple fixes to XENMEM_acquire_resource

[PATCH 0/5] Multiple fixes to XENMEM_acquire_resource

[Andrew Cooper]

I thought this was going to be a very simple small bugfix for Michał's
Processor Trace series.  Serves me right for expecting it not to be full of
bear traps...

The sole implementation of acquire_resource never asks for size, so its little
surprise that Xen is broken for compat callers, and returns incorrect
information for regular callers.

Patches 1-2 are cleanup with no net functional change.  Patches 3-5 are fixes
to the size side of this interface, and allow userspace to obtain the actual
size of resources.

I'm still working on fixing the batch mapping support, which has further sharp
corners, especially for compat callers.

This is sufficenet of a series so far to post for comments.  A branch can be
obtained from:

  https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/xen-acquire-size

Andrew Cooper (5):
  xen/memory: Introduce CONFIG_ARCH_ACQUIRE_RESOURCE
  xen/gnttab: Rework resource acquisition
  xen/memory: Fix compat XENMEM_acquire_resource for size requests
  xen/memory: Fix acquire_resource size semantics
  tools/foreignmem: Support querying the size of a resource

 tools/libs/foreignmemory/Makefile                  |   2 +-
 tools/libs/foreignmemory/core.c                    |  14 +++
 .../libs/foreignmemory/include/xenforeignmemory.h  |  15 +++
 tools/libs/foreignmemory/libxenforeignmemory.map   |   4 +
 tools/libs/foreignmemory/linux.c                   |  35 +++++++
 tools/libs/foreignmemory/private.h                 |  14 +++
 xen/arch/x86/Kconfig                               |   1 +
 xen/arch/x86/mm.c                                  |  20 ++++
 xen/common/Kconfig                                 |   3 +
 xen/common/compat/memory.c                         |   2 +-
 xen/common/grant_table.c                           | 112 ++++++++++++++++-----
 xen/common/memory.c                                |  85 +++++++---------
 xen/include/asm-arm/mm.h                           |   8 --
 xen/include/asm-x86/mm.h                           |   3 +
 xen/include/public/memory.h                        |  16 ++-
 xen/include/xen/grant_table.h                      |  21 ++--
 xen/include/xen/mm.h                               |  15 +++
 17 files changed, 273 insertions(+), 97 deletions(-)

-- 
2.11.0

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

[Andrew Cooper]

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

[Jan Beulich]

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>

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

[Paul Durrant]

> -----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

[Andrew Cooper]

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

[Paul Durrant]

> -----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

[Julien Grall]

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

[Andrew Cooper]

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

[Julien Grall]

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

[PATCH 2/5] xen/gnttab: Rework resource acquisition

[Andrew Cooper]

The existing logic doesn't function in the general case for mapping a guests
grant table, due to arbitrary 32 frame limit, and the default grant table
limit being 64.

In order to start addressing this, rework the existing grant table logic by
implementing a single gnttab_acquire_resource().  This is far more efficient
than the previous acquire_grant_table() in memory.c because it doesn't take
the grant table write lock, and attempt to grow the table, for every single
frame.

The new gnttab_acquire_resource() function subsumes the previous two
gnttab_get_{shared,status}_frame() helpers.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>
CC: Paul Durrant <paul@xen.org>
CC: Michał Leszczyński <michal.leszczynski@cert.pl>
CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
---
 xen/common/grant_table.c      | 93 ++++++++++++++++++++++++++++++-------------
 xen/common/memory.c           | 42 +------------------
 xen/include/xen/grant_table.h | 19 ++++-----
 3 files changed, 75 insertions(+), 79 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 9f0cae52c0..122d1e7596 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -4013,6 +4013,72 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
     return 0;
 }
 
+int gnttab_acquire_resource(
+    struct domain *d, unsigned int id, unsigned long frame,
+    unsigned int nr_frames, xen_pfn_t mfn_list[])
+{
+    struct grant_table *gt = d->grant_table;
+    unsigned int i = nr_frames, tot_frames;
+    void **vaddrs;
+    int rc = 0;
+
+    /* Input sanity. */
+    if ( !nr_frames )
+        return -EINVAL;
+
+    /* Overflow checks */
+    if ( frame + nr_frames < frame )
+        return -EINVAL;
+
+    tot_frames = frame + nr_frames;
+    if ( tot_frames != frame + nr_frames )
+        return -EINVAL;
+
+    /* Grow table if necessary. */
+    grant_write_lock(gt);
+    switch ( id )
+    {
+        mfn_t tmp;
+
+    case XENMEM_resource_grant_table_id_shared:
+        rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp);
+        break;
+
+    case XENMEM_resource_grant_table_id_status:
+        if ( gt->gt_version != 2 )
+        {
+    default:
+            rc = -EINVAL;
+            break;
+        }
+        rc = gnttab_get_status_frame_mfn(d, tot_frames - 1, &tmp);
+        break;
+    }
+
+    /* Any errors from growing the table? */
+    if ( rc )
+        goto out;
+
+    switch ( id )
+    {
+    case XENMEM_resource_grant_table_id_shared:
+        vaddrs = gt->shared_raw;
+        break;
+
+    case XENMEM_resource_grant_table_id_status:
+        vaddrs = (void **)gt->status;
+        break;
+    }
+
+    for ( i = 0; i < nr_frames; ++i )
+        mfn_list[i] = virt_to_mfn(vaddrs[frame + i]);
+
+ out:
+    grant_write_unlock(gt);
+
+    return rc;
+}
+
 int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t *mfn)
 {
     int rc = 0;
@@ -4047,33 +4113,6 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t *mfn)
     return rc;
 }
 
-int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
-                            mfn_t *mfn)
-{
-    struct grant_table *gt = d->grant_table;
-    int rc;
-
-    grant_write_lock(gt);
-    rc = gnttab_get_shared_frame_mfn(d, idx, mfn);
-    grant_write_unlock(gt);
-
-    return rc;
-}
-
-int gnttab_get_status_frame(struct domain *d, unsigned long idx,
-                            mfn_t *mfn)
-{
-    struct grant_table *gt = d->grant_table;
-    int rc;
-
-    grant_write_lock(gt);
-    rc = (gt->gt_version == 2) ?
-        gnttab_get_status_frame_mfn(d, idx, mfn) : -EINVAL;
-    grant_write_unlock(gt);
-
-    return rc;
-}
-
 static void gnttab_usage_print(struct domain *rd)
 {
     int first = 1;
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 714077c1e5..dc3a7248e3 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1007,44 +1007,6 @@ static long xatp_permission_check(struct domain *d, unsigned int space)
     return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
 }
 
-static int acquire_grant_table(struct domain *d, unsigned int id,
-                               unsigned long frame,
-                               unsigned int nr_frames,
-                               xen_pfn_t mfn_list[])
-{
-    unsigned int i = nr_frames;
-
-    /* Iterate backwards in case table needs to grow */
-    while ( i-- != 0 )
-    {
-        mfn_t mfn = INVALID_MFN;
-        int rc;
-
-        switch ( id )
-        {
-        case XENMEM_resource_grant_table_id_shared:
-            rc = gnttab_get_shared_frame(d, frame + i, &mfn);
-            break;
-
-        case XENMEM_resource_grant_table_id_status:
-            rc = gnttab_get_status_frame(d, frame + i, &mfn);
-            break;
-
-        default:
-            rc = -EINVAL;
-            break;
-        }
-
-        if ( rc )
-            return rc;
-
-        ASSERT(!mfn_eq(mfn, INVALID_MFN));
-        mfn_list[i] = mfn_x(mfn);
-    }
-
-    return 0;
-}
-
 static int acquire_resource(
     XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
 {
@@ -1091,8 +1053,8 @@ static int acquire_resource(
     switch ( xmar.type )
     {
     case XENMEM_resource_grant_table:
-        rc = acquire_grant_table(d, xmar.id, xmar.frame, xmar.nr_frames,
-                                 mfn_list);
+        rc = gnttab_acquire_resource(d, xmar.id, xmar.frame, xmar.nr_frames,
+                                     mfn_list);
         break;
 
     default:
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 98603604b8..5a2c75b880 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -56,10 +56,10 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
 
 int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
                      mfn_t *mfn);
-int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
-                            mfn_t *mfn);
-int gnttab_get_status_frame(struct domain *d, unsigned long idx,
-                            mfn_t *mfn);
+
+int gnttab_acquire_resource(
+    struct domain *d, unsigned int id, unsigned long frame,
+    unsigned int nr_frames, xen_pfn_t mfn_list[]);
 
 #else
 
@@ -93,14 +93,9 @@ static inline int gnttab_map_frame(struct domain *d, unsigned long idx,
     return -EINVAL;
 }
 
-static inline int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
-                                          mfn_t *mfn)
-{
-    return -EINVAL;
-}
-
-static inline int gnttab_get_status_frame(struct domain *d, unsigned long idx,
-                                          mfn_t *mfn)
+static inline int gnttab_acquire_resource(
+    struct domain *d, unsigned int id, unsigned long frame,
+    unsigned int nr_frames, xen_pfn_t mfn_list[])
 {
     return -EINVAL;
 }
-- 
2.11.0

Re: [PATCH 2/5] xen/gnttab: Rework resource acquisition

[Andrew Cooper]

On 28/07/2020 12:37, Andrew Cooper wrote:
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 9f0cae52c0..122d1e7596 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -4013,6 +4013,72 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
>      return 0;
>  }
>  
> +int gnttab_acquire_resource(
> +    struct domain *d, unsigned int id, unsigned long frame,
> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
> +{
> +    struct grant_table *gt = d->grant_table;
> +    unsigned int i = nr_frames, tot_frames;
> +    void **vaddrs;

I've folded an "= NULL" here to fix the CentOS 6 build.

~Andrew

Re: [PATCH 2/5] xen/gnttab: Rework resource acquisition

[Jan Beulich]

On 28.07.2020 13:37, Andrew Cooper wrote:
> The existing logic doesn't function in the general case for mapping a guests
> grant table, due to arbitrary 32 frame limit, and the default grant table
> limit being 64.
> 
> In order to start addressing this, rework the existing grant table logic by
> implementing a single gnttab_acquire_resource().  This is far more efficient
> than the previous acquire_grant_table() in memory.c because it doesn't take
> the grant table write lock, and attempt to grow the table, for every single
> frame.

Among the code you replace there is a comment "Iterate backwards in case
table needs to grow" explaining why what you say about growing the grant
table didn't actually happen.

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -4013,6 +4013,72 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
>       return 0;
>   }
>   
> +int gnttab_acquire_resource(
> +    struct domain *d, unsigned int id, unsigned long frame,
> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
> +{
> +    struct grant_table *gt = d->grant_table;
> +    unsigned int i = nr_frames, tot_frames;
> +    void **vaddrs;
> +    int rc = 0;
> +
> +    /* Input sanity. */
> +    if ( !nr_frames )
> +        return -EINVAL;

I can't seem to be able to find an equivalent of this in the old logic,
and hence this looks like an unwarranted change in behavior to me. We
have quite a few hypercall ops where some count being zero is simply
a no-op, i.e. yielding success without doing anything.

> +    /* Overflow checks */
> +    if ( frame + nr_frames < frame )
> +        return -EINVAL;
> +
> +    tot_frames = frame + nr_frames;
> +    if ( tot_frames != frame + nr_frames )
> +        return -EINVAL;

I find the naming here quite confusing. I realize part of this stems
from the code you replace, but anyway: "unsigned long frame" typically
represents a memory frame number of some sort, making the calculation
look as if it was wrong. (Initially I merely meant to ask whether this
check isn't redundant with the prior one, or vice versa.)

> +    /* Grow table if necessary. */
> +    grant_write_lock(gt);
> +    switch ( id )
> +    {
> +        mfn_t tmp;
> +
> +    case XENMEM_resource_grant_table_id_shared:
> +        rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp);
> +        break;
> +
> +    case XENMEM_resource_grant_table_id_status:
> +        if ( gt->gt_version != 2 )
> +        {
> +    default:
> +            rc = -EINVAL;
> +            break;
> +        }
> +        rc = gnttab_get_status_frame_mfn(d, tot_frames - 1, &tmp);
> +        break;
> +    }
> +
> +    /* Any errors from growing the table? */
> +    if ( rc )
> +        goto out;
> +
> +    switch ( id )
> +    {
> +    case XENMEM_resource_grant_table_id_shared:
> +        vaddrs = gt->shared_raw;
> +        break;
> +
> +    case XENMEM_resource_grant_table_id_status:
> +        vaddrs = (void **)gt->status;

Now this is the kind of cast that I consider really dangerous, and hence
worth trying hard to avoid. With the code structure as is, I don't see
an immediate solution though.

> +        break;
> +    }

Worth having an ASSERT_UNREACHABLE() default case here?

Jan

Re: [PATCH 2/5] xen/gnttab: Rework resource acquisition

[Andrew Cooper]

On 29/07/2020 21:02, Jan Beulich wrote:
> On 28.07.2020 13:37, Andrew Cooper wrote:
>> The existing logic doesn't function in the general case for mapping a
>> guests
>> grant table, due to arbitrary 32 frame limit, and the default grant
>> table
>> limit being 64.
>>
>> In order to start addressing this, rework the existing grant table
>> logic by
>> implementing a single gnttab_acquire_resource().  This is far more
>> efficient
>> than the previous acquire_grant_table() in memory.c because it
>> doesn't take
>> the grant table write lock, and attempt to grow the table, for every
>> single
>> frame.
>
> Among the code you replace there is a comment "Iterate backwards in case
> table needs to grow" explaining why what you say about growing the grant
> table didn't actually happen.

What I have said is accurate.

Iterating backwards caused it to actually grow once, but every iteration
still takes the lock and attempts to grow the table.

>
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -4013,6 +4013,72 @@ static int gnttab_get_shared_frame_mfn(struct
>> domain *d,
>>       return 0;
>>   }
>>   +int gnttab_acquire_resource(
>> +    struct domain *d, unsigned int id, unsigned long frame,
>> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
>> +{
>> +    struct grant_table *gt = d->grant_table;
>> +    unsigned int i = nr_frames, tot_frames;
>> +    void **vaddrs;
>> +    int rc = 0;
>> +
>> +    /* Input sanity. */
>> +    if ( !nr_frames )
>> +        return -EINVAL;
>
> I can't seem to be able to find an equivalent of this in the old logic,
> and hence this looks like an unwarranted change in behavior to me. We
> have quite a few hypercall ops where some count being zero is simply
> a no-op, i.e. yielding success without doing anything.

There is no possible circumstance when getting here requesting 0 is
legitimate.

Tolerating a zero input for a mapping request is a very expensive way of
hiding caller bugs.

Most importantly however, the correctness of the logic does depends on
nr_frames being nonzero.

>
>> +    /* Overflow checks */
>> +    if ( frame + nr_frames < frame )
>> +        return -EINVAL;
>> +
>> +    tot_frames = frame + nr_frames;
>> +    if ( tot_frames != frame + nr_frames )
>> +        return -EINVAL;
>
> I find the naming here quite confusing. I realize part of this stems
> from the code you replace, but anyway: "unsigned long frame" typically
> represents a memory frame number of some sort, making the calculation
> look as if it was wrong. (Initially I merely meant to ask whether this
> check isn't redundant with the prior one, or vice versa.)

Both checks are strictly necessary.  The top one checks 64bit wrapping,
while the second is checking 32bit truncation.

This code is horrible, both in terms of the acquire_resource API, and
the grant API being off by one compared to normal, and needing the
max_frame index, not the number of frames.

~Andrew

Re: [PATCH 2/5] xen/gnttab: Rework resource acquisition

[Jan Beulich]

On 22.09.2020 15:10, Andrew Cooper wrote:
> On 29/07/2020 21:02, Jan Beulich wrote:
>> On 28.07.2020 13:37, Andrew Cooper wrote:
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -4013,6 +4013,72 @@ static int gnttab_get_shared_frame_mfn(struct
>>> domain *d,
>>>       return 0;
>>>   }
>>>   +int gnttab_acquire_resource(
>>> +    struct domain *d, unsigned int id, unsigned long frame,
>>> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
>>> +{
>>> +    struct grant_table *gt = d->grant_table;
>>> +    unsigned int i = nr_frames, tot_frames;
>>> +    void **vaddrs;
>>> +    int rc = 0;
>>> +
>>> +    /* Input sanity. */
>>> +    if ( !nr_frames )
>>> +        return -EINVAL;
>>
>> I can't seem to be able to find an equivalent of this in the old logic,
>> and hence this looks like an unwarranted change in behavior to me. We
>> have quite a few hypercall ops where some count being zero is simply
>> a no-op, i.e. yielding success without doing anything.
> 
> There is no possible circumstance when getting here requesting 0 is
> legitimate.
> 
> Tolerating a zero input for a mapping request is a very expensive way of
> hiding caller bugs.

That's just one possible view. There are people thinking that
some extra runtime overhead doesn't outweigh the "clutter" of
extra conditionals in their code, and hence would rather avoid
checking if some calculation of theirs yielded zero before
making a hypercall. I think we should try to consistently
regard counts of zero as "nothing to do" in all our hypercalls.

> Most importantly however, the correctness of the logic does depends on
> nr_frames being nonzero.

I didn't mean to ask to drop the conditional if it's needed;
all I'm opposed to is raising an error in this case.

Jan

Re: [PATCH 2/5] xen/gnttab: Rework resource acquisition

[Andrew Cooper]

On 22/09/2020 14:34, Jan Beulich wrote:
> On 22.09.2020 15:10, Andrew Cooper wrote:
>> On 29/07/2020 21:02, Jan Beulich wrote:
>>> On 28.07.2020 13:37, Andrew Cooper wrote:
>>>> --- a/xen/common/grant_table.c
>>>> +++ b/xen/common/grant_table.c
>>>> @@ -4013,6 +4013,72 @@ static int gnttab_get_shared_frame_mfn(struct
>>>> domain *d,
>>>>       return 0;
>>>>   }
>>>>   +int gnttab_acquire_resource(
>>>> +    struct domain *d, unsigned int id, unsigned long frame,
>>>> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
>>>> +{
>>>> +    struct grant_table *gt = d->grant_table;
>>>> +    unsigned int i = nr_frames, tot_frames;
>>>> +    void **vaddrs;
>>>> +    int rc = 0;
>>>> +
>>>> +    /* Input sanity. */
>>>> +    if ( !nr_frames )
>>>> +        return -EINVAL;
>>> I can't seem to be able to find an equivalent of this in the old logic,
>>> and hence this looks like an unwarranted change in behavior to me. We
>>> have quite a few hypercall ops where some count being zero is simply
>>> a no-op, i.e. yielding success without doing anything.
>> There is no possible circumstance when getting here requesting 0 is
>> legitimate.
>>
>> Tolerating a zero input for a mapping request is a very expensive way of
>> hiding caller bugs.
> That's just one possible view.

... that is fully enforced in the ecosystem we work in.

You can't get a 0 to here in the first place.

However, when it comes to the XLAT and the chunking fixes, getting 0
here is a hard error, and I'm not interested in breaking the that logic
for a non-existent corner case.

~Andrew

Re: [PATCH 2/5] xen/gnttab: Rework resource acquisition

[Jan Beulich]

On 22.09.2020 16:50, Andrew Cooper wrote:
> On 22/09/2020 14:34, Jan Beulich wrote:
>> On 22.09.2020 15:10, Andrew Cooper wrote:
>>> On 29/07/2020 21:02, Jan Beulich wrote:
>>>> On 28.07.2020 13:37, Andrew Cooper wrote:
>>>>> --- a/xen/common/grant_table.c
>>>>> +++ b/xen/common/grant_table.c
>>>>> @@ -4013,6 +4013,72 @@ static int gnttab_get_shared_frame_mfn(struct
>>>>> domain *d,
>>>>>       return 0;
>>>>>   }
>>>>>   +int gnttab_acquire_resource(
>>>>> +    struct domain *d, unsigned int id, unsigned long frame,
>>>>> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
>>>>> +{
>>>>> +    struct grant_table *gt = d->grant_table;
>>>>> +    unsigned int i = nr_frames, tot_frames;
>>>>> +    void **vaddrs;
>>>>> +    int rc = 0;
>>>>> +
>>>>> +    /* Input sanity. */
>>>>> +    if ( !nr_frames )
>>>>> +        return -EINVAL;
>>>> I can't seem to be able to find an equivalent of this in the old logic,
>>>> and hence this looks like an unwarranted change in behavior to me. We
>>>> have quite a few hypercall ops where some count being zero is simply
>>>> a no-op, i.e. yielding success without doing anything.
>>> There is no possible circumstance when getting here requesting 0 is
>>> legitimate.
>>>
>>> Tolerating a zero input for a mapping request is a very expensive way of
>>> hiding caller bugs.
>> That's just one possible view.
> 
> ... that is fully enforced in the ecosystem we work in.
> 
> You can't get a 0 to here in the first place.

Would you mind pointing me at why this is? All I can see is

    if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
        return -E2BIG;

as far as the caller's argument checking goes here. And
acquire_grant_table(), which is what you're replacing, also is dealing
with zero quite fine afaics. As is arch_acquire_resource().

> However, when it comes to the XLAT and the chunking fixes, getting 0
> here is a hard error, and I'm not interested in breaking the that logic
> for a non-existent corner case.

I don't share this view. It's also interesting that you had to address
similar questions by Paul, isn't it?

Jan

RE: [PATCH 2/5] xen/gnttab: Rework resource acquisition

[Paul Durrant]

> -----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>; George Dunlap <George.Dunlap@eu.citrix.com>; Ian
> Jackson <ian.jackson@citrix.com>; Jan Beulich <JBeulich@suse.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Julien
> Grall <julien@xen.org>; Paul Durrant <paul@xen.org>; Michał Leszczyński <michal.leszczynski@cert.pl>;
> Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
> Subject: [PATCH 2/5] xen/gnttab: Rework resource acquisition
> 
> The existing logic doesn't function in the general case for mapping a guests
> grant table, due to arbitrary 32 frame limit, and the default grant table
> limit being 64.
> 
> In order to start addressing this, rework the existing grant table logic by
> implementing a single gnttab_acquire_resource().  This is far more efficient
> than the previous acquire_grant_table() in memory.c because it doesn't take
> the grant table write lock, and attempt to grow the table, for every single
> frame.
> 

But that should not have happened before because the code deliberately iterates backwards, thereby starting with the last frame, thereby growing the table at most once. (I agree that dropping and re-acquiring the lock every time was sub-optimal).

> The new gnttab_acquire_resource() function subsumes the previous two
> gnttab_get_{shared,status}_frame() helpers.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Wei Liu <wl@xen.org>
> CC: Julien Grall <julien@xen.org>
> CC: Paul Durrant <paul@xen.org>
> CC: Michał Leszczyński <michal.leszczynski@cert.pl>
> CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
> ---
>  xen/common/grant_table.c      | 93 ++++++++++++++++++++++++++++++-------------
>  xen/common/memory.c           | 42 +------------------
>  xen/include/xen/grant_table.h | 19 ++++-----
>  3 files changed, 75 insertions(+), 79 deletions(-)
> 
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 9f0cae52c0..122d1e7596 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -4013,6 +4013,72 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
>      return 0;
>  }
> 
> +int gnttab_acquire_resource(
> +    struct domain *d, unsigned int id, unsigned long frame,
> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
> +{
> +    struct grant_table *gt = d->grant_table;
> +    unsigned int i = nr_frames, tot_frames;
> +    void **vaddrs;
> +    int rc = 0;
> +
> +    /* Input sanity. */
> +    if ( !nr_frames )
> +        return -EINVAL;

This was not an error before. Does mapping 0 frames really need to be a failure?

> +
> +    /* Overflow checks */
> +    if ( frame + nr_frames < frame )
> +        return -EINVAL;
> +
> +    tot_frames = frame + nr_frames;

That name is confusing. 'last_frame' perhaps (and then make the -1 implicit)?

> +    if ( tot_frames != frame + nr_frames )
> +        return -EINVAL;
> +
> +    /* Grow table if necessary. */
> +    grant_write_lock(gt);
> +    switch ( id )
> +    {
> +        mfn_t tmp;
> +
> +    case XENMEM_resource_grant_table_id_shared:
> +        rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp);
> +        break;
> +
> +    case XENMEM_resource_grant_table_id_status:
> +        if ( gt->gt_version != 2 )
> +        {
> +    default:
> +            rc = -EINVAL;
> +            break;
> +        }
> +        rc = gnttab_get_status_frame_mfn(d, tot_frames - 1, &tmp);
> +        break;
> +    }
> +
> +    /* Any errors from growing the table? */
> +    if ( rc )
> +        goto out;
> +
> +    switch ( id )
> +    {
> +    case XENMEM_resource_grant_table_id_shared:
> +        vaddrs = gt->shared_raw;
> +        break;
> +
> +    case XENMEM_resource_grant_table_id_status:
> +        vaddrs = (void **)gt->status;

Erk. Could we avoid this casting nastiness by putting a loop in each switch case. I know it could be considered code duplication but this is a bit icky.

> +        break;
> +    }
> +
> +    for ( i = 0; i < nr_frames; ++i )
> +        mfn_list[i] = virt_to_mfn(vaddrs[frame + i]);
> +
> + out:
> +    grant_write_unlock(gt);

Since you deliberately grew the table first, could you not drop the write lock and acquire it a read lock before looping over the frames?

  Paul

> +
> +    return rc;
> +}
> +
>  int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t *mfn)
>  {
>      int rc = 0;
> @@ -4047,33 +4113,6 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t
> *mfn)
>      return rc;
>  }
> 
> -int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
> -                            mfn_t *mfn)
> -{
> -    struct grant_table *gt = d->grant_table;
> -    int rc;
> -
> -    grant_write_lock(gt);
> -    rc = gnttab_get_shared_frame_mfn(d, idx, mfn);
> -    grant_write_unlock(gt);
> -
> -    return rc;
> -}
> -
> -int gnttab_get_status_frame(struct domain *d, unsigned long idx,
> -                            mfn_t *mfn)
> -{
> -    struct grant_table *gt = d->grant_table;
> -    int rc;
> -
> -    grant_write_lock(gt);
> -    rc = (gt->gt_version == 2) ?
> -        gnttab_get_status_frame_mfn(d, idx, mfn) : -EINVAL;
> -    grant_write_unlock(gt);
> -
> -    return rc;
> -}
> -
>  static void gnttab_usage_print(struct domain *rd)
>  {
>      int first = 1;
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 714077c1e5..dc3a7248e3 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1007,44 +1007,6 @@ static long xatp_permission_check(struct domain *d, unsigned int space)
>      return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
>  }
> 
> -static int acquire_grant_table(struct domain *d, unsigned int id,
> -                               unsigned long frame,
> -                               unsigned int nr_frames,
> -                               xen_pfn_t mfn_list[])
> -{
> -    unsigned int i = nr_frames;
> -
> -    /* Iterate backwards in case table needs to grow */
> -    while ( i-- != 0 )
> -    {
> -        mfn_t mfn = INVALID_MFN;
> -        int rc;
> -
> -        switch ( id )
> -        {
> -        case XENMEM_resource_grant_table_id_shared:
> -            rc = gnttab_get_shared_frame(d, frame + i, &mfn);
> -            break;
> -
> -        case XENMEM_resource_grant_table_id_status:
> -            rc = gnttab_get_status_frame(d, frame + i, &mfn);
> -            break;
> -
> -        default:
> -            rc = -EINVAL;
> -            break;
> -        }
> -
> -        if ( rc )
> -            return rc;
> -
> -        ASSERT(!mfn_eq(mfn, INVALID_MFN));
> -        mfn_list[i] = mfn_x(mfn);
> -    }
> -
> -    return 0;
> -}
> -
>  static int acquire_resource(
>      XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
>  {
> @@ -1091,8 +1053,8 @@ static int acquire_resource(
>      switch ( xmar.type )
>      {
>      case XENMEM_resource_grant_table:
> -        rc = acquire_grant_table(d, xmar.id, xmar.frame, xmar.nr_frames,
> -                                 mfn_list);
> +        rc = gnttab_acquire_resource(d, xmar.id, xmar.frame, xmar.nr_frames,
> +                                     mfn_list);
>          break;
> 
>      default:
> diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
> index 98603604b8..5a2c75b880 100644
> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -56,10 +56,10 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
> 
>  int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
>                       mfn_t *mfn);
> -int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
> -                            mfn_t *mfn);
> -int gnttab_get_status_frame(struct domain *d, unsigned long idx,
> -                            mfn_t *mfn);
> +
> +int gnttab_acquire_resource(
> +    struct domain *d, unsigned int id, unsigned long frame,
> +    unsigned int nr_frames, xen_pfn_t mfn_list[]);
> 
>  #else
> 
> @@ -93,14 +93,9 @@ static inline int gnttab_map_frame(struct domain *d, unsigned long idx,
>      return -EINVAL;
>  }
> 
> -static inline int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
> -                                          mfn_t *mfn)
> -{
> -    return -EINVAL;
> -}
> -
> -static inline int gnttab_get_status_frame(struct domain *d, unsigned long idx,
> -                                          mfn_t *mfn)
> +static inline int gnttab_acquire_resource(
> +    struct domain *d, unsigned int id, unsigned long frame,
> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
>  {
>      return -EINVAL;
>  }
> --
> 2.11.0

Re: [PATCH 2/5] xen/gnttab: Rework resource acquisition

[Andrew Cooper]

On 30/07/2020 09:14, 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>; George Dunlap <George.Dunlap@eu.citrix.com>; Ian
>> Jackson <ian.jackson@citrix.com>; Jan Beulich <JBeulich@suse.com>; Konrad Rzeszutek Wilk
>> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Julien
>> Grall <julien@xen.org>; Paul Durrant <paul@xen.org>; Michał Leszczyński <michal.leszczynski@cert.pl>;
>> Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
>> Subject: [PATCH 2/5] xen/gnttab: Rework resource acquisition
>>
>> The existing logic doesn't function in the general case for mapping a guests
>> grant table, due to arbitrary 32 frame limit, and the default grant table
>> limit being 64.
>>
>> In order to start addressing this, rework the existing grant table logic by
>> implementing a single gnttab_acquire_resource().  This is far more efficient
>> than the previous acquire_grant_table() in memory.c because it doesn't take
>> the grant table write lock, and attempt to grow the table, for every single
>> frame.
>>
> But that should not have happened before because the code deliberately iterates backwards, thereby starting with the last frame, thereby growing the table at most once. (I agree that dropping and re-acquiring the lock every time was sub-optimal).

It still attempts to grow on every iteration.  Its just growing to a
smaller size than already succeeded.

>
>> The new gnttab_acquire_resource() function subsumes the previous two
>> gnttab_get_{shared,status}_frame() helpers.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: George Dunlap <George.Dunlap@eu.citrix.com>
>> CC: Ian Jackson <ian.jackson@citrix.com>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Wei Liu <wl@xen.org>
>> CC: Julien Grall <julien@xen.org>
>> CC: Paul Durrant <paul@xen.org>
>> CC: Michał Leszczyński <michal.leszczynski@cert.pl>
>> CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
>> ---
>>  xen/common/grant_table.c      | 93 ++++++++++++++++++++++++++++++-------------
>>  xen/common/memory.c           | 42 +------------------
>>  xen/include/xen/grant_table.h | 19 ++++-----
>>  3 files changed, 75 insertions(+), 79 deletions(-)
>>
>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>> index 9f0cae52c0..122d1e7596 100644
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -4013,6 +4013,72 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
>>      return 0;
>>  }
>>
>> +int gnttab_acquire_resource(
>> +    struct domain *d, unsigned int id, unsigned long frame,
>> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
>> +{
>> +    struct grant_table *gt = d->grant_table;
>> +    unsigned int i = nr_frames, tot_frames;
>> +    void **vaddrs;
>> +    int rc = 0;
>> +
>> +    /* Input sanity. */
>> +    if ( !nr_frames )
>> +        return -EINVAL;
> This was not an error before. Does mapping 0 frames really need to be a failure?

Yes.

You have spotted the -1 which depends on nr_frames being nonzero to
function correctly.

>> +
>> +    /* Overflow checks */
>> +    if ( frame + nr_frames < frame )
>> +        return -EINVAL;
>> +
>> +    tot_frames = frame + nr_frames;
> That name is confusing. 'last_frame' perhaps (and then make the -1 implicit)?

How is that naming any less confusing?

>> +        break;
>> +    }
>> +
>> +    for ( i = 0; i < nr_frames; ++i )
>> +        mfn_list[i] = virt_to_mfn(vaddrs[frame + i]);
>> +
>> + out:
>> +    grant_write_unlock(gt);
> Since you deliberately grew the table first, could you not drop the write lock and acquire it a read lock before looping over the frames?

I tried originally.  That's not an operation supported by percpu
read/write locks, and this isn't a fastpath.

~Andrew

Re: [PATCH 2/5] xen/gnttab: Rework resource acquisition

[Julien Grall]

Hi Andrew,

On 28/07/2020 12:37, Andrew Cooper wrote:
> The existing logic doesn't function in the general case for mapping a guests
> grant table, due to arbitrary 32 frame limit, and the default grant table
> limit being 64.
> 
> In order to start addressing this, rework the existing grant table logic by
> implementing a single gnttab_acquire_resource().  This is far more efficient
> than the previous acquire_grant_table() in memory.c because it doesn't take
> the grant table write lock, and attempt to grow the table, for every single
> frame.
> 
> The new gnttab_acquire_resource() function subsumes the previous two
> gnttab_get_{shared,status}_frame() helpers.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Wei Liu <wl@xen.org>
> CC: Julien Grall <julien@xen.org>
> CC: Paul Durrant <paul@xen.org>
> CC: Michał Leszczyński <michal.leszczynski@cert.pl>
> CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
> ---
>   xen/common/grant_table.c      | 93 ++++++++++++++++++++++++++++++-------------
>   xen/common/memory.c           | 42 +------------------
>   xen/include/xen/grant_table.h | 19 ++++-----
>   3 files changed, 75 insertions(+), 79 deletions(-)
> 
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 9f0cae52c0..122d1e7596 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -4013,6 +4013,72 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
>       return 0;
>   }
>   
> +int gnttab_acquire_resource(
> +    struct domain *d, unsigned int id, unsigned long frame,
> +    unsigned int nr_frames, xen_pfn_t mfn_list[])

Any reasons for changing the way we usually indent parameters?

> +{
> +    struct grant_table *gt = d->grant_table;
> +    unsigned int i = nr_frames, tot_frames;
> +    void **vaddrs;
> +    int rc = 0;

AFAICT rc will always be initialized when used. So is it necessary?

> +
> +    /* Input sanity. */
> +    if ( !nr_frames )
> +        return -EINVAL;
> +
> +    /* Overflow checks */
> +    if ( frame + nr_frames < frame )
> +        return -EINVAL;
> +
> +    tot_frames = frame + nr_frames;
> +    if ( tot_frames != frame + nr_frames )
> +        return -EINVAL;
> +
> +    /* Grow table if necessary. */
> +    grant_write_lock(gt);
> +    switch ( id )
> +    {
> +        mfn_t tmp;
> +
> +    case XENMEM_resource_grant_table_id_shared:
> +        rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp);
> +        break;
> +
> +    case XENMEM_resource_grant_table_id_status:
> +        if ( gt->gt_version != 2 )
> +        {
> +    default:
> +            rc = -EINVAL;
> +            break;
I am aware this is valid C code, but I find this construct confusing. 
Could you just duplicate the 2 lines and have a separate default case?

> +        }
> +        rc = gnttab_get_status_frame_mfn(d, tot_frames - 1, &tmp);
> +        break;
> +    }
> +
> +    /* Any errors from growing the table? */
> +    if ( rc )
> +        goto out;
> +
> +    switch ( id )
> +    {
> +    case XENMEM_resource_grant_table_id_shared:
> +        vaddrs = gt->shared_raw;
> +        break;
> +
> +    case XENMEM_resource_grant_table_id_status:
> +        vaddrs = (void **)gt->status;
> +        break;
> +    }
> +

NIT: Would it make sense to check vaddrs? This would avoid NULL 
dereference pointers if something went wrong.

> +    for ( i = 0; i < nr_frames; ++i )
> +        mfn_list[i] = virt_to_mfn(vaddrs[frame + i]);
> +
> + out:
> +    grant_write_unlock(gt);
> +
> +    return rc;
> +}
> +

Cheers,

-- 
Julien Grall

[PATCH 3/5] xen/memory: Fix compat XENMEM_acquire_resource for size requests

[Andrew Cooper]

Copy the nr_frames from the correct structure, so the caller doesn't
unconditionally receive 0.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>
CC: Paul Durrant <paul@xen.org>
CC: Michał Leszczyński <michal.leszczynski@cert.pl>
CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
---
 xen/common/compat/memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/compat/memory.c b/xen/common/compat/memory.c
index 3851f756c7..ed92e05b08 100644
--- a/xen/common/compat/memory.c
+++ b/xen/common/compat/memory.c
@@ -599,7 +599,7 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
                 if ( __copy_field_to_guest(
                          guest_handle_cast(compat,
                                            compat_mem_acquire_resource_t),
-                         &cmp.mar, nr_frames) )
+                         nat.mar, nr_frames) )
                     return -EFAULT;
             }
             else
-- 
2.11.0

Re: [PATCH 3/5] xen/memory: Fix compat XENMEM_acquire_resource for size requests

[Jan Beulich]

On 28.07.2020 13:37, Andrew Cooper wrote:
> Copy the nr_frames from the correct structure, so the caller doesn't
> unconditionally receive 0.

Well, no - it does get copied from the correct structure. It's just
that the field doesn't get set properly up front. Otherwise you'll
(a) build in an unchecked assumption that the native and compat
fields match in type and (b) set a bad example for people looking
here and then cloning this code in perhaps a case where (a) is not
even true. If you agree, the alternative change of setting
cmp.mar.nr_frames from nat.mar->nr_frames before the call is

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH 3/5] xen/memory: Fix compat XENMEM_acquire_resource for size requests

[Andrew Cooper]

On 29/07/2020 21:09, Jan Beulich wrote:
> On 28.07.2020 13:37, Andrew Cooper wrote:
>> Copy the nr_frames from the correct structure, so the caller doesn't
>> unconditionally receive 0.
>
> Well, no - it does get copied from the correct structure. It's just
> that the field doesn't get set properly up front.

You appear to be objecting to my use of the term "correct".

There are two structures.  One contains the correct value, and one
contains the wrong value, which happens to always be 0.

I stand by sentence as currently written.

> Otherwise you'll
> (a) build in an unchecked assumption that the native and compat
> fields match in type

Did you actually check?  Because I did before embarking on this course
of action.

In file included from /local/xen.git/xen/include/xen/guest_access.h:10:0,
                 from compat/memory.c:5:
/local/xen.git/xen/include/asm/guest_access.h:152:28: error: comparison
of distinct pointer types lacks a cast [-Werror]
     (void)(&(hnd).p->field == _s);                      \
                            ^
compat/memory.c:628:22: note: in expansion of macro ‘__copy_field_to_guest’
                 if ( __copy_field_to_guest(
                      ^~~~~~~~~~~~~~~~~~~~~

This is what the compiler thinks of the code, when nr_frames is changed
from uint32_t to unsigned long.


> and (b) set a bad example for people looking
> here

This entire function is a massive set of bad examples; the worst IMO
being the fact that there isn't a single useful comment anywhere in it
concerning how the higher level loop structure works.

I'm constantly annoyed that I need to reverse engineer it from scratch
every time I look at it, despite having a better-than-most understanding
of what it is trying to achieve, and how it is supposed to work.

I realise this is noones fault in particular, but it is not
fair/reasonable to claim that this change is the thing setting a bad
example in this file.

> and then cloning this code in perhaps a case where (a) is not
> even true. If you agree, the alternative change of setting
> cmp.mar.nr_frames from nat.mar->nr_frames before the call is

Is there more to this sentence?

While this example could be implemented (at even higher overhead) by
copying nat back to cmp before passing it back to the guest, the same is
not true for the changes required to fix batching (which is another
series the same size as this).

~Andrew

Re: [PATCH 3/5] xen/memory: Fix compat XENMEM_acquire_resource for size requests

[Jan Beulich]

On 30.07.2020 21:12, Andrew Cooper wrote:
> On 29/07/2020 21:09, Jan Beulich wrote:
>> On 28.07.2020 13:37, Andrew Cooper wrote:
>>> Copy the nr_frames from the correct structure, so the caller doesn't
>>> unconditionally receive 0.
>>
>> Well, no - it does get copied from the correct structure. It's just
>> that the field doesn't get set properly up front.
> 
> You appear to be objecting to my use of the term "correct".
> 
> There are two structures.  One contains the correct value, and one
> contains the wrong value, which happens to always be 0.
> 
> I stand by sentence as currently written.

At the risk of splitting hair, what you copy from is a field holding
the correct value, but not the correct field. This only works
correctly because of the way __copy_field_{from,to}_guest() happen
to be implemented; there are possible alternative implementations
where this would break, despite ...

>> Otherwise you'll
>> (a) build in an unchecked assumption that the native and compat
>> fields match in type
> 
> Did you actually check?  Because I did before embarking on this course
> of action.
> 
> In file included from /local/xen.git/xen/include/xen/guest_access.h:10:0,
>                  from compat/memory.c:5:
> /local/xen.git/xen/include/asm/guest_access.h:152:28: error: comparison
> of distinct pointer types lacks a cast [-Werror]
>      (void)(&(hnd).p->field == _s);                      \
>                             ^
> compat/memory.c:628:22: note: in expansion of macro ‘__copy_field_to_guest’
>                  if ( __copy_field_to_guest(
>                       ^~~~~~~~~~~~~~~~~~~~~
> 
> This is what the compiler thinks of the code, when nr_frames is changed
> from uint32_t to unsigned long.

... this type safety check (which, I admit, I didn't consider when
writing my reply). I continue to think that handle and struct should
match up not just for {,__}copy_{from,to}_guest() but also for
{,__}copy_field_{from,to}_guest().

>> and (b) set a bad example for people looking
>> here
> 
> This entire function is a massive set of bad examples; the worst IMO
> being the fact that there isn't a single useful comment anywhere in it
> concerning how the higher level loop structure works.
> 
> I'm constantly annoyed that I need to reverse engineer it from scratch
> every time I look at it, despite having a better-than-most understanding
> of what it is trying to achieve, and how it is supposed to work.
> 
> I realise this is noones fault in particular, but it is not
> fair/reasonable to claim that this change is the thing setting a bad
> example in this file.

I'd be happy to see "bad examples" be corrected. As stated at various
occasions, at the time I first implemented the compat layer this seemed
like the most reasonable approach to me. If you see room for
improvement, then I'm all for it.

>> and then cloning this code in perhaps a case where (a) is not
>> even true. If you agree, the alternative change of setting
>> cmp.mar.nr_frames from nat.mar->nr_frames before the call is
> 
> Is there more to this sentence?

I guess I can't figure what you mean here.

> While this example could be implemented (at even higher overhead) by
> copying nat back to cmp before passing it back to the guest, the same is
> not true for the changes required to fix batching (which is another
> series the same size as this).

I'll see when you post this, but I think we will want the principle
outlined above to continue to hold.

Jan

RE: [PATCH 3/5] xen/memory: Fix compat XENMEM_acquire_resource for size requests

[Paul Durrant]

> -----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>; George Dunlap <George.Dunlap@eu.citrix.com>; Ian
> Jackson <ian.jackson@citrix.com>; Jan Beulich <JBeulich@suse.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Julien
> Grall <julien@xen.org>; Paul Durrant <paul@xen.org>; Michał Leszczyński <michal.leszczynski@cert.pl>;
> Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
> Subject: [PATCH 3/5] xen/memory: Fix compat XENMEM_acquire_resource for size requests
> 
> Copy the nr_frames from the correct structure, so the caller doesn't
> unconditionally receive 0.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Paul Durrant <paul@xen.org>

> ---
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Wei Liu <wl@xen.org>
> CC: Julien Grall <julien@xen.org>
> CC: Paul Durrant <paul@xen.org>
> CC: Michał Leszczyński <michal.leszczynski@cert.pl>
> CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
> ---
>  xen/common/compat/memory.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/common/compat/memory.c b/xen/common/compat/memory.c
> index 3851f756c7..ed92e05b08 100644
> --- a/xen/common/compat/memory.c
> +++ b/xen/common/compat/memory.c
> @@ -599,7 +599,7 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
>                  if ( __copy_field_to_guest(
>                           guest_handle_cast(compat,
>                                             compat_mem_acquire_resource_t),
> -                         &cmp.mar, nr_frames) )
> +                         nat.mar, nr_frames) )
>                      return -EFAULT;
>              }
>              else
> --
> 2.11.0

[PATCH 4/5] xen/memory: Fix acquire_resource size semantics

[Andrew Cooper]

Calling XENMEM_acquire_resource with a NULL frame_list is a request for the
size of the resource, but the returned 32 is bogus.

If someone tries to follow it for XENMEM_resource_ioreq_server, the acquire
call will fail as IOREQ servers currently top out at 2 frames, and it is only
half the size of the default grant table limit for guests.

Also, no users actually request a resource size, because it was never wired up
in the sole implemenation of resource aquisition in Linux.

Introduce a new resource_max_frames() to calculate the size of a resource, and
implement it the IOREQ and grant subsystems.

It is impossible to guarentee that a mapping call following a successful size
call will succedd (e.g. The target IOREQ server gets destroyed, or the domain
switches from grant v2 to v1).  Document the restriction, and use the
flexibility to simplify the paths to be lockless.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>
CC: Paul Durrant <paul@xen.org>
CC: Michał Leszczyński <michal.leszczynski@cert.pl>
CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
---
 xen/arch/x86/mm.c             | 20 ++++++++++++++++
 xen/common/grant_table.c      | 19 +++++++++++++++
 xen/common/memory.c           | 55 +++++++++++++++++++++++++++++++++----------
 xen/include/asm-x86/mm.h      |  3 +++
 xen/include/public/memory.h   | 16 +++++++++----
 xen/include/xen/grant_table.h |  8 +++++++
 xen/include/xen/mm.h          |  6 +++++
 7 files changed, 110 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 82bc676553..f73a90a2ab 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4600,6 +4600,26 @@ int xenmem_add_to_physmap_one(
     return rc;
 }
 
+unsigned int arch_resource_max_frames(
+    struct domain *d, unsigned int type, unsigned int id)
+{
+    unsigned int nr = 0;
+
+    switch ( type )
+    {
+#ifdef CONFIG_HVM
+    case XENMEM_resource_ioreq_server:
+        if ( !is_hvm_domain(d) )
+            break;
+        /* One frame for the buf-ioreq ring, and one frame per 128 vcpus. */
+        nr = 1 + DIV_ROUND_UP(d->max_vcpus * sizeof(struct ioreq), PAGE_SIZE);
+        break;
+#endif
+    }
+
+    return nr;
+}
+
 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[])
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 122d1e7596..0962fc7169 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -4013,6 +4013,25 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
     return 0;
 }
 
+unsigned int gnttab_resource_max_frames(struct domain *d, unsigned int id)
+{
+    unsigned int nr = 0;
+
+    /* Don't need the grant lock.  This limit is fixed at domain create time. */
+    switch ( id )
+    {
+    case XENMEM_resource_grant_table_id_shared:
+        nr = d->grant_table->max_grant_frames;
+        break;
+
+    case XENMEM_resource_grant_table_id_status:
+        nr = grant_to_status_frames(d->grant_table->max_grant_frames);
+        break;
+    }
+
+    return nr;
+}
+
 int gnttab_acquire_resource(
     struct domain *d, unsigned int id, unsigned long frame,
     unsigned int nr_frames, xen_pfn_t mfn_list[])
diff --git a/xen/common/memory.c b/xen/common/memory.c
index dc3a7248e3..21edabf9cc 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1007,6 +1007,26 @@ static long xatp_permission_check(struct domain *d, unsigned int space)
     return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
 }
 
+/*
+ * Return 0 on any kind of error.  Caller converts to -EINVAL.
+ *
+ * All nonzero values should be repeatable (i.e. derived from some fixed
+ * proerty of the domain), and describe the full resource (i.e. mapping the
+ * result of this call will be the entire resource).
+ */
+static unsigned int resource_max_frames(struct domain *d,
+                                        unsigned int type, unsigned int id)
+{
+    switch ( type )
+    {
+    case XENMEM_resource_grant_table:
+        return gnttab_resource_max_frames(d, id);
+
+    default:
+        return arch_resource_max_frames(d, type, id);
+    }
+}
+
 static int acquire_resource(
     XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
 {
@@ -1018,6 +1038,7 @@ static int acquire_resource(
      * use-cases then per-CPU arrays or heap allocations may be required.
      */
     xen_pfn_t mfn_list[32];
+    unsigned int max_frames;
     int rc;
 
     if ( copy_from_guest(&xmar, arg, 1) )
@@ -1026,19 +1047,6 @@ static int acquire_resource(
     if ( xmar.pad != 0 )
         return -EINVAL;
 
-    if ( guest_handle_is_null(xmar.frame_list) )
-    {
-        if ( xmar.nr_frames )
-            return -EINVAL;
-
-        xmar.nr_frames = ARRAY_SIZE(mfn_list);
-
-        if ( __copy_field_to_guest(arg, &xmar, nr_frames) )
-            return -EFAULT;
-
-        return 0;
-    }
-
     if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
         return -E2BIG;
 
@@ -1050,6 +1058,27 @@ static int acquire_resource(
     if ( rc )
         goto out;
 
+    max_frames = resource_max_frames(d, xmar.type, xmar.id);
+
+    rc = -EINVAL;
+    if ( !max_frames )
+        goto out;
+
+    if ( guest_handle_is_null(xmar.frame_list) )
+    {
+        if ( xmar.nr_frames )
+            goto out;
+
+        xmar.nr_frames = max_frames;
+
+        rc = -EFAULT;
+        if ( __copy_field_to_guest(arg, &xmar, nr_frames) )
+            goto out;
+
+        rc = 0;
+        goto out;
+    }
+
     switch ( xmar.type )
     {
     case XENMEM_resource_grant_table:
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 7e74996053..b0caf372a8 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -649,6 +649,9 @@ static inline bool arch_mfn_in_directmap(unsigned long mfn)
     return mfn <= (virt_to_mfn(eva - 1) + 1);
 }
 
+unsigned int arch_resource_max_frames(struct domain *d,
+                                      unsigned int type, unsigned int id);
+
 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[]);
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 21057ed78e..cea88cf40c 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -639,10 +639,18 @@ struct xen_mem_acquire_resource {
 #define XENMEM_resource_grant_table_id_status 1
 
     /*
-     * IN/OUT - As an IN parameter number of frames of the resource
-     *          to be mapped. However, if the specified value is 0 and
-     *          frame_list is NULL then this field will be set to the
-     *          maximum value supported by the implementation on return.
+     * IN/OUT
+     *
+     * As an IN parameter number of frames of the resource to be mapped.
+     *
+     * When frame_list is NULL and nr_frames is 0, this is interpreted as a
+     * request for the size of the resource, which shall be returned in the
+     * nr_frames field.
+     *
+     * The size of a resource will never be zero, but a nonzero result doesn't
+     * guarentee that a subsequent mapping request will be successful.  There
+     * are further type/id specific constraints which may change between the
+     * two calls.
      */
     uint32_t nr_frames;
     uint32_t pad;
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 5a2c75b880..bae4d79623 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -57,6 +57,8 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
 int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
                      mfn_t *mfn);
 
+unsigned int gnttab_resource_max_frames(struct domain *d, unsigned int id);
+
 int gnttab_acquire_resource(
     struct domain *d, unsigned int id, unsigned long frame,
     unsigned int nr_frames, xen_pfn_t mfn_list[]);
@@ -93,6 +95,12 @@ static inline int gnttab_map_frame(struct domain *d, unsigned long idx,
     return -EINVAL;
 }
 
+static inline unsigned int gnttab_resource_max_frames(
+    struct domain *d, unsigned int id)
+{
+    return 0;
+}
+
 static inline int gnttab_acquire_resource(
     struct domain *d, unsigned int id, unsigned long frame,
     unsigned int nr_frames, xen_pfn_t mfn_list[])
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 1b2c1f6b32..c184dc1db1 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -686,6 +686,12 @@ static inline void put_page_alloc_ref(struct page_info *page)
 }
 
 #ifndef CONFIG_ARCH_ACQUIRE_RESOURCE
+static inline unsigned int arch_resource_max_frames(
+    struct domain *d, unsigned int type, unsigned int id)
+{
+    return 0;
+}
+
 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[])
-- 
2.11.0

RE: [PATCH 4/5] xen/memory: Fix acquire_resource size semantics

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Andrew Cooper
> Sent: 28 July 2020 12:37
> To: Xen-devel <xen-devel@lists.xenproject.org>
> Cc: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>; Stefano Stabellini <sstabellini@kernel.org>; Julien
> Grall <julien@xen.org>; Wei Liu <wl@xen.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; George
> Dunlap <George.Dunlap@eu.citrix.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Paul Durrant
> <paul@xen.org>; Jan Beulich <JBeulich@suse.com>; Michał Leszczyński <michal.leszczynski@cert.pl>; Ian
> Jackson <ian.jackson@citrix.com>
> Subject: [PATCH 4/5] xen/memory: Fix acquire_resource size semantics
> 
> Calling XENMEM_acquire_resource with a NULL frame_list is a request for the
> size of the resource, but the returned 32 is bogus.
> 
> If someone tries to follow it for XENMEM_resource_ioreq_server, the acquire
> call will fail as IOREQ servers currently top out at 2 frames, and it is only
> half the size of the default grant table limit for guests.
> 
> Also, no users actually request a resource size, because it was never wired up
> in the sole implemenation of resource aquisition in Linux.
> 
> Introduce a new resource_max_frames() to calculate the size of a resource, and
> implement it the IOREQ and grant subsystems.
> 
> It is impossible to guarentee that a mapping call following a successful size

s/guarantee/guarantee

> call will succedd (e.g. The target IOREQ server gets destroyed, or the domain

s/succedd/succeed

> switches from grant v2 to v1).  Document the restriction, and use the
> flexibility to simplify the paths to be lockless.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Wei Liu <wl@xen.org>
> CC: Julien Grall <julien@xen.org>
> CC: Paul Durrant <paul@xen.org>
> CC: Michał Leszczyński <michal.leszczynski@cert.pl>
> CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
> ---
>  xen/arch/x86/mm.c             | 20 ++++++++++++++++
>  xen/common/grant_table.c      | 19 +++++++++++++++
>  xen/common/memory.c           | 55 +++++++++++++++++++++++++++++++++----------
>  xen/include/asm-x86/mm.h      |  3 +++
>  xen/include/public/memory.h   | 16 +++++++++----
>  xen/include/xen/grant_table.h |  8 +++++++
>  xen/include/xen/mm.h          |  6 +++++
>  7 files changed, 110 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 82bc676553..f73a90a2ab 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4600,6 +4600,26 @@ int xenmem_add_to_physmap_one(
>      return rc;
>  }
> 
> +unsigned int arch_resource_max_frames(
> +    struct domain *d, unsigned int type, unsigned int id)
> +{
> +    unsigned int nr = 0;
> +
> +    switch ( type )
> +    {
> +#ifdef CONFIG_HVM
> +    case XENMEM_resource_ioreq_server:
> +        if ( !is_hvm_domain(d) )
> +            break;
> +        /* One frame for the buf-ioreq ring, and one frame per 128 vcpus. */
> +        nr = 1 + DIV_ROUND_UP(d->max_vcpus * sizeof(struct ioreq), PAGE_SIZE);
> +        break;
> +#endif
> +    }
> +
> +    return nr;
> +}
> +
>  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[])
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 122d1e7596..0962fc7169 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -4013,6 +4013,25 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
>      return 0;
>  }
> 
> +unsigned int gnttab_resource_max_frames(struct domain *d, unsigned int id)
> +{
> +    unsigned int nr = 0;
> +
> +    /* Don't need the grant lock.  This limit is fixed at domain create time. */
> +    switch ( id )
> +    {
> +    case XENMEM_resource_grant_table_id_shared:
> +        nr = d->grant_table->max_grant_frames;
> +        break;
> +
> +    case XENMEM_resource_grant_table_id_status:
> +        nr = grant_to_status_frames(d->grant_table->max_grant_frames);

Two uses of d->grant_table, so perhaps define a stack variable for it? Also, should you not make sure 0 is returned in the case of a v1 table?

> +        break;
> +    }
> +
> +    return nr;
> +}
> +
>  int gnttab_acquire_resource(
>      struct domain *d, unsigned int id, unsigned long frame,
>      unsigned int nr_frames, xen_pfn_t mfn_list[])
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index dc3a7248e3..21edabf9cc 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1007,6 +1007,26 @@ static long xatp_permission_check(struct domain *d, unsigned int space)
>      return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
>  }
> 
> +/*
> + * Return 0 on any kind of error.  Caller converts to -EINVAL.
> + *
> + * All nonzero values should be repeatable (i.e. derived from some fixed
> + * proerty of the domain), and describe the full resource (i.e. mapping the

s/property/property

> + * result of this call will be the entire resource).

This precludes dynamically adding a resource to a running domain. Do we really want to bake in that restriction?

> + */
> +static unsigned int resource_max_frames(struct domain *d,
> +                                        unsigned int type, unsigned int id)
> +{
> +    switch ( type )
> +    {
> +    case XENMEM_resource_grant_table:
> +        return gnttab_resource_max_frames(d, id);
> +
> +    default:
> +        return arch_resource_max_frames(d, type, id);
> +    }
> +}
> +
>  static int acquire_resource(
>      XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
>  {
> @@ -1018,6 +1038,7 @@ static int acquire_resource(
>       * use-cases then per-CPU arrays or heap allocations may be required.
>       */
>      xen_pfn_t mfn_list[32];
> +    unsigned int max_frames;
>      int rc;
> 
>      if ( copy_from_guest(&xmar, arg, 1) )
> @@ -1026,19 +1047,6 @@ static int acquire_resource(
>      if ( xmar.pad != 0 )
>          return -EINVAL;
> 
> -    if ( guest_handle_is_null(xmar.frame_list) )
> -    {
> -        if ( xmar.nr_frames )
> -            return -EINVAL;
> -
> -        xmar.nr_frames = ARRAY_SIZE(mfn_list);
> -
> -        if ( __copy_field_to_guest(arg, &xmar, nr_frames) )
> -            return -EFAULT;
> -
> -        return 0;
> -    }
> -
>      if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
>          return -E2BIG;
> 
> @@ -1050,6 +1058,27 @@ static int acquire_resource(
>      if ( rc )
>          goto out;
> 
> +    max_frames = resource_max_frames(d, xmar.type, xmar.id);
> +
> +    rc = -EINVAL;
> +    if ( !max_frames )
> +        goto out;
> +
> +    if ( guest_handle_is_null(xmar.frame_list) )
> +    {
> +        if ( xmar.nr_frames )
> +            goto out;
> +
> +        xmar.nr_frames = max_frames;
> +
> +        rc = -EFAULT;
> +        if ( __copy_field_to_guest(arg, &xmar, nr_frames) )
> +            goto out;
> +
> +        rc = 0;
> +        goto out;
> +    }
> +
>      switch ( xmar.type )
>      {
>      case XENMEM_resource_grant_table:
> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> index 7e74996053..b0caf372a8 100644
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -649,6 +649,9 @@ static inline bool arch_mfn_in_directmap(unsigned long mfn)
>      return mfn <= (virt_to_mfn(eva - 1) + 1);
>  }
> 
> +unsigned int arch_resource_max_frames(struct domain *d,
> +                                      unsigned int type, unsigned int id);
> +
>  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[]);
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index 21057ed78e..cea88cf40c 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -639,10 +639,18 @@ struct xen_mem_acquire_resource {
>  #define XENMEM_resource_grant_table_id_status 1
> 
>      /*
> -     * IN/OUT - As an IN parameter number of frames of the resource
> -     *          to be mapped. However, if the specified value is 0 and
> -     *          frame_list is NULL then this field will be set to the
> -     *          maximum value supported by the implementation on return.
> +     * IN/OUT
> +     *
> +     * As an IN parameter number of frames of the resource to be mapped.
> +     *
> +     * When frame_list is NULL and nr_frames is 0, this is interpreted as a
> +     * request for the size of the resource, which shall be returned in the
> +     * nr_frames field.
> +     *
> +     * The size of a resource will never be zero, but a nonzero result doesn't
> +     * guarentee that a subsequent mapping request will be successful.  There

s/guarantee/guarantee

  Paul

> +     * are further type/id specific constraints which may change between the
> +     * two calls.
>       */
>      uint32_t nr_frames;
>      uint32_t pad;
> diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
> index 5a2c75b880..bae4d79623 100644
> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -57,6 +57,8 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
>  int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
>                       mfn_t *mfn);
> 
> +unsigned int gnttab_resource_max_frames(struct domain *d, unsigned int id);
> +
>  int gnttab_acquire_resource(
>      struct domain *d, unsigned int id, unsigned long frame,
>      unsigned int nr_frames, xen_pfn_t mfn_list[]);
> @@ -93,6 +95,12 @@ static inline int gnttab_map_frame(struct domain *d, unsigned long idx,
>      return -EINVAL;
>  }
> 
> +static inline unsigned int gnttab_resource_max_frames(
> +    struct domain *d, unsigned int id)
> +{
> +    return 0;
> +}
> +
>  static inline int gnttab_acquire_resource(
>      struct domain *d, unsigned int id, unsigned long frame,
>      unsigned int nr_frames, xen_pfn_t mfn_list[])
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 1b2c1f6b32..c184dc1db1 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -686,6 +686,12 @@ static inline void put_page_alloc_ref(struct page_info *page)
>  }
> 
>  #ifndef CONFIG_ARCH_ACQUIRE_RESOURCE
> +static inline unsigned int arch_resource_max_frames(
> +    struct domain *d, unsigned int type, unsigned int id)
> +{
> +    return 0;
> +}
> +
>  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[])
> --
> 2.11.0
> 

Re: [PATCH 4/5] xen/memory: Fix acquire_resource size semantics

[Julien Grall]

Hi Paul,

On 30/07/2020 09:31, Paul Durrant wrote:
>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>> index dc3a7248e3..21edabf9cc 100644
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -1007,6 +1007,26 @@ static long xatp_permission_check(struct domain *d, unsigned int space)
>>       return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
>>   }
>>
>> +/*
>> + * Return 0 on any kind of error.  Caller converts to -EINVAL.
>> + *
>> + * All nonzero values should be repeatable (i.e. derived from some fixed
>> + * proerty of the domain), and describe the full resource (i.e. mapping the
> 
> s/property/property
> 
>> + * result of this call will be the entire resource).
> 
> This precludes dynamically adding a resource to a running domain. Do we really want to bake in that restriction?

AFAICT, this restriction is not documented in the ABI. In particular, it 
is written:

"
The size of a resource will never be zero, but a nonzero result doesn't
guarentee that a subsequent mapping request will be successful.  There
are further type/id specific constraints which may change between the
two calls.
"

So I think a domain couldn't rely on this behavior. Although, it might 
be good to clarify in the comment on top of resource_max_frames that 
this an implementation decision and not part of the ABI.

Cheers,

-- 
Julien Grall

Re: [PATCH 4/5] xen/memory: Fix acquire_resource size semantics

[Andrew Cooper]

On 30/07/2020 13:54, Julien Grall wrote:
> Hi Paul,
>
> On 30/07/2020 09:31, Paul Durrant wrote:
>>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>>> index dc3a7248e3..21edabf9cc 100644
>>> --- a/xen/common/memory.c
>>> +++ b/xen/common/memory.c
>>> @@ -1007,6 +1007,26 @@ static long xatp_permission_check(struct
>>> domain *d, unsigned int space)
>>>       return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
>>>   }
>>>
>>> +/*
>>> + * Return 0 on any kind of error.  Caller converts to -EINVAL.
>>> + *
>>> + * All nonzero values should be repeatable (i.e. derived from some
>>> fixed
>>> + * proerty of the domain), and describe the full resource (i.e.
>>> mapping the
>>
>> s/property/property
>>
>>> + * result of this call will be the entire resource).
>>
>> This precludes dynamically adding a resource to a running domain. Do
>> we really want to bake in that restriction?
>
> AFAICT, this restriction is not documented in the ABI. In particular,
> it is written:
>
> "
> The size of a resource will never be zero, but a nonzero result doesn't
> guarentee that a subsequent mapping request will be successful.  There
> are further type/id specific constraints which may change between the
> two calls.
> "
>
> So I think a domain couldn't rely on this behavior. Although, it might
> be good to clarify in the comment on top of resource_max_frames that
> this an implementation decision and not part of the ABI.

There are two aspects here.

First, yes - I deliberately didn't state it in the ABI, just in case we
might want to use it in the future.  I could theoretically foresee using
-EBUSY for the purpose.

That said however, we are currently deliberately taking dynamic
resources out of Xen, because they've proved to be unnecessary in
practice and a fertile source of complexity and security bugs.

I don't foresee accepting new dynamic resources, but that's not to say
that someone can't theoretically come up with a sufficiently compelling
counterexample.

~Andrew

Re: [PATCH 4/5] xen/memory: Fix acquire_resource size semantics

[Andrew Cooper]

On 30/07/2020 09:31, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Andrew Cooper
>> Sent: 28 July 2020 12:37
>> To: Xen-devel <xen-devel@lists.xenproject.org>
>> Cc: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>; Stefano Stabellini <sstabellini@kernel.org>; Julien
>> Grall <julien@xen.org>; Wei Liu <wl@xen.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; George
>> Dunlap <George.Dunlap@eu.citrix.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Paul Durrant
>> <paul@xen.org>; Jan Beulich <JBeulich@suse.com>; Michał Leszczyński <michal.leszczynski@cert.pl>; Ian
>> Jackson <ian.jackson@citrix.com>
>> Subject: [PATCH 4/5] xen/memory: Fix acquire_resource size semantics
>>
>> Calling XENMEM_acquire_resource with a NULL frame_list is a request for the
>> size of the resource, but the returned 32 is bogus.
>>
>> If someone tries to follow it for XENMEM_resource_ioreq_server, the acquire
>> call will fail as IOREQ servers currently top out at 2 frames, and it is only
>> half the size of the default grant table limit for guests.
>>
>> Also, no users actually request a resource size, because it was never wired up
>> in the sole implemenation of resource aquisition in Linux.
>>
>> Introduce a new resource_max_frames() to calculate the size of a resource, and
>> implement it the IOREQ and grant subsystems.
>>
>> It is impossible to guarentee that a mapping call following a successful size
> s/guarantee/guarantee
>
>> call will succedd (e.g. The target IOREQ server gets destroyed, or the domain
> s/succedd/succeed
>
>> switches from grant v2 to v1).  Document the restriction, and use the
>> flexibility to simplify the paths to be lockless.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: George Dunlap <George.Dunlap@eu.citrix.com>
>> CC: Ian Jackson <ian.jackson@citrix.com>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Wei Liu <wl@xen.org>
>> CC: Julien Grall <julien@xen.org>
>> CC: Paul Durrant <paul@xen.org>
>> CC: Michał Leszczyński <michal.leszczynski@cert.pl>
>> CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
>> ---
>>  xen/arch/x86/mm.c             | 20 ++++++++++++++++
>>  xen/common/grant_table.c      | 19 +++++++++++++++
>>  xen/common/memory.c           | 55 +++++++++++++++++++++++++++++++++----------
>>  xen/include/asm-x86/mm.h      |  3 +++
>>  xen/include/public/memory.h   | 16 +++++++++----
>>  xen/include/xen/grant_table.h |  8 +++++++
>>  xen/include/xen/mm.h          |  6 +++++
>>  7 files changed, 110 insertions(+), 17 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>> index 82bc676553..f73a90a2ab 100644
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -4600,6 +4600,26 @@ int xenmem_add_to_physmap_one(
>>      return rc;
>>  }
>>
>> +unsigned int arch_resource_max_frames(
>> +    struct domain *d, unsigned int type, unsigned int id)
>> +{
>> +    unsigned int nr = 0;
>> +
>> +    switch ( type )
>> +    {
>> +#ifdef CONFIG_HVM
>> +    case XENMEM_resource_ioreq_server:
>> +        if ( !is_hvm_domain(d) )
>> +            break;
>> +        /* One frame for the buf-ioreq ring, and one frame per 128 vcpus. */
>> +        nr = 1 + DIV_ROUND_UP(d->max_vcpus * sizeof(struct ioreq), PAGE_SIZE);
>> +        break;
>> +#endif
>> +    }
>> +
>> +    return nr;
>> +}
>> +
>>  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[])
>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>> index 122d1e7596..0962fc7169 100644
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -4013,6 +4013,25 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
>>      return 0;
>>  }
>>
>> +unsigned int gnttab_resource_max_frames(struct domain *d, unsigned int id)
>> +{
>> +    unsigned int nr = 0;
>> +
>> +    /* Don't need the grant lock.  This limit is fixed at domain create time. */
>> +    switch ( id )
>> +    {
>> +    case XENMEM_resource_grant_table_id_shared:
>> +        nr = d->grant_table->max_grant_frames;
>> +        break;
>> +
>> +    case XENMEM_resource_grant_table_id_status:
>> +        nr = grant_to_status_frames(d->grant_table->max_grant_frames);
> Two uses of d->grant_table, so perhaps define a stack variable for it?

Can do.

>  Also, should you not make sure 0 is returned in the case of a v1 table?

This was the case specifically discussed in the commit message, but
perhaps it needs expanding.

Doing so would be buggy.

Some utility is going to query the resource size, and then try to map it
(if it doesn't blindly know the size and/or subset it cares about already).

In between these two hypercalls from the utility, the guest can do a
v1=>v2 or v2=>v1 switch and make the resource spontaneously appear or
disappear.

The only case where we can know for certain whether the resource is
available is when we're in the map hypercall.  Therefore, userspace has
to be able to get to the map call if there is potentially a resource
available.

The semantics of the size call are really "this resource might exist,
and if it does, this is how large it is".


As for the grant status frames specifically, I think making them a
mappable resource might have been a poor choice in hind sight.

Only the guest can switch between grant versions.  GNTTABOP_set_version
strictly operates on current, unlike most of the other grant hypercalls
which take a domid and let dom0 specify something other than DOMID_SELF.

There is GNTTABOP_get_version, but it is racy to use in the same way as
described above, and if some utility does successfully map the status
frames, what will happen in practice is that a guest attempting to
switch from v2 back to v1 will have the set_version hypercall fail due
to outstanding refs on the frames.

~Andrew

Re: [PATCH 4/5] xen/memory: Fix acquire_resource size semantics

[Julien Grall]

Hi Andrew,

On 30/07/2020 20:46, Andrew Cooper wrote:
> On 30/07/2020 09:31, Paul Durrant wrote:
> In between these two hypercalls from the utility, the guest can do a
> v1=>v2 or v2=>v1 switch and make the resource spontaneously appear or
> disappear.

This can only happen on platform where grant-table v2 is enabled. Where 
this is not enabled (e.g Arm), then I think we want to return 0 as there 
is nothing to map.

Cheers,

-- 
Julien Grall

Re: [PATCH 4/5] xen/memory: Fix acquire_resource size semantics

[Jan Beulich]

On 30.07.2020 21:46, Andrew Cooper wrote:
> On 30/07/2020 09:31, Paul Durrant wrote:
>>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Andrew Cooper
>>> Sent: 28 July 2020 12:37
>>>
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -4013,6 +4013,25 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
>>>      return 0;
>>>  }
>>>
>>> +unsigned int gnttab_resource_max_frames(struct domain *d, unsigned int id)
>>> +{
>>> +    unsigned int nr = 0;
>>> +
>>> +    /* Don't need the grant lock.  This limit is fixed at domain create time. */
>>> +    switch ( id )
>>> +    {
>>> +    case XENMEM_resource_grant_table_id_shared:
>>> +        nr = d->grant_table->max_grant_frames;
>>> +        break;
>>> +
>>> +    case XENMEM_resource_grant_table_id_status:
>>> +        nr = grant_to_status_frames(d->grant_table->max_grant_frames);
>> Two uses of d->grant_table, so perhaps define a stack variable for it?
> 
> Can do.
> 
>>  Also, should you not make sure 0 is returned in the case of a v1 table?
> 
> This was the case specifically discussed in the commit message, but
> perhaps it needs expanding.
> 
> Doing so would be buggy.
> 
> Some utility is going to query the resource size, and then try to map it
> (if it doesn't blindly know the size and/or subset it cares about already).
> 
> In between these two hypercalls from the utility, the guest can do a
> v1=>v2 or v2=>v1 switch and make the resource spontaneously appear or
> disappear.
> 
> The only case where we can know for certain whether the resource is
> available is when we're in the map hypercall.  Therefore, userspace has
> to be able to get to the map call if there is potentially a resource
> available.
> 
> The semantics of the size call are really "this resource might exist,
> and if it does, this is how large it is".

With you deriving from d->grant_table->max_grant_frames, this approach
would imply that by obtaining a mapping the grant tables will get
grown to their permitted maximum, no matter whether as much is actually
needed by the guest. If this is indeed the intention, then we could as
well set up maximum grant structures right at domain creation. Not
something I would favor, but anyway...

Jan

Re: [PATCH 4/5] xen/memory: Fix acquire_resource size semantics

[Jan Beulich]

On 28.07.2020 13:37, Andrew Cooper wrote:
> @@ -1026,19 +1047,6 @@ static int acquire_resource(
>      if ( xmar.pad != 0 )
>          return -EINVAL;
>  
> -    if ( guest_handle_is_null(xmar.frame_list) )
> -    {
> -        if ( xmar.nr_frames )
> -            return -EINVAL;
> -
> -        xmar.nr_frames = ARRAY_SIZE(mfn_list);
> -
> -        if ( __copy_field_to_guest(arg, &xmar, nr_frames) )
> -            return -EFAULT;
> -
> -        return 0;
> -    }
> -
>      if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
>          return -E2BIG;

While arguably minor, the error code in the null-handle case
would imo better be the same, no matter how big xmar.nr_frames
is.

Jan

Re: [PATCH 4/5] xen/memory: Fix acquire_resource size semantics

[Andrew Cooper]

On 31/07/2020 15:44, Jan Beulich wrote:
> On 28.07.2020 13:37, Andrew Cooper wrote:
>> @@ -1026,19 +1047,6 @@ static int acquire_resource(
>>      if ( xmar.pad != 0 )
>>          return -EINVAL;
>>  
>> -    if ( guest_handle_is_null(xmar.frame_list) )
>> -    {
>> -        if ( xmar.nr_frames )
>> -            return -EINVAL;
>> -
>> -        xmar.nr_frames = ARRAY_SIZE(mfn_list);
>> -
>> -        if ( __copy_field_to_guest(arg, &xmar, nr_frames) )
>> -            return -EFAULT;
>> -
>> -        return 0;
>> -    }
>> -
>>      if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
>>          return -E2BIG;
> While arguably minor, the error code in the null-handle case
> would imo better be the same, no matter how big xmar.nr_frames
> is.

This clause doesn't survive the fixes to batching.

Given how broken this infrastructure is, I'm not concerned with
transient differences in error codes for users which will ultimately
fail anyway.

~Andrew

[PATCH 5/5] tools/foreignmem: Support querying the size of a resource

[Andrew Cooper]

With the Xen side of this interface fixed to return real sizes, userspace
needs to be able to make the query.

Introduce xenforeignmemory_resource_size() for the purpose, bumping the
library minor version and providing compatiblity for the non-Linux builds.

Its not possible to reuse the IOCTL_PRIVCMD_MMAP_RESOURCE infrastructure,
because it depends on having already mmap()'d a suitably sized region before
it will make an XENMEM_acquire_resource hypercall to Xen.

Instead, open a xencall handle and make an XENMEM_acquire_resource hypercall
directly.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Paul Durrant <paul@xen.org>
CC: Michał Leszczyński <michal.leszczynski@cert.pl>
CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
---
 tools/libs/foreignmemory/Makefile                  |  2 +-
 tools/libs/foreignmemory/core.c                    | 14 +++++++++
 .../libs/foreignmemory/include/xenforeignmemory.h  | 15 ++++++++++
 tools/libs/foreignmemory/libxenforeignmemory.map   |  4 +++
 tools/libs/foreignmemory/linux.c                   | 35 ++++++++++++++++++++++
 tools/libs/foreignmemory/private.h                 | 14 +++++++++
 6 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/tools/libs/foreignmemory/Makefile b/tools/libs/foreignmemory/Makefile
index 28f1bddc96..8e07f92c59 100644
--- a/tools/libs/foreignmemory/Makefile
+++ b/tools/libs/foreignmemory/Makefile
@@ -2,7 +2,7 @@ XEN_ROOT = $(CURDIR)/../../..
 include $(XEN_ROOT)/tools/Rules.mk
 
 MAJOR    = 1
-MINOR    = 3
+MINOR    = 4
 LIBNAME  := foreignmemory
 USELIBS  := toollog toolcore
 
diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
index 63f12e2450..5d95c59c48 100644
--- a/tools/libs/foreignmemory/core.c
+++ b/tools/libs/foreignmemory/core.c
@@ -53,6 +53,10 @@ xenforeignmemory_handle *xenforeignmemory_open(xentoollog_logger *logger,
         if (!fmem->logger) goto err;
     }
 
+    fmem->xcall = xencall_open(fmem->logger, 0);
+    if ( !fmem->xcall )
+        goto err;
+
     rc = osdep_xenforeignmemory_open(fmem);
     if ( rc  < 0 ) goto err;
 
@@ -61,6 +65,7 @@ xenforeignmemory_handle *xenforeignmemory_open(xentoollog_logger *logger,
 err:
     xentoolcore__deregister_active_handle(&fmem->tc_ah);
     osdep_xenforeignmemory_close(fmem);
+    xencall_close(fmem->xcall);
     xtl_logger_destroy(fmem->logger_tofree);
     free(fmem);
     return NULL;
@@ -75,6 +80,7 @@ int xenforeignmemory_close(xenforeignmemory_handle *fmem)
 
     xentoolcore__deregister_active_handle(&fmem->tc_ah);
     rc = osdep_xenforeignmemory_close(fmem);
+    xencall_close(fmem->xcall);
     xtl_logger_destroy(fmem->logger_tofree);
     free(fmem);
     return rc;
@@ -188,6 +194,14 @@ int xenforeignmemory_unmap_resource(
     return rc;
 }
 
+int xenforeignmemory_resource_size(
+    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
+    unsigned int id, unsigned long *nr_frames)
+{
+    return osdep_xenforeignmemory_resource_size(fmem, domid, type,
+                                                id, nr_frames);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libs/foreignmemory/include/xenforeignmemory.h b/tools/libs/foreignmemory/include/xenforeignmemory.h
index d594be8df0..1ba2f5316b 100644
--- a/tools/libs/foreignmemory/include/xenforeignmemory.h
+++ b/tools/libs/foreignmemory/include/xenforeignmemory.h
@@ -179,6 +179,21 @@ xenforeignmemory_resource_handle *xenforeignmemory_map_resource(
 int xenforeignmemory_unmap_resource(
     xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres);
 
+/**
+ * Determine the maximum size of a specific resource.
+ *
+ * @parm fmem handle to the open foreignmemory interface
+ * @parm domid the domain id
+ * @parm type the resource type
+ * @parm id the type-specific resource identifier
+ *
+ * Return 0 on success and fills in *nr_frames.  Sets errno and return -1 on
+ * error.
+ */
+int xenforeignmemory_resource_size(
+    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
+    unsigned int id, unsigned long *nr_frames);
+
 #endif
 
 /*
diff --git a/tools/libs/foreignmemory/libxenforeignmemory.map b/tools/libs/foreignmemory/libxenforeignmemory.map
index d5323c87d9..8aca341b99 100644
--- a/tools/libs/foreignmemory/libxenforeignmemory.map
+++ b/tools/libs/foreignmemory/libxenforeignmemory.map
@@ -19,3 +19,7 @@ VERS_1.3 {
 		xenforeignmemory_map_resource;
 		xenforeignmemory_unmap_resource;
 } VERS_1.2;
+VERS_1.4 {
+	global:
+		xenforeignmemory_resource_size;
+} VERS_1.3;
diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
index 8daa5828e3..67e0ca1e83 100644
--- a/tools/libs/foreignmemory/linux.c
+++ b/tools/libs/foreignmemory/linux.c
@@ -28,6 +28,8 @@
 
 #include "private.h"
 
+#include <xen/memory.h>
+
 #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & ~((1UL<<(_w))-1))
 
 #ifndef O_CLOEXEC
@@ -340,6 +342,39 @@ int osdep_xenforeignmemory_map_resource(
     return 0;
 }
 
+int osdep_xenforeignmemory_resource_size(
+    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
+    unsigned int id, unsigned long *nr_frames)
+{
+    int rc;
+    struct xen_mem_acquire_resource *xmar =
+        xencall_alloc_buffer(fmem->xcall, sizeof(*xmar));
+
+    if ( !xmar )
+    {
+        PERROR("Could not bounce memory for acquire_resource hypercall");
+        return -1;
+    }
+
+    *xmar = (struct xen_mem_acquire_resource){
+        .domid = domid,
+        .type = type,
+        .id = id,
+    };
+
+    rc = xencall2(fmem->xcall, __HYPERVISOR_memory_op,
+                  XENMEM_acquire_resource, (uintptr_t)xmar);
+    if ( rc )
+        goto out;
+
+    *nr_frames = xmar->nr_frames;
+
+ out:
+    xencall_free_buffer(fmem->xcall, xmar);
+
+    return rc;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libs/foreignmemory/private.h b/tools/libs/foreignmemory/private.h
index 8f1bf081ed..1a6b685f45 100644
--- a/tools/libs/foreignmemory/private.h
+++ b/tools/libs/foreignmemory/private.h
@@ -4,6 +4,7 @@
 #include <xentoollog.h>
 
 #include <xenforeignmemory.h>
+#include <xencall.h>
 
 #include <xentoolcore_internal.h>
 
@@ -20,6 +21,7 @@
 
 struct xenforeignmemory_handle {
     xentoollog_logger *logger, *logger_tofree;
+    xencall_handle *xcall;
     unsigned flags;
     int fd;
     Xentoolcore__Active_Handle tc_ah;
@@ -74,6 +76,15 @@ static inline int osdep_xenforeignmemory_unmap_resource(
 {
     return 0;
 }
+
+static inline int osdep_xenforeignmemory_resource_size(
+    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
+    unsigned int id, unsigned long *nr_frames)
+{
+    errno = EOPNOTSUPP;
+    return -1;
+}
+
 #else
 int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
                                     domid_t domid);
@@ -81,6 +92,9 @@ int osdep_xenforeignmemory_map_resource(
     xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres);
 int osdep_xenforeignmemory_unmap_resource(
     xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres);
+int osdep_xenforeignmemory_resource_size(
+    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
+    unsigned int id, unsigned long *nr_frames);
 #endif
 
 #define PERROR(_f...) \
-- 
2.11.0

Re: [PATCH 5/5] tools/foreignmem: Support querying the size of a resource

[Andrew Cooper]

On 28/07/2020 12:37, Andrew Cooper wrote:
> With the Xen side of this interface fixed to return real sizes, userspace
> needs to be able to make the query.
>
> Introduce xenforeignmemory_resource_size() for the purpose, bumping the
> library minor version and providing compatiblity for the non-Linux builds.
>
> Its not possible to reuse the IOCTL_PRIVCMD_MMAP_RESOURCE infrastructure,
> because it depends on having already mmap()'d a suitably sized region before
> it will make an XENMEM_acquire_resource hypercall to Xen.
>
> Instead, open a xencall handle and make an XENMEM_acquire_resource hypercall
> directly.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Ian Jackson <Ian.Jackson@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Paul Durrant <paul@xen.org>
> CC: Michał Leszczyński <michal.leszczynski@cert.pl>
> CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>

I've folded:

diff --git a/tools/Rules.mk b/tools/Rules.mk
index 5ed5664bf7..b8ccf03ea9 100644
--- a/tools/Rules.mk
+++ b/tools/Rules.mk
@@ -123,7 +123,7 @@ LDLIBS_libxencall = $(SHDEPS_libxencall)
$(XEN_LIBXENCALL)/libxencall$(libextens
 SHLIB_libxencall  = $(SHDEPS_libxencall) -Wl,-rpath-link=$(XEN_LIBXENCALL)
 
 CFLAGS_libxenforeignmemory = -I$(XEN_LIBXENFOREIGNMEMORY)/include
$(CFLAGS_xeninclude)
-SHDEPS_libxenforeignmemory = $(SHLIB_libxentoolcore)
+SHDEPS_libxenforeignmemory = $(SHLIB_libxentoolcore) $(SHDEPS_libxencall)
 LDLIBS_libxenforeignmemory = $(SHDEPS_libxenforeignmemory)
$(XEN_LIBXENFOREIGNMEMORY)/libxenforeignmemory$(libextension)
 SHLIB_libxenforeignmemory  = $(SHDEPS_libxenforeignmemory)
-Wl,-rpath-link=$(XEN_LIBXENFOREIGNMEMORY)
 
diff --git a/tools/libs/foreignmemory/Makefile
b/tools/libs/foreignmemory/Makefile
index 8e07f92c59..f3a61e27c7 100644
--- a/tools/libs/foreignmemory/Makefile
+++ b/tools/libs/foreignmemory/Makefile
@@ -4,7 +4,7 @@ include $(XEN_ROOT)/tools/Rules.mk
 MAJOR    = 1
 MINOR    = 4
 LIBNAME  := foreignmemory
-USELIBS  := toollog toolcore
+USELIBS  := toollog toolcore call
 
 SRCS-y                 += core.c
 SRCS-$(CONFIG_Linux)   += linux.c

to fix the build in certain containers.

~Andrew

Re: [PATCH 5/5] tools/foreignmem: Support querying the size of a resource

[Wei Liu]

On Tue, Jul 28, 2020 at 03:14:39PM +0100, Andrew Cooper wrote:
> On 28/07/2020 12:37, Andrew Cooper wrote:
> > With the Xen side of this interface fixed to return real sizes, userspace
> > needs to be able to make the query.
> >
> > Introduce xenforeignmemory_resource_size() for the purpose, bumping the
> > library minor version and providing compatiblity for the non-Linux builds.
> >
> > Its not possible to reuse the IOCTL_PRIVCMD_MMAP_RESOURCE infrastructure,
> > because it depends on having already mmap()'d a suitably sized region before
> > it will make an XENMEM_acquire_resource hypercall to Xen.
> >
> > Instead, open a xencall handle and make an XENMEM_acquire_resource hypercall
> > directly.
> >
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> > CC: Ian Jackson <Ian.Jackson@citrix.com>
> > CC: Wei Liu <wl@xen.org>
> > CC: Paul Durrant <paul@xen.org>
> > CC: Michał Leszczyński <michal.leszczynski@cert.pl>
> > CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
> 
> I've folded:
> 
> diff --git a/tools/Rules.mk b/tools/Rules.mk
> index 5ed5664bf7..b8ccf03ea9 100644
> --- a/tools/Rules.mk
> +++ b/tools/Rules.mk
> @@ -123,7 +123,7 @@ LDLIBS_libxencall = $(SHDEPS_libxencall)
> $(XEN_LIBXENCALL)/libxencall$(libextens
>  SHLIB_libxencall  = $(SHDEPS_libxencall) -Wl,-rpath-link=$(XEN_LIBXENCALL)
>  
>  CFLAGS_libxenforeignmemory = -I$(XEN_LIBXENFOREIGNMEMORY)/include
> $(CFLAGS_xeninclude)
> -SHDEPS_libxenforeignmemory = $(SHLIB_libxentoolcore)
> +SHDEPS_libxenforeignmemory = $(SHLIB_libxentoolcore) $(SHDEPS_libxencall)
>  LDLIBS_libxenforeignmemory = $(SHDEPS_libxenforeignmemory)
> $(XEN_LIBXENFOREIGNMEMORY)/libxenforeignmemory$(libextension)
>  SHLIB_libxenforeignmemory  = $(SHDEPS_libxenforeignmemory)
> -Wl,-rpath-link=$(XEN_LIBXENFOREIGNMEMORY)
>  
> diff --git a/tools/libs/foreignmemory/Makefile
> b/tools/libs/foreignmemory/Makefile
> index 8e07f92c59..f3a61e27c7 100644
> --- a/tools/libs/foreignmemory/Makefile
> +++ b/tools/libs/foreignmemory/Makefile
> @@ -4,7 +4,7 @@ include $(XEN_ROOT)/tools/Rules.mk
>  MAJOR    = 1
>  MINOR    = 4
>  LIBNAME  := foreignmemory
> -USELIBS  := toollog toolcore
> +USELIBS  := toollog toolcore call
>  
>  SRCS-y                 += core.c
>  SRCS-$(CONFIG_Linux)   += linux.c
> 
> to fix the build in certain containers.

Acked-by: Wei Liu <wl@xen.org>

RE: [PATCH 5/5] tools/foreignmem: Support querying the size of a resource

[Paul Durrant]

> -----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>; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu
> <wl@xen.org>; Paul Durrant <paul@xen.org>; Michał Leszczyński <michal.leszczynski@cert.pl>; Hubert
> Jasudowicz <hubert.jasudowicz@cert.pl>
> Subject: [PATCH 5/5] tools/foreignmem: Support querying the size of a resource
> 
> With the Xen side of this interface fixed to return real sizes, userspace
> needs to be able to make the query.
> 
> Introduce xenforeignmemory_resource_size() for the purpose, bumping the
> library minor version and providing compatiblity for the non-Linux builds.
> 
> Its not possible to reuse the IOCTL_PRIVCMD_MMAP_RESOURCE infrastructure,
> because it depends on having already mmap()'d a suitably sized region before
> it will make an XENMEM_acquire_resource hypercall to Xen.
> 
> Instead, open a xencall handle and make an XENMEM_acquire_resource hypercall
> directly.

Shame we have to do that but, as you say, it's the only option.

> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Paul Durrant <paul@xen.org>

> ---
> CC: Ian Jackson <Ian.Jackson@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Paul Durrant <paul@xen.org>
> CC: Michał Leszczyński <michal.leszczynski@cert.pl>
> CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
> ---
>  tools/libs/foreignmemory/Makefile                  |  2 +-
>  tools/libs/foreignmemory/core.c                    | 14 +++++++++
>  .../libs/foreignmemory/include/xenforeignmemory.h  | 15 ++++++++++
>  tools/libs/foreignmemory/libxenforeignmemory.map   |  4 +++
>  tools/libs/foreignmemory/linux.c                   | 35 ++++++++++++++++++++++
>  tools/libs/foreignmemory/private.h                 | 14 +++++++++
>  6 files changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libs/foreignmemory/Makefile b/tools/libs/foreignmemory/Makefile
> index 28f1bddc96..8e07f92c59 100644
> --- a/tools/libs/foreignmemory/Makefile
> +++ b/tools/libs/foreignmemory/Makefile
> @@ -2,7 +2,7 @@ XEN_ROOT = $(CURDIR)/../../..
>  include $(XEN_ROOT)/tools/Rules.mk
> 
>  MAJOR    = 1
> -MINOR    = 3
> +MINOR    = 4
>  LIBNAME  := foreignmemory
>  USELIBS  := toollog toolcore
> 
> diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
> index 63f12e2450..5d95c59c48 100644
> --- a/tools/libs/foreignmemory/core.c
> +++ b/tools/libs/foreignmemory/core.c
> @@ -53,6 +53,10 @@ xenforeignmemory_handle *xenforeignmemory_open(xentoollog_logger *logger,
>          if (!fmem->logger) goto err;
>      }
> 
> +    fmem->xcall = xencall_open(fmem->logger, 0);
> +    if ( !fmem->xcall )
> +        goto err;
> +
>      rc = osdep_xenforeignmemory_open(fmem);
>      if ( rc  < 0 ) goto err;
> 
> @@ -61,6 +65,7 @@ xenforeignmemory_handle *xenforeignmemory_open(xentoollog_logger *logger,
>  err:
>      xentoolcore__deregister_active_handle(&fmem->tc_ah);
>      osdep_xenforeignmemory_close(fmem);
> +    xencall_close(fmem->xcall);
>      xtl_logger_destroy(fmem->logger_tofree);
>      free(fmem);
>      return NULL;
> @@ -75,6 +80,7 @@ int xenforeignmemory_close(xenforeignmemory_handle *fmem)
> 
>      xentoolcore__deregister_active_handle(&fmem->tc_ah);
>      rc = osdep_xenforeignmemory_close(fmem);
> +    xencall_close(fmem->xcall);
>      xtl_logger_destroy(fmem->logger_tofree);
>      free(fmem);
>      return rc;
> @@ -188,6 +194,14 @@ int xenforeignmemory_unmap_resource(
>      return rc;
>  }
> 
> +int xenforeignmemory_resource_size(
> +    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
> +    unsigned int id, unsigned long *nr_frames)
> +{
> +    return osdep_xenforeignmemory_resource_size(fmem, domid, type,
> +                                                id, nr_frames);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libs/foreignmemory/include/xenforeignmemory.h
> b/tools/libs/foreignmemory/include/xenforeignmemory.h
> index d594be8df0..1ba2f5316b 100644
> --- a/tools/libs/foreignmemory/include/xenforeignmemory.h
> +++ b/tools/libs/foreignmemory/include/xenforeignmemory.h
> @@ -179,6 +179,21 @@ xenforeignmemory_resource_handle *xenforeignmemory_map_resource(
>  int xenforeignmemory_unmap_resource(
>      xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres);
> 
> +/**
> + * Determine the maximum size of a specific resource.
> + *
> + * @parm fmem handle to the open foreignmemory interface
> + * @parm domid the domain id
> + * @parm type the resource type
> + * @parm id the type-specific resource identifier
> + *
> + * Return 0 on success and fills in *nr_frames.  Sets errno and return -1 on
> + * error.
> + */
> +int xenforeignmemory_resource_size(
> +    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
> +    unsigned int id, unsigned long *nr_frames);
> +
>  #endif
> 
>  /*
> diff --git a/tools/libs/foreignmemory/libxenforeignmemory.map
> b/tools/libs/foreignmemory/libxenforeignmemory.map
> index d5323c87d9..8aca341b99 100644
> --- a/tools/libs/foreignmemory/libxenforeignmemory.map
> +++ b/tools/libs/foreignmemory/libxenforeignmemory.map
> @@ -19,3 +19,7 @@ VERS_1.3 {
>  		xenforeignmemory_map_resource;
>  		xenforeignmemory_unmap_resource;
>  } VERS_1.2;
> +VERS_1.4 {
> +	global:
> +		xenforeignmemory_resource_size;
> +} VERS_1.3;
> diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
> index 8daa5828e3..67e0ca1e83 100644
> --- a/tools/libs/foreignmemory/linux.c
> +++ b/tools/libs/foreignmemory/linux.c
> @@ -28,6 +28,8 @@
> 
>  #include "private.h"
> 
> +#include <xen/memory.h>
> +
>  #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & ~((1UL<<(_w))-1))
> 
>  #ifndef O_CLOEXEC
> @@ -340,6 +342,39 @@ int osdep_xenforeignmemory_map_resource(
>      return 0;
>  }
> 
> +int osdep_xenforeignmemory_resource_size(
> +    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
> +    unsigned int id, unsigned long *nr_frames)
> +{
> +    int rc;
> +    struct xen_mem_acquire_resource *xmar =
> +        xencall_alloc_buffer(fmem->xcall, sizeof(*xmar));
> +
> +    if ( !xmar )
> +    {
> +        PERROR("Could not bounce memory for acquire_resource hypercall");
> +        return -1;
> +    }
> +
> +    *xmar = (struct xen_mem_acquire_resource){
> +        .domid = domid,
> +        .type = type,
> +        .id = id,
> +    };
> +
> +    rc = xencall2(fmem->xcall, __HYPERVISOR_memory_op,
> +                  XENMEM_acquire_resource, (uintptr_t)xmar);
> +    if ( rc )
> +        goto out;
> +
> +    *nr_frames = xmar->nr_frames;
> +
> + out:
> +    xencall_free_buffer(fmem->xcall, xmar);
> +
> +    return rc;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libs/foreignmemory/private.h b/tools/libs/foreignmemory/private.h
> index 8f1bf081ed..1a6b685f45 100644
> --- a/tools/libs/foreignmemory/private.h
> +++ b/tools/libs/foreignmemory/private.h
> @@ -4,6 +4,7 @@
>  #include <xentoollog.h>
> 
>  #include <xenforeignmemory.h>
> +#include <xencall.h>
> 
>  #include <xentoolcore_internal.h>
> 
> @@ -20,6 +21,7 @@
> 
>  struct xenforeignmemory_handle {
>      xentoollog_logger *logger, *logger_tofree;
> +    xencall_handle *xcall;
>      unsigned flags;
>      int fd;
>      Xentoolcore__Active_Handle tc_ah;
> @@ -74,6 +76,15 @@ static inline int osdep_xenforeignmemory_unmap_resource(
>  {
>      return 0;
>  }
> +
> +static inline int osdep_xenforeignmemory_resource_size(
> +    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
> +    unsigned int id, unsigned long *nr_frames)
> +{
> +    errno = EOPNOTSUPP;
> +    return -1;
> +}
> +
>  #else
>  int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
>                                      domid_t domid);
> @@ -81,6 +92,9 @@ int osdep_xenforeignmemory_map_resource(
>      xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres);
>  int osdep_xenforeignmemory_unmap_resource(
>      xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres);
> +int osdep_xenforeignmemory_resource_size(
> +    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
> +    unsigned int id, unsigned long *nr_frames);
>  #endif
> 
>  #define PERROR(_f...) \
> --
> 2.11.0