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.
