Re: [PATCH for-4.16 v5] gnttab: allow setting max version per-domain

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.11.2021 15:57, Roger Pau Monne wrote:
> --- a/tools/libs/light/libxl_create.c
> +++ b/tools/libs/light/libxl_create.c
> @@ -454,6 +454,28 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>          libxl_defbool_setdefault(&b_info->nested_hvm,               false);
>      }
>  
> +    if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
> +        libxl_physinfo info;
> +
> +        rc = libxl_get_physinfo(CTX, &info);
> +        if (rc) {
> +            LOG(ERROR, "failed to get hypervisor info");
> +            return rc;
> +        }
> +
> +        if (info.cap_gnttab_v2)
> +            b_info->max_grant_version = 2;
> +        else if (info.cap_gnttab_v1)
> +            b_info->max_grant_version = 1;
> +        else
> +            /* No grant table support reported */
> +            b_info->max_grant_version = 0;
> +    } else if (b_info->max_grant_version & ~XEN_DOMCTL_GRANT_version_mask) {

Aren't you introducing a dependency of a stable library on an unstable
interface here? I'm surprised this even builds, as I didn't expect
libxl sources to include domctl.h in the first place.

> @@ -219,6 +220,13 @@ static void parse_global_config(const char *configfile,
>      else if (e != ESRCH)
>          exit(1);
>  
> +    e = xlu_cfg_get_bounded_long (config, "max_grant_version", 0,
> +                                  INT_MAX, &l, 1);
> +    if (!e)
> +        max_grant_version = l;
> +    else if (e != ESRCH)
> +        exit(1);

Would be kind of nice if out-of-range values were detected and
reported right here, rather than causing puzzling errors at domain
creation. But I have no idea whether doing so would be inconsistent
with the processing of other global settings.

> @@ -1917,11 +1918,26 @@ active_alloc_failed:
>  }
>  
>  int grant_table_init(struct domain *d, int max_grant_frames,
> -                     int max_maptrack_frames)
> +                     int max_maptrack_frames, unsigned int options)
>  {
>      struct grant_table *gt;
> +    unsigned int max_grant_version = options & XEN_DOMCTL_GRANT_version_mask;
>      int ret = -ENOMEM;
>  
> +    if ( !max_grant_version )
> +    {
> +        dprintk(XENLOG_INFO, "%pd: invalid grant table version 0 
> requested\n",
> +                d);
> +        return -EINVAL;
> +    }

Wouldn't 0 rather be the most natural way to request no gnttab at all
for a domain? (Arguably making things work that way could be left to
a future change.)

> +    if ( max_grant_version > opt_gnttab_max_version )
> +    {
> +        dprintk(XENLOG_INFO,
> +                "%pd: requested grant version (%u) greater than supported 
> (%u)\n",
> +                d, max_grant_version, opt_gnttab_max_version);
> +        return -EINVAL;
> +    }
> +
>      /* Default to maximum value if no value was specified */
>      if ( max_grant_frames < 0 )
>          max_grant_frames = opt_max_grant_frames;

Neither here nor in domain_create() you check that the other bits of
"options" are zero.

> @@ -69,7 +69,8 @@ int gnttab_acquire_resource(
>  
>  static inline int grant_table_init(struct domain *d,
>                                     int max_grant_frames,
> -                                   int max_maptrack_frames)
> +                                   int max_maptrack_frames,
> +                                   unsigned int options)
>  {
>      return 0;
>  }

While arbitrary table size requests may be okay to ignore here, I'm
unsure about the max-version: Requesting v3 is surely an error in any
event; I'd even be inclined to suggest requesting v1 or v2 is as
well. Adding such a check here looks like it would be compatible with
all the other adjustments you're making.

Jan