Re: [PATCH] libxl: Add "grant_usage" parameter for virtio disk devices

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Fri, Feb 02, 2024 at 12:49:03PM +0200, Oleksandr Tyshchenko wrote:
> diff --git a/tools/libs/util/libxlu_disk_l.l b/tools/libs/util/libxlu_disk_l.l
> index 6d53c093a3..f37dd443bd 100644
> --- a/tools/libs/util/libxlu_disk_l.l
> +++ b/tools/libs/util/libxlu_disk_l.l
> @@ -220,6 +220,9 @@ hidden-disk=[^,]*,?       { STRIP(','); 
> SAVESTRING("hidden-disk", hidden_disk, FROMEQU
>  trusted,?            { libxl_defbool_set(&DPC->disk->trusted, true); }
>  untrusted,?          { libxl_defbool_set(&DPC->disk->trusted, false); }
>  
> +grant_usage=1,?              { libxl_defbool_set(&DPC->disk->grant_usage, 
> true); }
> +grant_usage=0,?              { libxl_defbool_set(&DPC->disk->grant_usage, 
> false); }

For other boolean type for the disk, we have "trusted/untrusted",
"discard/no-discard", "direct-io-save/", but you are adding
"grant_usage=1/grant_usage=0". Is that fine? But I guess having the new
option spelled "grant_usage" might be better, so it match the other
virtio devices and the implementation. But maybe
"use-grant/no-use-grant" might be ok?

In any case, the implementation need to match the documentation, and
vice versa. See below.

> diff --git a/docs/man/xl-disk-configuration.5.pod.in 
> b/docs/man/xl-disk-configuration.5.pod.in
> index bc945cc517..3c035456d5 100644
> --- a/docs/man/xl-disk-configuration.5.pod.in
> +++ b/docs/man/xl-disk-configuration.5.pod.in
> @@ -404,6 +404,31 @@ Virtio frontend driver (virtio-blk) to be used. Please 
> note, the virtual
> +=item B<grant_usage=BOOLEAN>
>
> +=over 4
> +
> +=item Description
> +
> +Specifies the usage of Xen grants for accessing guest memory. Only applicable
> +to specification "virtio".
> +
> +=item Supported values
> +
> +If this option is B<true>, the Xen grants are always enabled.
> +If this option is B<false>, the Xen grants are always disabled.

Unfortunately, this is wrong, the implementation in the patch only
support two values: 1 / 0, nothing else, and trying to write "true" or
"false" would lead to an error. (Well actually it's "grant_usage=1" or
"grant_usage=0", there's nothing that cut that string at the '='.)

Also, do we really need the extra verbal description of each value here?
Is simply having the following would be enough?

    =item Supported values

    1, 0

The description in "Description" section would hopefully be enough.

> +=item Mandatory
> +
> +No
> +
> +=item Default value
> +
> +If this option is missing, then the default grant setting will be used,
> +i.e. enable grants if backend-domid != 0.
> +
> +=back
> +
> diff --git a/tools/libs/light/libxl_disk.c b/tools/libs/light/libxl_disk.c
> index ea3623dd6f..f39f427091 100644
> --- a/tools/libs/light/libxl_disk.c
> +++ b/tools/libs/light/libxl_disk.c
> @@ -181,6 +181,9 @@ static int libxl__device_disk_setdefault(libxl__gc *gc, 
> uint32_t domid,
>              return ERROR_INVAL;
>          }
>          disk->transport = LIBXL_DISK_TRANSPORT_MMIO;
> +
> +        libxl_defbool_setdefault(&disk->grant_usage,
> +                                 disk->backend_domid != 
> LIBXL_TOOLSTACK_DOMID);
>      }
>  
>      if (hotplug && disk->specification == LIBXL_DISK_SPECIFICATION_VIRTIO) {
> @@ -429,6 +432,8 @@ static void device_disk_add(libxl__egc *egc, uint32_t 
> domid,
>              flexarray_append(back, 
> libxl__device_disk_string_of_transport(disk->transport));
>              flexarray_append_pair(back, "base", GCSPRINTF("%"PRIu64, 
> disk->base));
>              flexarray_append_pair(back, "irq", GCSPRINTF("%u", disk->irq));
> +            flexarray_append_pair(back, "grant_usage",
> +                                  libxl_defbool_val(disk->grant_usage) ? "1" 
> : "0");
>          }
>  
>          flexarray_append(front, "backend-id");
> @@ -623,6 +628,14 @@ static int libxl__disk_from_xenstore(libxl__gc *gc, 
> const char *libxl_path,
>              goto cleanup;
>          }
>          disk->irq = strtoul(tmp, NULL, 10);
> +
> +        tmp = libxl__xs_read(gc, XBT_NULL,
> +                             GCSPRINTF("%s/grant_usage", libxl_path));
> +        if (!tmp) {
> +            LOG(ERROR, "Missing xenstore node %s/grant_usage", libxl_path);
> +            goto cleanup;

I wonder if it's such a good idea to make this node mandatory. Could we
just apply the default value if the path is missing? I don't think the
value is going to be used anyway because I don't think from_xenstore() is
used during guest creation, and it looks like "grant_usage" is only
useful during guest creation. Also, the "grant_usage" node isn't
mandatory in "libxl_virtio.c", so no need to do something different
for disk.

> +        }
> +        libxl_defbool_set(&disk->grant_usage, strtoul(tmp, NULL, 0));
>      }
>  
>      disk->vdev = xs_read(ctx->xsh, XBT_NULL,

Thanks,

-- 
Anthony PERARD