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
