Re: [PATCH] xen/grants: repurpose command line max options

[Jan Beulich <jbeulich@suse.com>]

On 13.03.2023 13:16, Roger Pau Monne wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1232,9 +1232,8 @@ The usage of gnttab v2 is not security supported on ARM platforms.
>  
>  > Can be modified at runtime
>  
> -Specify the maximum number of frames which any domain may use as part
> -of its grant table. This value is an upper boundary of the per-domain
> -value settable via Xen tools.
> +Specify the default maximum number of frames which any domain may use as part
> +of its grant table unless a different value is specified at domain creation.
>  
>  Dom0 is using this value for sizing its grant table.

dom0less DomU-s do as well, at the very least, also ...

> @@ -1245,9 +1244,10 @@ Dom0 is using this value for sizing its grant table.
>  
>  > Can be modified at runtime
>  
> -Specify the maximum number of frames to use as part of a domains
> -maptrack array. This value is an upper boundary of the per-domain
> -value settable via Xen tools.
> +Specify the default maximum number of frames to use as part of a domains
> +maptrack array unless a different value is specified at domain creation.
> +
> +Dom0 is using this value for sizing its maptrack array.

... here. And even ordinary DomU-s appear to default to that in the
absence of a specific value in the guest config. IOW at the very least
the info you add should not be misleading. Better would be if the pre-
existing info was adjusted at the same time.

I also wonder about the specific wording down here: While the max grant
table size can indeed be queried, this isn't the case for the maptrack
array. A domain also doesn't need to know its size, so maybe "This value
is used to size all domains' maptrack arrays, unless overridden by their
guest config"?

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1956,18 +1956,15 @@ int grant_table_init(struct domain *d, int max_grant_frames,
>          return -EINVAL;
>      }
>  
> -    /* Default to maximum value if no value was specified */
> +    /* Apply defaults 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 )
> +    if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES )
>      {
> -        dprintk(XENLOG_INFO, "Bad grant table sizes: grant %u, maptrack %u\n",
> -                max_grant_frames, max_maptrack_frames);
> +        dprintk(XENLOG_INFO, "Bad grant table size %u\n", max_grant_frames);
>          return -EINVAL;
>      }

I think I agree with the relaxation done here, but I also think this not
introducing security concerns wants spelling out in the description: My
understanding is that even in disaggregated environments we assume only
fully privileged entities can create domains.

Jan