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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Nov 04, 2021 at 09:55:03AM +0100, Jan Beulich wrote:
> 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.

libxl API is stable, but not it's ABI, and libxl strictly depends and
uses domctl. See for example libxl__domain_make and it's usage of
xen_domctl_createdomain.

I think the usage here of XEN_DOMCTL_GRANT_version_mask is fine.

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

Hm, it could be done but doesn't seem to be common. AFAICT
parse_global_config just reads the values from the config file,
whether those values are sane doesn't seem to be implemented there.

> > @@ -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.)

Indeed, but that's not part of this change. I will post an updated
version of the grant disabling patch separately, as I don't think
that's 4.16 material.

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

Right, will add.

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

I think if the grant table is disabled at build time we should only
accept version 0 as valid here.

Thanks, Roger.