[PATCH-for-4.13,v5] Rationalize max_grant_frames and max_maptrack_frames handling

Commit Message

Paul Durrant Nov. 28, 2019, 4:52 p.m. UTC

From: George Dunlap <[e-mail]>

Xen used to have single, system-wide limits for the number of grant
frames and maptrack frames a guest was allowed to create. Increasing
or decreasing this single limit on the Xen command-line would change
the limit for all guests on the system.

Later, per-domain limits for these values was created. The system-wide
limits became strict limits: domains could not be created with higher
limits, but could be created with lower limits. However, that change
also introduced a range of different "default" values into various
places in the toolstack:

- The python libxc bindings hard-coded these values to 32 and 1024,
  respectively
- The libxl default values are 32 and 1024 respectively.
- xl will use the libxl default for maptrack, but does its own default
  calculation for grant frames: either 32 or 64, based on the max
  possible mfn.

These defaults interact poorly with the hypervisor command-line limit:

- The hypervisor command-line limit cannot be used to raise the limit
  for all guests anymore, as the default in the toolstack will
  effectively override this.
- If you use the hypervisor command-line limit to *reduce* the limit,
  then the "default" values generated by the toolstack are too high,
  and all guest creations will fail.

In other words, the toolstack defaults require any change to be
effected by having the admin explicitly specify a new value in every
guest.

In order to address this, have grant_table_init treat negative values
for max_grant_frames and max_maptrack_frames as instructions to use the
system-wide default, and have all the above toolstacks default to passing
-1 unless a different value is explicitly configured.

This restores the old behavior in that changing the hypervisor command-line
option can change the behavior for all guests, while retaining the ability
to set per-guest values.  It also removes the bug that reducing the
system-wide max will cause all domains without explicit limits to fail.

NOTE: - The Ocaml bindings require the caller to always specify a value,
        and the code to start a xenstored stubdomain hard-codes these to 4
	and 128 respectively; this behavour will not be modified.

Signed-off-by: George Dunlap <[e-mail]>
Signed-off-by: Paul Durrant <[e-mail]>
---
Cc: Ian Jackson <[e-mail]>
Cc: Wei Liu <[e-mail]>
Cc: Andrew Cooper <[e-mail]>
Cc: George Dunlap <[e-mail]>
Cc: Jan Beulich <[e-mail]>
Cc: Julien Grall <[e-mail]>
Cc: Konrad Rzeszutek Wilk <[e-mail]>
Cc: Stefano Stabellini <[e-mail]>
Cc: Anthony PERARD <[e-mail]>
Cc: "Marek Marczykowski-Górecki" <[e-mail]>
Cc: Volodymyr Babchuk <[e-mail]>
Cc: "Roger Pau Monné" <[e-mail]>

v5:
 - Remove erroneous __init annotations
 - Fail out of range command line values with ERANGE
 - Make opt_max_maptrack_frames static

v4:
 - Add missing braces in xlu_cfg_get_bounded_long()

v3:
 - Make sure that specified values cannot be negative or overflow a
   signed int

v2:
 - re-worked George's original commit massage a little
 - fixed the text in xl.conf.5.pod
 - use -1 as the sentinel value for 'default' and < 0 for checking it
---
 docs/man/xl.conf.5.pod            |  6 +++--
 tools/libxl/libxl.h               |  4 +--
 tools/libxl/libxl_types.idl       |  4 +--
 tools/libxl/libxlu_cfg.c          | 26 ++++++++++++++++--
 tools/libxl/libxlutil.h           |  2 ++
 tools/python/xen/lowlevel/xc/xc.c |  4 +--
 tools/xl/xl.c                     | 15 ++++-------
 tools/xl/xl_parse.c               |  9 ++++---
 xen/arch/arm/setup.c              |  2 +-
 xen/arch/x86/setup.c              |  4 +--
 xen/common/grant_table.c          | 45 +++++++++++++++++++++++++++----
 xen/include/public/domctl.h       | 10 ++++---
 xen/include/xen/grant_table.h     | 10 +++----
 13 files changed, 100 insertions(+), 41 deletions(-)

Comments

Jan Beulich Nov. 29, 2019, 10:22 a.m. UTC | #1

On 28.11.2019 17:52, Paul Durrant wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -84,11 +84,40 @@ struct grant_table {
>      struct grant_table_arch arch;
>  };
>  
> +static int parse_gnttab_limit(const char *param, const char *arg,
> +                              unsigned int *valp)
> +{
> +    const char *e;
> +    unsigned long val;
> +
> +    val = simple_strtoul(arg, &e, 0);
> +    if ( *e )
> +        return -EINVAL;
> +
> +    if ( val > INT_MAX )
> +        return -ERANGE;
> +
> +    return 0;
> +}

*valp doesn't get written to anymore. With this fixed (and no new
issues introduced ;-) ) hypervisor side
Reviewed-by: Jan Beulich <[e-mail]>

As an additional remark (not for this patch) - the original wrong
use of __init in v4 could have been replaced by __cold. But I
guess we may want to consider adding such in a wider fashion.

Jan

Jan Beulich Nov. 29, 2019, 10:28 a.m. UTC | #2

On 29.11.2019 11:22, Jan Beulich wrote:
> On 28.11.2019 17:52, Paul Durrant wrote:
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -84,11 +84,40 @@ struct grant_table {
>>      struct grant_table_arch arch;
>>  };
>>  
>> +static int parse_gnttab_limit(const char *param, const char *arg,
>> +                              unsigned int *valp)
>> +{
>> +    const char *e;
>> +    unsigned long val;
>> +
>> +    val = simple_strtoul(arg, &e, 0);
>> +    if ( *e )
>> +        return -EINVAL;
>> +
>> +    if ( val > INT_MAX )
>> +        return -ERANGE;
>> +
>> +    return 0;
>> +}
> 
> *valp doesn't get written to anymore. With this fixed (and no new
> issues introduced ;-) ) hypervisor side
> Reviewed-by: Jan Beulich <[e-mail]>

And I guess I should have clarified: I'd be fine adding the missing
assignment while committing, provided the tools side won't require
any changes.

Jan

Paul Durrant Nov. 29, 2019, 10:39 a.m. UTC | #3

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 29 November 2019 10:29
> To: Durrant, Paul <[e-mail]>
> Cc: Andrew Cooper <[e-mail]>; Anthony PERARD
> <[e-mail]>; George Dunlap <[e-mail]>;
> Roger Pau Monné <[e-mail]>; Volodymyr Babchuk
> <[e-mail]>; George Dunlap <[e-mail]>;
> Ian Jackson <[e-mail]>; Marek Marczykowski-Górecki
> <[e-mail]>; Stefano Stabellini
> <[e-mail]>; [e-mail]; Konrad Rzeszutek
> Wilk <[e-mail]>; Julien Grall <[e-mail]>; Wei Liu
> <[e-mail]>
> Subject: Re: [PATCH-for-4.13 v5] Rationalize max_grant_frames and
> max_maptrack_frames handling
> 
> On 29.11.2019 11:22, Jan Beulich wrote:
> > On 28.11.2019 17:52, Paul Durrant wrote:
> >> --- a/xen/common/grant_table.c
> >> +++ b/xen/common/grant_table.c
> >> @@ -84,11 +84,40 @@ struct grant_table {
> >>      struct grant_table_arch arch;
> >>  };
> >>
> >> +static int parse_gnttab_limit(const char *param, const char *arg,
> >> +                              unsigned int *valp)
> >> +{
> >> +    const char *e;
> >> +    unsigned long val;
> >> +
> >> +    val = simple_strtoul(arg, &e, 0);
> >> +    if ( *e )
> >> +        return -EINVAL;
> >> +
> >> +    if ( val > INT_MAX )
> >> +        return -ERANGE;
> >> +
> >> +    return 0;
> >> +}
> >
> > *valp doesn't get written to anymore.

That was intentional, given Juergen's comment...

> With this fixed (and no new
> > issues introduced ;-) ) hypervisor side
> > Reviewed-by: Jan Beulich <[e-mail]>
> 
> And I guess I should have clarified: I'd be fine adding the missing
> assignment while committing, provided the tools side won't require
> any changes.

...but if we want to allow dom0 to set itself up for INT_MAX frames in the event of a bad value then I'm not objecting.

  Paul

> 
> Jan

Jan Beulich Nov. 29, 2019, 10:46 a.m. UTC | #4

On 29.11.2019 11:39, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jan Beulich <[e-mail]>
>> Sent: 29 November 2019 10:29
>> To: Durrant, Paul <[e-mail]>
>> Cc: Andrew Cooper <[e-mail]>; Anthony PERARD
>> <[e-mail]>; George Dunlap <[e-mail]>;
>> Roger Pau Monné <[e-mail]>; Volodymyr Babchuk
>> <[e-mail]>; George Dunlap <[e-mail]>;
>> Ian Jackson <[e-mail]>; Marek Marczykowski-Górecki
>> <[e-mail]>; Stefano Stabellini
>> <[e-mail]>; [e-mail]; Konrad Rzeszutek
>> Wilk <[e-mail]>; Julien Grall <[e-mail]>; Wei Liu
>> <[e-mail]>
>> Subject: Re: [PATCH-for-4.13 v5] Rationalize max_grant_frames and
>> max_maptrack_frames handling
>>
>> On 29.11.2019 11:22, Jan Beulich wrote:
>>> On 28.11.2019 17:52, Paul Durrant wrote:
>>>> --- a/xen/common/grant_table.c
>>>> +++ b/xen/common/grant_table.c
>>>> @@ -84,11 +84,40 @@ struct grant_table {
>>>>      struct grant_table_arch arch;
>>>>  };
>>>>
>>>> +static int parse_gnttab_limit(const char *param, const char *arg,
>>>> +                              unsigned int *valp)
>>>> +{
>>>> +    const char *e;
>>>> +    unsigned long val;
>>>> +
>>>> +    val = simple_strtoul(arg, &e, 0);
>>>> +    if ( *e )
>>>> +        return -EINVAL;
>>>> +
>>>> +    if ( val > INT_MAX )
>>>> +        return -ERANGE;
>>>> +
>>>> +    return 0;
>>>> +}
>>>
>>> *valp doesn't get written to anymore.
> 
> That was intentional, given Juergen's comment...
> 
>> With this fixed (and no new
>>> issues introduced ;-) ) hypervisor side
>>> Reviewed-by: Jan Beulich <[e-mail]>
>>
>> And I guess I should have clarified: I'd be fine adding the missing
>> assignment while committing, provided the tools side won't require
>> any changes.
> 
> ...but if we want to allow dom0 to set itself up for INT_MAX frames
> in the event of a bad value then I'm not objecting.

Looks like you're misunderstanding, or I'm missing something:
The command line options right now won't take any effect, as
the opt_* global variables won't be written to at all. I'm not
taking about falling back to using INT_MAX when we've noticed
an out of bounds value.

Jan

Paul Durrant Nov. 29, 2019, 10:48 a.m. UTC | #5

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 29 November 2019 10:46
> To: Durrant, Paul <[e-mail]>
> Cc: Andrew Cooper <[e-mail]>; Anthony PERARD
> <[e-mail]>; George Dunlap <[e-mail]>;
> Roger Pau Monné <[e-mail]>; Volodymyr Babchuk
> <[e-mail]>; George Dunlap <[e-mail]>;
> Ian Jackson <[e-mail]>; Marek Marczykowski-Górecki
> <[e-mail]>; Stefano Stabellini
> <[e-mail]>; [e-mail]; Konrad Rzeszutek
> Wilk <[e-mail]>; Julien Grall <[e-mail]>; Wei Liu
> <[e-mail]>
> Subject: Re: [PATCH-for-4.13 v5] Rationalize max_grant_frames and
> max_maptrack_frames handling
> 
> On 29.11.2019 11:39, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <[e-mail]>
> >> Sent: 29 November 2019 10:29
> >> To: Durrant, Paul <[e-mail]>
> >> Cc: Andrew Cooper <[e-mail]>; Anthony PERARD
> >> <[e-mail]>; George Dunlap <[e-mail]>;
> >> Roger Pau Monné <[e-mail]>; Volodymyr Babchuk
> >> <[e-mail]>; George Dunlap
> <[e-mail]>;
> >> Ian Jackson <[e-mail]>; Marek Marczykowski-Górecki
> >> <[e-mail]>; Stefano Stabellini
> >> <[e-mail]>; [e-mail]; Konrad
> Rzeszutek
> >> Wilk <[e-mail]>; Julien Grall <[e-mail]>; Wei Liu
> >> <[e-mail]>
> >> Subject: Re: [PATCH-for-4.13 v5] Rationalize max_grant_frames and
> >> max_maptrack_frames handling
> >>
> >> On 29.11.2019 11:22, Jan Beulich wrote:
> >>> On 28.11.2019 17:52, Paul Durrant wrote:
> >>>> --- a/xen/common/grant_table.c
> >>>> +++ b/xen/common/grant_table.c
> >>>> @@ -84,11 +84,40 @@ struct grant_table {
> >>>>      struct grant_table_arch arch;
> >>>>  };
> >>>>
> >>>> +static int parse_gnttab_limit(const char *param, const char *arg,
> >>>> +                              unsigned int *valp)
> >>>> +{
> >>>> +    const char *e;
> >>>> +    unsigned long val;
> >>>> +
> >>>> +    val = simple_strtoul(arg, &e, 0);
> >>>> +    if ( *e )
> >>>> +        return -EINVAL;
> >>>> +
> >>>> +    if ( val > INT_MAX )
> >>>> +        return -ERANGE;
> >>>> +
> >>>> +    return 0;
> >>>> +}
> >>>
> >>> *valp doesn't get written to anymore.
> >
> > That was intentional, given Juergen's comment...
> >
> >> With this fixed (and no new
> >>> issues introduced ;-) ) hypervisor side
> >>> Reviewed-by: Jan Beulich <[e-mail]>
> >>
> >> And I guess I should have clarified: I'd be fine adding the missing
> >> assignment while committing, provided the tools side won't require
> >> any changes.
> >
> > ...but if we want to allow dom0 to set itself up for INT_MAX frames
> > in the event of a bad value then I'm not objecting.
> 
> Looks like you're misunderstanding, or I'm missing something:
> The command line options right now won't take any effect, as
> the opt_* global variables won't be written to at all. I'm not
> taking about falling back to using INT_MAX when we've noticed
> an out of bounds value.

Oh, sorry... too deep with my cutting. Yes, of course there should be a '*valp = val' just above 'return 0'.

  Paul

> 
> Jan

Jürgen Groß Nov. 29, 2019, 12:01 p.m. UTC | #6

On 29.11.19 11:22, Jan Beulich wrote:
> On 28.11.2019 17:52, Paul Durrant wrote:
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -84,11 +84,40 @@ struct grant_table {
>>       struct grant_table_arch arch;
>>   };
>>   
>> +static int parse_gnttab_limit(const char *param, const char *arg,
>> +                              unsigned int *valp)
>> +{
>> +    const char *e;
>> +    unsigned long val;
>> +
>> +    val = simple_strtoul(arg, &e, 0);
>> +    if ( *e )
>> +        return -EINVAL;
>> +
>> +    if ( val > INT_MAX )
>> +        return -ERANGE;
>> +
>> +    return 0;
>> +}
> 
> *valp doesn't get written to anymore. With this fixed (and no new
> issues introduced ;-) ) hypervisor side
> Reviewed-by: Jan Beulich <[e-mail]>

And:

Release-acked-by: Juergen Gross <[e-mail]>


Juergen

Jan Beulich Nov. 29, 2019, 12:17 p.m. UTC | #7

On 29.11.2019 13:01, Jürgen Groß wrote:
> On 29.11.19 11:22, Jan Beulich wrote:
>> On 28.11.2019 17:52, Paul Durrant wrote:
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -84,11 +84,40 @@ struct grant_table {
>>>       struct grant_table_arch arch;
>>>   };
>>>   
>>> +static int parse_gnttab_limit(const char *param, const char *arg,
>>> +                              unsigned int *valp)
>>> +{
>>> +    const char *e;
>>> +    unsigned long val;
>>> +
>>> +    val = simple_strtoul(arg, &e, 0);
>>> +    if ( *e )
>>> +        return -EINVAL;
>>> +
>>> +    if ( val > INT_MAX )
>>> +        return -ERANGE;
>>> +
>>> +    return 0;
>>> +}
>>
>> *valp doesn't get written to anymore. With this fixed (and no new
>> issues introduced ;-) ) hypervisor side
>> Reviewed-by: Jan Beulich <[e-mail]>
> 
> And:
> 
> Release-acked-by: Juergen Gross <[e-mail]>

Noted, but - ahead of a tool stack side ack? I.e. valid indefinitely
no matter when that one would arrive?

Jan

Jürgen Groß Nov. 29, 2019, 12:19 p.m. UTC | #8

On 29.11.19 13:17, Jan Beulich wrote:
> On 29.11.2019 13:01, Jürgen Groß wrote:
>> On 29.11.19 11:22, Jan Beulich wrote:
>>> On 28.11.2019 17:52, Paul Durrant wrote:
>>>> --- a/xen/common/grant_table.c
>>>> +++ b/xen/common/grant_table.c
>>>> @@ -84,11 +84,40 @@ struct grant_table {
>>>>        struct grant_table_arch arch;
>>>>    };
>>>>    
>>>> +static int parse_gnttab_limit(const char *param, const char *arg,
>>>> +                              unsigned int *valp)
>>>> +{
>>>> +    const char *e;
>>>> +    unsigned long val;
>>>> +
>>>> +    val = simple_strtoul(arg, &e, 0);
>>>> +    if ( *e )
>>>> +        return -EINVAL;
>>>> +
>>>> +    if ( val > INT_MAX )
>>>> +        return -ERANGE;
>>>> +
>>>> +    return 0;
>>>> +}
>>>
>>> *valp doesn't get written to anymore. With this fixed (and no new
>>> issues introduced ;-) ) hypervisor side
>>> Reviewed-by: Jan Beulich <[e-mail]>
>>
>> And:
>>
>> Release-acked-by: Juergen Gross <[e-mail]>
> 
> Noted, but - ahead of a tool stack side ack? I.e. valid indefinitely
> no matter when that one would arrive?

We agreed this one to be a blocker, right?

Nevertheless: tools maintainers, please?


Juergen

Wei Liu Nov. 29, 2019, 12:44 p.m. UTC | #9

On Thu, Nov 28, 2019 at 04:52:24PM +0000, Paul Durrant wrote:
> From: George Dunlap <[e-mail]>
> 
> Xen used to have single, system-wide limits for the number of grant
> frames and maptrack frames a guest was allowed to create. Increasing
> or decreasing this single limit on the Xen command-line would change
> the limit for all guests on the system.
> 
> Later, per-domain limits for these values was created. The system-wide
> limits became strict limits: domains could not be created with higher
> limits, but could be created with lower limits. However, that change
> also introduced a range of different "default" values into various
> places in the toolstack:
> 
> - The python libxc bindings hard-coded these values to 32 and 1024,
>   respectively
> - The libxl default values are 32 and 1024 respectively.
> - xl will use the libxl default for maptrack, but does its own default
>   calculation for grant frames: either 32 or 64, based on the max
>   possible mfn.
> 
> These defaults interact poorly with the hypervisor command-line limit:
> 
> - The hypervisor command-line limit cannot be used to raise the limit
>   for all guests anymore, as the default in the toolstack will
>   effectively override this.
> - If you use the hypervisor command-line limit to *reduce* the limit,
>   then the "default" values generated by the toolstack are too high,
>   and all guest creations will fail.
> 
> In other words, the toolstack defaults require any change to be
> effected by having the admin explicitly specify a new value in every
> guest.
> 
> In order to address this, have grant_table_init treat negative values
> for max_grant_frames and max_maptrack_frames as instructions to use the
> system-wide default, and have all the above toolstacks default to passing
> -1 unless a different value is explicitly configured.
> 
> This restores the old behavior in that changing the hypervisor command-line
> option can change the behavior for all guests, while retaining the ability
> to set per-guest values.  It also removes the bug that reducing the
> system-wide max will cause all domains without explicit limits to fail.
> 
> NOTE: - The Ocaml bindings require the caller to always specify a value,
>         and the code to start a xenstored stubdomain hard-codes these to 4
> 	and 128 respectively; this behavour will not be modified.
> 
> Signed-off-by: George Dunlap <[e-mail]>
> Signed-off-by: Paul Durrant <[e-mail]>
> ---
> Cc: Ian Jackson <[e-mail]>
> Cc: Wei Liu <[e-mail]>
> Cc: Andrew Cooper <[e-mail]>
> Cc: George Dunlap <[e-mail]>
> Cc: Jan Beulich <[e-mail]>
> Cc: Julien Grall <[e-mail]>
> Cc: Konrad Rzeszutek Wilk <[e-mail]>
> Cc: Stefano Stabellini <[e-mail]>
> Cc: Anthony PERARD <[e-mail]>
> Cc: "Marek Marczykowski-Górecki" <[e-mail]>
> Cc: Volodymyr Babchuk <[e-mail]>
> Cc: "Roger Pau Monné" <[e-mail]>
> 
> v5:
>  - Remove erroneous __init annotations
>  - Fail out of range command line values with ERANGE
>  - Make opt_max_maptrack_frames static
> 
> v4:
>  - Add missing braces in xlu_cfg_get_bounded_long()
> 
> v3:
>  - Make sure that specified values cannot be negative or overflow a
>    signed int
> 
> v2:
>  - re-worked George's original commit massage a little
>  - fixed the text in xl.conf.5.pod
>  - use -1 as the sentinel value for 'default' and < 0 for checking it
> ---
>  docs/man/xl.conf.5.pod            |  6 +++--
>  tools/libxl/libxl.h               |  4 +--
>  tools/libxl/libxl_types.idl       |  4 +--
>  tools/libxl/libxlu_cfg.c          | 26 ++++++++++++++++--
>  tools/libxl/libxlutil.h           |  2 ++
>  tools/python/xen/lowlevel/xc/xc.c |  4 +--
>  tools/xl/xl.c                     | 15 ++++-------
>  tools/xl/xl_parse.c               |  9 ++++---
>  xen/arch/arm/setup.c              |  2 +-
>  xen/arch/x86/setup.c              |  4 +--
>  xen/common/grant_table.c          | 45 +++++++++++++++++++++++++++----
>  xen/include/public/domctl.h       | 10 ++++---
>  xen/include/xen/grant_table.h     | 10 +++----
>  13 files changed, 100 insertions(+), 41 deletions(-)
> 
> diff --git a/docs/man/xl.conf.5.pod b/docs/man/xl.conf.5.pod
> index 962144e38e..207ab3e77a 100644
> --- a/docs/man/xl.conf.5.pod
> +++ b/docs/man/xl.conf.5.pod
> @@ -81,13 +81,15 @@ Default: C</var/lock/xl>
>  
>  Sets the default value for the C<max_grant_frames> domain config value.
>  
> -Default: C<32> on hosts up to 16TB of memory, C<64> on hosts larger than 16TB
> +Default: value of Xen command line B<gnttab_max_frames> parameter (or its
> +default value if unspecified).
>  
>  =item B<max_maptrack_frames=NUMBER>
>  
>  Sets the default value for the C<max_maptrack_frames> domain config value.
>  
> -Default: C<1024>
> +Default: value of Xen command line B<gnttab_max_maptrack_frames>
> +parameter (or its default value if unspecified).
>  
>  =item B<vif.default.script="PATH">
>  
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 49b56fa1a3..a2a5d321c5 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -364,8 +364,8 @@
>   */
>  #define LIBXL_HAVE_BUILDINFO_GRANT_LIMITS 1
>  
> -#define LIBXL_MAX_GRANT_FRAMES_DEFAULT 32
> -#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT 1024
> +#define LIBXL_MAX_GRANT_FRAMES_DEFAULT -1
> +#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT -1
>  
>  /*
>   * LIBXL_HAVE_BUILDINFO_* indicates that libxl_domain_build_info has
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 0546d7865a..63e29bb2fb 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -511,8 +511,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
>  
>      ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
>  
> -    ("max_grant_frames",    uint32, {'init_val': 'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}),
> -    ("max_maptrack_frames", uint32, {'init_val': 'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}),
> +    ("max_grant_frames",    integer, {'init_val': 'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}),
> +    ("max_maptrack_frames", integer, {'init_val': 'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}),

What if we use 0xffffffff to denote default instead? That wouldn't
require changing the type here.

The type change here makes me feel a bit uncomfortable, though in
practice it may not matter. I don't see anyone would specify a value
that would become negative when cast from uint32 to integer.

If the decision is to change the type, please provide a #define in
libxl.h.

Ian and Anthony, your opinion?

Wei.

Anthony PERARD Nov. 29, 2019, 12:46 p.m. UTC | #10

On Thu, Nov 28, 2019 at 04:52:24PM +0000, Paul Durrant wrote:
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 49b56fa1a3..a2a5d321c5 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -364,8 +364,8 @@
>   */
>  #define LIBXL_HAVE_BUILDINFO_GRANT_LIMITS 1
>  
> -#define LIBXL_MAX_GRANT_FRAMES_DEFAULT 32
> -#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT 1024
> +#define LIBXL_MAX_GRANT_FRAMES_DEFAULT -1
> +#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT -1
>  
>  /*
>   * LIBXL_HAVE_BUILDINFO_* indicates that libxl_domain_build_info has
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 0546d7865a..63e29bb2fb 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -511,8 +511,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
>  
>      ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
>  
> -    ("max_grant_frames",    uint32, {'init_val': 'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}),
> -    ("max_maptrack_frames", uint32, {'init_val': 'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}),
> +    ("max_grant_frames",    integer, {'init_val': 'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}),
> +    ("max_maptrack_frames", integer, {'init_val': 'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}),

That's a change in the libxl API, could you add a LIBX_HAVE_* macro?

>      
>      ("device_model_version", libxl_device_model_version),
>      ("device_model_stubdomain", libxl_defbool),
> diff --git a/tools/libxl/libxlu_cfg.c b/tools/libxl/libxlu_cfg.c
> index 72815d25dd..cafc632fc1 100644
> --- a/tools/libxl/libxlu_cfg.c
> +++ b/tools/libxl/libxlu_cfg.c
> @@ -268,8 +268,9 @@ int xlu_cfg_replace_string(const XLU_Config *cfg, const char *n,
>      return 0;
>  }
>  
> -int xlu_cfg_get_long(const XLU_Config *cfg, const char *n,
> -                     long *value_r, int dont_warn) {
> +int xlu_cfg_get_bounded_long(const XLU_Config *cfg, const char *n,
> +                             long min, long max, long *value_r,
> +                             int dont_warn) {
>      long l;
>      XLU_ConfigSetting *set;
>      int e;
> @@ -303,10 +304,31 @@ int xlu_cfg_get_long(const XLU_Config *cfg, const char *n,
>                      cfg->config_source, set->lineno, n);
>          return EINVAL;
>      }
> +    if (l < min) {
> +        if (!dont_warn)
> +            fprintf(cfg->report,
> +                    "%s:%d: warning: value `%ld' is smaller than minimum bound '%ld'\n",
> +                    cfg->config_source, set->lineno, l, min);
> +        return EINVAL;
> +    }
> +    if (l > max) {
> +        if (!dont_warn)
> +            fprintf(cfg->report,
> +                    "%s:%d: warning: value `%ld' is greater than maximum bound '%ld'\n",
> +                    cfg->config_source, set->lineno, l, max);
> +        return EINVAL;
> +    }

I'm not sure what was the intention with the new function
xlu_cfg_get_bounded_long(), but I don't think libxlu is the right place
for it. That function is only going to make it harder for users to find
mistakes in the config file. If `n' value is out of bound, it will only
get ignored, and xl will keep going. I think xlu_cfg should only be a
parser (and can check for syntax error).

Can you move that function to xl?

Thanks,

Paul Durrant Nov. 29, 2019, 12:51 p.m. UTC | #11

> -----Original Message-----
> From: Anthony PERARD <[e-mail]>
> Sent: 29 November 2019 12:46
> To: Durrant, Paul <[e-mail]>
> Cc: [e-mail]; George Dunlap
> <[e-mail]>; Ian Jackson <[e-mail]>; Wei
> Liu <[e-mail]>; Andrew Cooper <[e-mail]>; George Dunlap
> <[e-mail]>; Jan Beulich <[e-mail]>; Julien
> Grall <[e-mail]>; Konrad Rzeszutek Wilk <[e-mail]>;
> Stefano Stabellini <[e-mail]>; Marek Marczykowski-Górecki
> <[e-mail]>; Volodymyr Babchuk
> <[e-mail]>; Roger Pau Monné <[e-mail]>
> Subject: Re: [PATCH-for-4.13 v5] Rationalize max_grant_frames and
> max_maptrack_frames handling
> 
> On Thu, Nov 28, 2019 at 04:52:24PM +0000, Paul Durrant wrote:
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 49b56fa1a3..a2a5d321c5 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -364,8 +364,8 @@
> >   */
> >  #define LIBXL_HAVE_BUILDINFO_GRANT_LIMITS 1
> >
> > -#define LIBXL_MAX_GRANT_FRAMES_DEFAULT 32
> > -#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT 1024
> > +#define LIBXL_MAX_GRANT_FRAMES_DEFAULT -1
> > +#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT -1
> >
> >  /*
> >   * LIBXL_HAVE_BUILDINFO_* indicates that libxl_domain_build_info has
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 0546d7865a..63e29bb2fb 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -511,8 +511,8 @@ libxl_domain_build_info =
> Struct("domain_build_info",[
> >
> >      ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
> >
> > -    ("max_grant_frames",    uint32, {'init_val':
> 'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}),
> > -    ("max_maptrack_frames", uint32, {'init_val':
> 'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}),
> > +    ("max_grant_frames",    integer, {'init_val':
> 'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}),
> > +    ("max_maptrack_frames", integer, {'init_val':
> 'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}),
> 
> That's a change in the libxl API, could you add a LIBX_HAVE_* macro?
> 

Is it really, in practice?

> >
> >      ("device_model_version", libxl_device_model_version),
> >      ("device_model_stubdomain", libxl_defbool),
> > diff --git a/tools/libxl/libxlu_cfg.c b/tools/libxl/libxlu_cfg.c
> > index 72815d25dd..cafc632fc1 100644
> > --- a/tools/libxl/libxlu_cfg.c
> > +++ b/tools/libxl/libxlu_cfg.c
> > @@ -268,8 +268,9 @@ int xlu_cfg_replace_string(const XLU_Config *cfg,
> const char *n,
> >      return 0;
> >  }
> >
> > -int xlu_cfg_get_long(const XLU_Config *cfg, const char *n,
> > -                     long *value_r, int dont_warn) {
> > +int xlu_cfg_get_bounded_long(const XLU_Config *cfg, const char *n,
> > +                             long min, long max, long *value_r,
> > +                             int dont_warn) {
> >      long l;
> >      XLU_ConfigSetting *set;
> >      int e;
> > @@ -303,10 +304,31 @@ int xlu_cfg_get_long(const XLU_Config *cfg, const
> char *n,
> >                      cfg->config_source, set->lineno, n);
> >          return EINVAL;
> >      }
> > +    if (l < min) {
> > +        if (!dont_warn)
> > +            fprintf(cfg->report,
> > +                    "%s:%d: warning: value `%ld' is smaller than
> minimum bound '%ld'\n",
> > +                    cfg->config_source, set->lineno, l, min);
> > +        return EINVAL;
> > +    }
> > +    if (l > max) {
> > +        if (!dont_warn)
> > +            fprintf(cfg->report,
> > +                    "%s:%d: warning: value `%ld' is greater than
> maximum bound '%ld'\n",
> > +                    cfg->config_source, set->lineno, l, max);
> > +        return EINVAL;
> > +    }
> 
> I'm not sure what was the intention with the new function
> xlu_cfg_get_bounded_long(), but I don't think libxlu is the right place
> for it. That function is only going to make it harder for users to find
> mistakes in the config file. If `n' value is out of bound, it will only
> get ignored, and xl will keep going. I think xlu_cfg should only be a
> parser (and can check for syntax error).
> 
> Can you move that function to xl?
> 

I can, but why is this not considered useful in libxl? The call returns failure for an out-of-bounds check. If xl currently chooses to treat EINVAL as ENOENT then that's xl's bug to deal with.

  Paul

> Thanks,
> 
> --
> Anthony PERARD

Anthony PERARD Nov. 29, 2019, 1:52 p.m. UTC | #12

On Fri, Nov 29, 2019 at 12:51:47PM +0000, Durrant, Paul wrote:
> > -----Original Message-----
> > From: Anthony PERARD <[e-mail]>
> > Sent: 29 November 2019 12:46
> > I'm not sure what was the intention with the new function
> > xlu_cfg_get_bounded_long(), but I don't think libxlu is the right place
> > for it. That function is only going to make it harder for users to find
> > mistakes in the config file. If `n' value is out of bound, it will only
> > get ignored, and xl will keep going. I think xlu_cfg should only be a
> > parser (and can check for syntax error).
> > 
> > Can you move that function to xl?
> > 
> 
> I can, but why is this not considered useful in libxl? The call returns failure for an out-of-bounds check.

Sorry that the repo layout is confusing, but libxl != libxlu. libxl
doesn't even use libxlu!

> If xl currently chooses to treat EINVAL as ENOENT then that's xl's bug to deal with.

The general use of xlu_cfg_get_*() that treats all errors as ENOENT in
xl is an issue, I think, but this patch does the same thing and treat
EINVAL as ENOENT when using the newly introduced
xlu_cfg_get_bounded_long() function. I don't think that an xl bug to
deal with, but an issue with the patch.

Cheers,

Paul Durrant Nov. 29, 2019, 2:04 p.m. UTC | #13

> -----Original Message-----
> From: Anthony PERARD <[e-mail]>
> Sent: 29 November 2019 13:52
> To: Durrant, Paul <[e-mail]>
> Cc: [e-mail]; George Dunlap
> <[e-mail]>; Ian Jackson <[e-mail]>; Wei
> Liu <[e-mail]>; Andrew Cooper <[e-mail]>; George Dunlap
> <[e-mail]>; Jan Beulich <[e-mail]>; Julien
> Grall <[e-mail]>; Konrad Rzeszutek Wilk <[e-mail]>;
> Stefano Stabellini <[e-mail]>; Marek Marczykowski-Górecki
> <[e-mail]>; Volodymyr Babchuk
> <[e-mail]>; Roger Pau Monné <[e-mail]>
> Subject: Re: [PATCH-for-4.13 v5] Rationalize max_grant_frames and
> max_maptrack_frames handling
> 
> On Fri, Nov 29, 2019 at 12:51:47PM +0000, Durrant, Paul wrote:
> > > -----Original Message-----
> > > From: Anthony PERARD <[e-mail]>
> > > Sent: 29 November 2019 12:46
> > > I'm not sure what was the intention with the new function
> > > xlu_cfg_get_bounded_long(), but I don't think libxlu is the right
> place
> > > for it. That function is only going to make it harder for users to
> find
> > > mistakes in the config file. If `n' value is out of bound, it will
> only
> > > get ignored, and xl will keep going. I think xlu_cfg should only be a
> > > parser (and can check for syntax error).
> > >
> > > Can you move that function to xl?
> > >
> >
> > I can, but why is this not considered useful in libxl? The call returns
> failure for an out-of-bounds check.
> 
> Sorry that the repo layout is confusing, but libxl != libxlu. libxl
> doesn't even use libxlu!

Oh, that is confusing... there is code under tools/libxl that is not use by libxl; totally sane, of course.

> 
> > If xl currently chooses to treat EINVAL as ENOENT then that's xl's bug
> to deal with.
> 
> The general use of xlu_cfg_get_*() that treats all errors as ENOENT in
> xl is an issue, I think, but this patch does the same thing and treat
> EINVAL as ENOENT when using the newly introduced
> xlu_cfg_get_bounded_long() function. I don't think that an xl bug to
> deal with, but an issue with the patch.
> 

True, but I don't think that makes things strictly worse. I'll send v6 with extra checks though.

  Paul

> Cheers,
> 
> --
> Anthony PERARD

Ian Jackson Nov. 29, 2019, 3:47 p.m. UTC | #14

Wei Liu writes ("Re: [PATCH-for-4.13 v5] Rationalize max_grant_frames and max_maptrack_frames handling"):
> What if we use 0xffffffff to denote default instead? That wouldn't
> require changing the type here.

Is there some reason we wouldn't use ~0 to mean default ?

In the tools area we normally spell this as
     ~(some appropriate type)0
to make sure it has the right width.  But if we know the type and it
is of fixed length, as here, 0xffffffffu is OK too.

> The type change here makes me feel a bit uncomfortable, though in
> practice it may not matter. I don't see anyone would specify a value
> that would become negative when cast from uint32 to integer.

The problem with the type change is that in principle we have to audit
all the places the variables are used.

Ian.

Paul Durrant Nov. 29, 2019, 3:57 p.m. UTC | #15

> -----Original Message-----
> From: Ian Jackson <[e-mail]>
> Sent: 29 November 2019 15:47
> To: Wei Liu <[e-mail]>
> Cc: Durrant, Paul <[e-mail]>; Anthony Perard
> <[e-mail]>; [e-mail]; George Dunlap
> <[e-mail]>; Andrew Cooper <[e-mail]>; Jan
> Beulich <[e-mail]>; Julien Grall <[e-mail]>; Konrad
> Rzeszutek Wilk <[e-mail]>; Stefano Stabellini
> <[e-mail]>; Marek Marczykowski-Górecki
> <[e-mail]>; Volodymyr Babchuk
> <[e-mail]>; Roger Pau Monne <[e-mail]>
> Subject: Re: [PATCH-for-4.13 v5] Rationalize max_grant_frames and
> max_maptrack_frames handling
> 
> Wei Liu writes ("Re: [PATCH-for-4.13 v5] Rationalize max_grant_frames and
> max_maptrack_frames handling"):
> > What if we use 0xffffffff to denote default instead? That wouldn't
> > require changing the type here.
> 
> Is there some reason we wouldn't use ~0 to mean default ?
> 
> In the tools area we normally spell this as
>      ~(some appropriate type)0
> to make sure it has the right width.  But if we know the type and it
> is of fixed length, as here, 0xffffffffu is OK too.
> 
> > The type change here makes me feel a bit uncomfortable, though in
> > practice it may not matter. I don't see anyone would specify a value
> > that would become negative when cast from uint32 to integer.
> 
> The problem with the type change is that in principle we have to audit
> all the places the variables are used.
> 

Can a toolstack maintainer please come up with a concrete suggestion as to what the patch should do then? It's now at v6 and time is short.

  Paul

Ian Jackson Nov. 29, 2019, 4:07 p.m. UTC | #16

Durrant, Paul writes ("RE: [PATCH-for-4.13 v5] Rationalize max_grant_frames and max_maptrack_frames handling"):
> > -----Original Message-----
> > From: Ian Jackson <[e-mail]>
...
> > Is there some reason we wouldn't use ~0 to mean default ?
> > 
> > In the tools area we normally spell this as
> >      ~(some appropriate type)0
> > to make sure it has the right width.  But if we know the type and it
> > is of fixed length, as here, 0xffffffffu is OK too.
> > 
> > > The type change here makes me feel a bit uncomfortable, though in
> > > practice it may not matter. I don't see anyone would specify a value
> > > that would become negative when cast from uint32 to integer.
> > 
> > The problem with the type change is that in principle we have to audit
> > all the places the variables are used.
> 
> Can a toolstack maintainer please come up with a concrete suggestion as to what the patch should do then? It's now at v6 and time is short.

I think our proposal is to drop the type change, continue to use
uint32_t everwhere for these values, and specify the "use default"
value to be all-bits-set.

Ian.

George Dunlap Nov. 29, 2019, 4:09 p.m. UTC | #17

> On Nov 29, 2019, at 4:07 PM, Ian Jackson <[e-mail]> wrote:
> 
> Durrant, Paul writes ("RE: [PATCH-for-4.13 v5] Rationalize max_grant_frames and max_maptrack_frames handling"):
>>> -----Original Message-----
>>> From: Ian Jackson <[e-mail]>
> ...
>>> Is there some reason we wouldn't use ~0 to mean default ?
>>> 
>>> In the tools area we normally spell this as
>>>     ~(some appropriate type)0
>>> to make sure it has the right width.  But if we know the type and it
>>> is of fixed length, as here, 0xffffffffu is OK too.
>>> 
>>>> The type change here makes me feel a bit uncomfortable, though in
>>>> practice it may not matter. I don't see anyone would specify a value
>>>> that would become negative when cast from uint32 to integer.
>>> 
>>> The problem with the type change is that in principle we have to audit
>>> all the places the variables are used.
>> 
>> Can a toolstack maintainer please come up with a concrete suggestion as to what the patch should do then? It's now at v6 and time is short.
> 
> I think our proposal is to drop the type change, continue to use
> uint32_t everwhere for these values, and specify the "use default"
> value to be all-bits-set.

I tried to suggest something like this, but Jan didn’t like it for some reason.

 -George

Paul Durrant Nov. 29, 2019, 4:10 p.m. UTC | #18

> -----Original Message-----
> From: Ian Jackson <[e-mail]>
> Sent: 29 November 2019 16:08
> To: Durrant, Paul <[e-mail]>
> Cc: Wei Liu <[e-mail]>; Anthony Perard <[e-mail]>; xen-
> [e-mail]; George Dunlap <[e-mail]>;
> Andrew Cooper <[e-mail]>; Jan Beulich
> <[e-mail]>; Julien Grall <[e-mail]>; Konrad Rzeszutek Wilk
> <[e-mail]>; Stefano Stabellini <[e-mail]>;
> Marek Marczykowski-Górecki <[e-mail]>; Volodymyr
> Babchuk <[e-mail]>; Roger Pau Monne
> <[e-mail]>
> Subject: RE: [PATCH-for-4.13 v5] Rationalize max_grant_frames and
> max_maptrack_frames handling
> 
> Durrant, Paul writes ("RE: [PATCH-for-4.13 v5] Rationalize
> max_grant_frames and max_maptrack_frames handling"):
> > > -----Original Message-----
> > > From: Ian Jackson <[e-mail]>
> ...
> > > Is there some reason we wouldn't use ~0 to mean default ?
> > >
> > > In the tools area we normally spell this as
> > >      ~(some appropriate type)0
> > > to make sure it has the right width.  But if we know the type and it
> > > is of fixed length, as here, 0xffffffffu is OK too.
> > >
> > > > The type change here makes me feel a bit uncomfortable, though in
> > > > practice it may not matter. I don't see anyone would specify a value
> > > > that would become negative when cast from uint32 to integer.
> > >
> > > The problem with the type change is that in principle we have to audit
> > > all the places the variables are used.
> >
> > Can a toolstack maintainer please come up with a concrete suggestion as
> to what the patch should do then? It's now at v6 and time is short.
> 
> I think our proposal is to drop the type change, continue to use
> uint32_t everwhere for these values, and specify the "use default"
> value to be all-bits-set.
> 

Where? Everywhere or just in buildinfo? The switch from uint32_t to int32_t in the domctl does not, of course, change the width at all.

  Paul

Ian Jackson Nov. 29, 2019, 5:10 p.m. UTC | #19

Ian Jackson writes ("RE: [PATCH-for-4.13 v5] Rationalize max_grant_frames and max_maptrack_frames handling"):
> I think our proposal is to drop the type change, continue to use
> uint32_t everwhere for these values, and specify the "use default"
> value to be all-bits-set.

Following discussion on IRC:

I think we toolstack maintainers do not think it right to change the
type anywhere, but the hypervisor API/ABI is outside our bailiwick.

Changing the type in the libxl API is definitely undesirable in this
patch, especially as we will want to backport it.  Changing the type
can cause compile errors in callers (who are out of tree), or even
wrong behaviours.

We (Anthony and I, at least, having discussed it) feel that the
libxl_types.idl type should remain unchanged.

Furthermore, in the existing libxl API, LIBXL_MAX_GRANT_FRAMES_DEFAULT
and LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT have concrete values.  Callers
may have developed code which does something like
LIBXL_MAX_GRANT_FRAMES_DEFAULT*4 to avoid the max_grant_frames issue.
We need to keep those callers working.

I think the best way to do this is to retain those #defines with their
existing values, but to decouple them.  Instead, we provide a new
#define for the sentinel value and change the init_val in the IDL to
refer to that.

The effect is that callers who use _DEFAULT explicitly will see no
change (and the only reason to do that would probably be to increase
it as a bug workaround).  Callers who do not set the value at all will
get the sentinel value from init, which gets passed to the hypervisor
and used as the default.

(In principle callers might call _init on the domain config and then
modify the value they get, but I think that is both not entirely
reasonable and quite unlikely.  I think fixing this issue for others
callers is more important.)

To demonstrate exactly what I mean here is a delta which can be
squashed into v6.

Ian.

From 86d6e3b6e5434f0d872a195ed457a4af25a00208 Mon Sep 17 00:00:00 2001
From: Ian Jackson <[e-mail]>
Date: Fri, 29 Nov 2019 16:51:45 +0000
Subject: [PATCH] squash! Rationalize max_grant_frames and max_maptrack_frames
 handling

---
v7: Do not change type of libxl fields.
    Do not change values of LIBXL_MAX_GRANT_FRAMES_DEFAULT etc.
    in case someone has done LIBXL_MAX_GRANT_FRAMES_DEFAULT*4.
    Deprecate those and provide LIBXL_MAX_GRANT_DEFAULT instead.
---
 tools/libxl/libxl.h         | 16 +++++++++-------
 tools/libxl/libxl_types.idl |  4 ++--
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index b829c1bbce..54abb9db1f 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -364,15 +364,17 @@
  */
 #define LIBXL_HAVE_BUILDINFO_GRANT_LIMITS 1
 
+#define LIBXL_MAX_GRANT_DEFAULT (~(uint32_t)0)
+#define LIBXL_MAX_GRANT_FRAMES_DEFAULT 32 /* deprecated */
+#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT 1024 /* deprecated */
 /*
- * LIBXL_HAVE_BUILDINFO_SIGNED_GRANT_LIMITS indicates that the
- * signed max_grant_frames and max_maptrack_frames fields in
- * libxl_domain_build_info are signed quantities.
+ * LIBXL_HAVE_BUILDINFO_GRANT_DEFAULT indicates that the default
+ * values of max_grant_frames and max_maptrack_frames fields in
+ * libxl_domain_build_info are the special sentinel value
+ * LIBXL_MAX_GRANT_DEFAULT rather than the fixed values above.
+ * This means to use the hypervisor's default.
  */
-#define LIBXL_HAVE_BUILDINFO_SIGNED_GRANT_LIMITS 1
-
-#define LIBXL_MAX_GRANT_FRAMES_DEFAULT -1
-#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT -1
+#define LIBXL_HAVE_BUILDINFO_GRANT_DEFAULT 1
 
 /*
  * LIBXL_HAVE_BUILDINFO_* indicates that libxl_domain_build_info has
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 63e29bb2fb..7921950f6a 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -511,8 +511,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
 
     ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
 
-    ("max_grant_frames",    integer, {'init_val': 'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}),
-    ("max_maptrack_frames", integer, {'init_val': 'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}),
+    ("max_grant_frames",    uint32, {'init_val': 'LIBXL_MAX_GRANT_DEFAULT'}),
+    ("max_maptrack_frames", uint32, {'init_val': 'LIBXL_MAX_GRANT_DEFAULT'}),
     
     ("device_model_version", libxl_device_model_version),
     ("device_model_stubdomain", libxl_defbool),

Patch

diff --git a/docs/man/xl.conf.5.pod b/docs/man/xl.conf.5.pod
index 962144e38e..207ab3e77a 100644
--- a/docs/man/xl.conf.5.pod
+++ b/docs/man/xl.conf.5.pod
@@ -81,13 +81,15 @@  Default: C</var/lock/xl>
 
 Sets the default value for the C<max_grant_frames> domain config value.
 
-Default: C<32> on hosts up to 16TB of memory, C<64> on hosts larger than 16TB
+Default: value of Xen command line B<gnttab_max_frames> parameter (or its
+default value if unspecified).
 
 =item B<max_maptrack_frames=NUMBER>
 
 Sets the default value for the C<max_maptrack_frames> domain config value.
 
-Default: C<1024>
+Default: value of Xen command line B<gnttab_max_maptrack_frames>
+parameter (or its default value if unspecified).
 
 =item B<vif.default.script="PATH">
 
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 49b56fa1a3..a2a5d321c5 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -364,8 +364,8 @@ 
  */
 #define LIBXL_HAVE_BUILDINFO_GRANT_LIMITS 1
 
-#define LIBXL_MAX_GRANT_FRAMES_DEFAULT 32
-#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT 1024
+#define LIBXL_MAX_GRANT_FRAMES_DEFAULT -1
+#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT -1
 
 /*
  * LIBXL_HAVE_BUILDINFO_* indicates that libxl_domain_build_info has
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 0546d7865a..63e29bb2fb 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -511,8 +511,8 @@  libxl_domain_build_info = Struct("domain_build_info",[
 
     ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
 
-    ("max_grant_frames",    uint32, {'init_val': 'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}),
-    ("max_maptrack_frames", uint32, {'init_val': 'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}),
+    ("max_grant_frames",    integer, {'init_val': 'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}),
+    ("max_maptrack_frames", integer, {'init_val': 'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}),
     
     ("device_model_version", libxl_device_model_version),
     ("device_model_stubdomain", libxl_defbool),
diff --git a/tools/libxl/libxlu_cfg.c b/tools/libxl/libxlu_cfg.c
index 72815d25dd..cafc632fc1 100644
--- a/tools/libxl/libxlu_cfg.c
+++ b/tools/libxl/libxlu_cfg.c
@@ -268,8 +268,9 @@  int xlu_cfg_replace_string(const XLU_Config *cfg, const char *n,
     return 0;
 }
 
-int xlu_cfg_get_long(const XLU_Config *cfg, const char *n,
-                     long *value_r, int dont_warn) {
+int xlu_cfg_get_bounded_long(const XLU_Config *cfg, const char *n,
+                             long min, long max, long *value_r,
+                             int dont_warn) {
     long l;
     XLU_ConfigSetting *set;
     int e;
@@ -303,10 +304,31 @@  int xlu_cfg_get_long(const XLU_Config *cfg, const char *n,
                     cfg->config_source, set->lineno, n);
         return EINVAL;
     }
+    if (l < min) {
+        if (!dont_warn)
+            fprintf(cfg->report,
+                    "%s:%d: warning: value `%ld' is smaller than minimum bound '%ld'\n",
+                    cfg->config_source, set->lineno, l, min);
+        return EINVAL;
+    }
+    if (l > max) {
+        if (!dont_warn)
+            fprintf(cfg->report,
+                    "%s:%d: warning: value `%ld' is greater than maximum bound '%ld'\n",
+                    cfg->config_source, set->lineno, l, max);
+        return EINVAL;
+    }
+
     *value_r= l;
     return 0;
 }
 
+int xlu_cfg_get_long(const XLU_Config *cfg, const char *n,
+                     long *value_r, int dont_warn) {
+    return xlu_cfg_get_bounded_long(cfg, n, LONG_MIN, LONG_MAX, value_r,
+                                    dont_warn);
+}
+
 int xlu_cfg_get_defbool(const XLU_Config *cfg, const char *n, libxl_defbool *b,
                      int dont_warn)
 {
diff --git a/tools/libxl/libxlutil.h b/tools/libxl/libxlutil.h
index 057cc25cb2..92e35c5462 100644
--- a/tools/libxl/libxlutil.h
+++ b/tools/libxl/libxlutil.h
@@ -63,6 +63,8 @@  int xlu_cfg_replace_string(const XLU_Config *cfg, const char *n,
                            char **value_r, int dont_warn);
 int xlu_cfg_get_long(const XLU_Config*, const char *n, long *value_r,
                      int dont_warn);
+int xlu_cfg_get_bounded_long(const XLU_Config*, const char *n, long min,
+                             long max, long *value_r, int dont_warn);
 int xlu_cfg_get_defbool(const XLU_Config*, const char *n, libxl_defbool *b,
                      int dont_warn);
 
diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index 44d3606141..a751e85910 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -127,8 +127,8 @@  static PyObject *pyxc_domain_create(XcObject *self,
         },
         .max_vcpus = 1,
         .max_evtchn_port = -1, /* No limit. */
-        .max_grant_frames = 32,
-        .max_maptrack_frames = 1024,
+        .max_grant_frames = -1,
+        .max_maptrack_frames = -1,
     };
 
     static char *kwd_list[] = { "domid", "ssidref", "handle", "flags",
diff --git a/tools/xl/xl.c b/tools/xl/xl.c
index ddd29b3f1b..921c64f5ed 100644
--- a/tools/xl/xl.c
+++ b/tools/xl/xl.c
@@ -23,6 +23,7 @@ 
 #include <ctype.h>
 #include <inttypes.h>
 #include <regex.h>
+#include <limits.h>
 
 #include <libxl.h>
 #include <libxl_utils.h>
@@ -96,7 +97,6 @@  static void parse_global_config(const char *configfile,
     XLU_Config *config;
     int e;
     const char *buf;
-    libxl_physinfo physinfo;
 
     config = xlu_cfg_init(stderr, configfile);
     if (!config) {
@@ -197,16 +197,11 @@  static void parse_global_config(const char *configfile,
     xlu_cfg_replace_string (config, "colo.default.proxyscript",
         &default_colo_proxy_script, 0);
 
-    if (!xlu_cfg_get_long (config, "max_grant_frames", &l, 0))
+    if (!xlu_cfg_get_bounded_long (config, "max_grant_frames", 0, INT_MAX,
+                                   &l, 0))
         max_grant_frames = l;
-    else {
-        libxl_physinfo_init(&physinfo);
-        max_grant_frames = (libxl_get_physinfo(ctx, &physinfo) != 0 ||
-                            !(physinfo.max_possible_mfn >> 32))
-                           ? 32 : 64;
-        libxl_physinfo_dispose(&physinfo);
-    }
-    if (!xlu_cfg_get_long (config, "max_maptrack_frames", &l, 0))
+    if (!xlu_cfg_get_bounded_long (config, "max_maptrack_frames", 0,
+                                   INT_MAX, &l, 0))
         max_maptrack_frames = l;
 
     libxl_cpu_bitmap_alloc(ctx, &global_vm_affinity_mask, 0);
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 112f8ee026..555991dae3 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1411,13 +1411,16 @@  void parse_config_data(const char *config_source,
         !xlu_cfg_get_string (config, "cpus_soft", &buf, 0))
         parse_vcpu_affinity(b_info, cpus, buf, num_cpus, false);
 
-    if (!xlu_cfg_get_long (config, "max_grant_frames", &l, 0))
+    if (!xlu_cfg_get_bounded_long (config, "max_grant_frames", 0, INT_MAX,
+                                   &l, 0))
         b_info->max_grant_frames = l;
     else
         b_info->max_grant_frames = max_grant_frames;
-    if (!xlu_cfg_get_long (config, "max_maptrack_frames", &l, 0))
+
+    if (!xlu_cfg_get_bounded_long (config, "max_maptrack_frames", 0,
+                                   INT_MAX, &l, 0))
         b_info->max_maptrack_frames = l;
-    else if (max_maptrack_frames != -1)
+    else
         b_info->max_maptrack_frames = max_maptrack_frames;
 
     libxl_defbool_set(&b_info->claim_mode, claim_mode);
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 51d32106b7..3c899cd4a0 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -789,7 +789,7 @@  void __init start_xen(unsigned long boot_phys_offset,
         .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
         .max_evtchn_port = -1,
         .max_grant_frames = gnttab_dom0_frames(),
-        .max_maptrack_frames = opt_max_maptrack_frames,
+        .max_maptrack_frames = -1,
     };
     int rc;
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 00ee87bde5..7d27f36053 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -697,8 +697,8 @@  void __init noreturn __start_xen(unsigned long mbi_p)
     struct xen_domctl_createdomain dom0_cfg = {
         .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
         .max_evtchn_port = -1,
-        .max_grant_frames = opt_max_grant_frames,
-        .max_maptrack_frames = opt_max_maptrack_frames,
+        .max_grant_frames = -1,
+        .max_maptrack_frames = -1,
     };
 
     /* Critical region without IDT or TSS.  Any fault is deadly! */
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index b34d520f6d..28358d33b9 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -84,11 +84,40 @@  struct grant_table {
     struct grant_table_arch arch;
 };
 
+static int parse_gnttab_limit(const char *param, const char *arg,
+                              unsigned int *valp)
+{
+    const char *e;
+    unsigned long val;
+
+    val = simple_strtoul(arg, &e, 0);
+    if ( *e )
+        return -EINVAL;
+
+    if ( val > INT_MAX )
+        return -ERANGE;
+
+    return 0;
+}
+
 unsigned int __read_mostly opt_max_grant_frames = 64;
-integer_runtime_param("gnttab_max_frames", opt_max_grant_frames);
 
-unsigned int __read_mostly opt_max_maptrack_frames = 1024;
-integer_runtime_param("gnttab_max_maptrack_frames", opt_max_maptrack_frames);
+static int parse_gnttab_max_frames(const char *arg)
+{
+    return parse_gnttab_limit("gnttab_max_frames", arg,
+                              &opt_max_grant_frames);
+}
+custom_runtime_param("gnttab_max_frames", parse_gnttab_max_frames);
+
+static unsigned int __read_mostly opt_max_maptrack_frames = 1024;
+
+static int parse_gnttab_max_maptrack_frames(const char *arg)
+{
+    return parse_gnttab_limit("gnttab_max_maptrack_frames", arg,
+                              &opt_max_maptrack_frames);
+}
+custom_runtime_param("gnttab_max_maptrack_frames",
+                     parse_gnttab_max_maptrack_frames);
 
 #ifndef GNTTAB_MAX_VERSION
 #define GNTTAB_MAX_VERSION 2
@@ -1837,12 +1866,18 @@  active_alloc_failed:
     return -ENOMEM;
 }
 
-int grant_table_init(struct domain *d, unsigned int max_grant_frames,
-                     unsigned int max_maptrack_frames)
+int grant_table_init(struct domain *d, int max_grant_frames,
+                     int max_maptrack_frames)
 {
     struct grant_table *gt;
     int ret = -ENOMEM;
 
+    /* Default to maximum value if no value was specified */
+    if ( max_grant_frames < 0 )
+        max_grant_frames = opt_max_grant_frames;
+    if ( max_maptrack_frames < 0 )
+        max_maptrack_frames = opt_max_maptrack_frames;
+
     if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
          max_grant_frames > opt_max_grant_frames ||
          max_maptrack_frames > opt_max_maptrack_frames )
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 9f2cfd602c..e313da499f 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -82,13 +82,15 @@  struct xen_domctl_createdomain {
     uint32_t iommu_opts;
 
     /*
-     * Various domain limits, which impact the quantity of resources (global
-     * mapping space, xenheap, etc) a guest may consume.
+     * Various domain limits, which impact the quantity of resources
+     * (global mapping space, xenheap, etc) a guest may consume.  For
+     * max_grant_frames and max_maptrack_frames, < 0 means "use the
+     * default maximum value in the hypervisor".
      */
     uint32_t max_vcpus;
     uint32_t max_evtchn_port;
-    uint32_t max_grant_frames;
-    uint32_t max_maptrack_frames;
+    int32_t max_grant_frames;
+    int32_t max_maptrack_frames;
 
     struct xen_arch_domainconfig arch;
 };
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 6f9345d9ef..98603604b8 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -33,11 +33,10 @@ 
 struct grant_table;
 
 extern unsigned int opt_max_grant_frames;
-extern unsigned int opt_max_maptrack_frames;
 
 /* Create/destroy per-domain grant table context. */
-int grant_table_init(struct domain *d, unsigned int max_grant_frames,
-                     unsigned int max_maptrack_frames);
+int grant_table_init(struct domain *d, int max_grant_frames,
+                     int max_maptrack_frames);
 void grant_table_destroy(
     struct domain *d);
 void grant_table_init_vcpu(struct vcpu *v);
@@ -65,11 +64,10 @@  int gnttab_get_status_frame(struct domain *d, unsigned long idx,
 #else
 
 #define opt_max_grant_frames 0
-#define opt_max_maptrack_frames 0
 
 static inline int grant_table_init(struct domain *d,
-                                   unsigned int max_grant_frames,
-                                   unsigned int max_maptrack_frames)
+                                   int max_grant_frames,
+                                   int max_maptrack_frames)
 {
     return 0;
 }