[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Roger Pau Monne <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 4 Nov 2021 09:55:03 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=ShUlOTLU116Krk8rATLYXHFG9Lxg2GMVu6mfRYYR3XU=; b=HS7yGQmiURDvKIdtv91PIQNa4Eu7/2ZolTXWB/H/Z3VOfLPWtW4q8VtxEmDldg+vw3rAMMPWJKCdSBsQpP4NUpWM5KKmKoA50k9pXZEQbMyO9E9DCh7wL/PwJz0yZ/KMg46U7e/6b5GF4q9zwAk2iEakRJXD44w2X4bEYjlctK+X9zMA8f3hA6FLv3f70cWxTtkGDBmsjIzpft1eDDvC5QkmckLcpcEtaEw8h+gnYKnNCXVy1vPdcuTKMqQ90iju5Fzw3dYAMLyyajueyC9db13JUhcWykQs+cSTaqf6YvALQx5dguXtJNBqgkQFlVHf7+OrgJI3H6aaW0nUZKeJUQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=OTquDklSdtNjM2D9MelTC3a7qHrY+hHA2VurmF/g4iLosc4DkSLDnc/DrM9xVWHTpKQr3n98SA1GWCNj8m1YSzT/ETkqIoj5P0E7ucHhBiJpqsNunJpA4tkudKsgfkU7DDrAqCTF/Cs7MWMpSCM3oGLjzisUsngUB4waMhVE1mi6ZbD9JCiyO76ZQHZ/yGQWZKunlNpXnv269HlNXWjbCOF/6dChABfCsNMFY2/kliNUnORRU9s5kVs2HD9mtLJeuFOY+CvimHctbDQusVb6rqy+aASjezaunASv9IRW7M7JDBz3s96PHke3CX5m/tAPI7swQrNyMxlaoEt0njKBGA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Wei Liu <wl@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Anthony PERARD <anthony.perard@xxxxxxxxxx>, Juergen Gross <jgross@xxxxxxxx>, Christian Lindig <christian.lindig@xxxxxxxxxx>, David Scott <dave@xxxxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Thu, 04 Nov 2021 08:56:06 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.