The grant table operation GNTTABOP_transfer is meant to be used in
PV device backends, and it hasn't been used in Linux since the old
Xen-o-Linux days.

Add a command line sub-option to the "gnttab" option for disabling the
GNTTABOP_transfer functionality.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 docs/misc/xen-command-line.pandoc | 7 +++++--
 xen/common/grant_table.c          | 6 ++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 6b3da6ddc1..97ddcfa523 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1167,9 +1167,9 @@ does not provide `VM_ENTRY_LOAD_GUEST_PAT`.
 Specify which console gdbstub should use. See **console**.
 
 ### gnttab
-> `= List of [ max-ver:<integer>, transitive=<bool> ]`
+> `= List of [ max-ver:<integer>, transitive=<bool>, transfer=<bool> ]`
 
-> Default: `gnttab=max-ver:2,transitive`
+> Default: `gnttab=max-ver:2,transitive,transfer`
 
 Control various aspects of the grant table behaviour available to guests.
 
@@ -1178,6 +1178,9 @@ version are 1 and 2.
 * `transitive` Permit or disallow the use of transitive grants.  Note that the
 use of grant table v2 without transitive grants is an ABI breakage from the
 guests point of view.
+* `transfer` Permit or disallow the GNTTABOP_transfer operation of the
+grant table hypercall.  Note that disallowing GNTTABOP_transfer is an ABI
+breakage from the guests point of view.
 
 The usage of gnttab v2 is not security supported on ARM platforms.
 
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index ed1e2fabce..d1c225e927 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -181,6 +181,7 @@ static int parse_gnttab_max_maptrack_frames(const char *arg)
 
 unsigned int __read_mostly opt_gnttab_max_version = GNTTAB_MAX_VERSION;
 static bool __read_mostly opt_transitive_grants = true;
+static bool __read_mostly opt_grant_transfer = true;
 
 static int __init parse_gnttab(const char *s)
 {
@@ -204,6 +205,8 @@ static int __init parse_gnttab(const char *s)
         }
         else if ( (val = parse_boolean("transitive", s, ss)) >= 0 )
             opt_transitive_grants = val;
+        else if ( (val = parse_boolean("transfer", s, ss)) >= 0 )
+            opt_grant_transfer = val;
         else
             rc = -EINVAL;
 
@@ -2233,6 +2236,9 @@ gnttab_transfer(
     unsigned int max_bitsize;
     struct active_grant_entry *act;
 
+    if ( !opt_grant_transfer )
+        return -ENOSYS;
+
     for ( i = 0; i < count; i++ )
     {
         bool_t okay;
-- 
2.34.1

On 01.02.2022 10:02, Juergen Gross wrote:
> The grant table operation GNTTABOP_transfer is meant to be used in
> PV device backends, and it hasn't been used in Linux since the old
> Xen-o-Linux days.

Kind of unusual spelling of XenoLinux ;-)

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -181,6 +181,7 @@ static int parse_gnttab_max_maptrack_frames(const char *arg)
>  
>  unsigned int __read_mostly opt_gnttab_max_version = GNTTAB_MAX_VERSION;
>  static bool __read_mostly opt_transitive_grants = true;
> +static bool __read_mostly opt_grant_transfer = true;

If this was conditional upon PV (with a #define to false in the
opposite case), it could be __ro_after_init right away, while at
the same time allowing the compiler to eliminate gnttab_transfer().

> @@ -204,6 +205,8 @@ static int __init parse_gnttab(const char *s)
>          }
>          else if ( (val = parse_boolean("transitive", s, ss)) >= 0 )
>              opt_transitive_grants = val;
> +        else if ( (val = parse_boolean("transfer", s, ss)) >= 0 )
> +            opt_grant_transfer = val;
>          else
>              rc = -EINVAL;

To possibly save a further roundtrip: If the PV dependency was added
above, I'd like to ask to follow the model of parse_iommu_param()
here and use "#ifndef opt_grant_transfer" around the added code in
favor of "#ifdef CONFIG_PV".

> @@ -2233,6 +2236,9 @@ gnttab_transfer(
>      unsigned int max_bitsize;
>      struct active_grant_entry *act;
>  
> +    if ( !opt_grant_transfer )
> +        return -ENOSYS;

-EOPNOTSUPP please.

Jan

On 03.02.22 10:10, Jan Beulich wrote:
> On 01.02.2022 10:02, Juergen Gross wrote:
>> The grant table operation GNTTABOP_transfer is meant to be used in
>> PV device backends, and it hasn't been used in Linux since the old
>> Xen-o-Linux days.
> 
> Kind of unusual spelling of XenoLinux ;-)
> 
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -181,6 +181,7 @@ static int parse_gnttab_max_maptrack_frames(const char *arg)
>>   
>>   unsigned int __read_mostly opt_gnttab_max_version = GNTTAB_MAX_VERSION;
>>   static bool __read_mostly opt_transitive_grants = true;
>> +static bool __read_mostly opt_grant_transfer = true;
> 
> If this was conditional upon PV (with a #define to false in the
> opposite case), it could be __ro_after_init right away, while at
> the same time allowing the compiler to eliminate gnttab_transfer().

Nice idea. The other option would be to put all (or most) of
gnttab_transfer() in a "#ifdef CONFIG_PV" section, allowing to
remove the "#ifdef CONFIG_X86" parts in it, too.

> 
>> @@ -204,6 +205,8 @@ static int __init parse_gnttab(const char *s)
>>           }
>>           else if ( (val = parse_boolean("transitive", s, ss)) >= 0 )
>>               opt_transitive_grants = val;
>> +        else if ( (val = parse_boolean("transfer", s, ss)) >= 0 )
>> +            opt_grant_transfer = val;
>>           else
>>               rc = -EINVAL;
> 
> To possibly save a further roundtrip: If the PV dependency was added
> above, I'd like to ask to follow the model of parse_iommu_param()
> here and use "#ifndef opt_grant_transfer" around the added code in
> favor of "#ifdef CONFIG_PV".

Okay.

> 
>> @@ -2233,6 +2236,9 @@ gnttab_transfer(
>>       unsigned int max_bitsize;
>>       struct active_grant_entry *act;
>>   
>> +    if ( !opt_grant_transfer )
>> +        return -ENOSYS;
> 
> -EOPNOTSUPP please.

Yes, that's better.


Juergen

On 03.02.2022 11:55, Juergen Gross wrote:
> On 03.02.22 10:10, Jan Beulich wrote:
>> On 01.02.2022 10:02, Juergen Gross wrote:
>>> The grant table operation GNTTABOP_transfer is meant to be used in
>>> PV device backends, and it hasn't been used in Linux since the old
>>> Xen-o-Linux days.
>>
>> Kind of unusual spelling of XenoLinux ;-)
>>
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -181,6 +181,7 @@ static int parse_gnttab_max_maptrack_frames(const char *arg)
>>>   
>>>   unsigned int __read_mostly opt_gnttab_max_version = GNTTAB_MAX_VERSION;
>>>   static bool __read_mostly opt_transitive_grants = true;
>>> +static bool __read_mostly opt_grant_transfer = true;
>>
>> If this was conditional upon PV (with a #define to false in the
>> opposite case), it could be __ro_after_init right away, while at
>> the same time allowing the compiler to eliminate gnttab_transfer().
> 
> Nice idea. The other option would be to put all (or most) of
> gnttab_transfer() in a "#ifdef CONFIG_PV" section, allowing to
> remove the "#ifdef CONFIG_X86" parts in it, too.

Yes, sure. The downside being that then this code won't be compile-
tested anymore in !PV builds. Yet keeping code visible to compilers
is what we aim for elsewhere by preferring if(IS_ENABLED(...)) over
#ifdef where possible.

Jan