1. Patchew
  2. Xen
  3. View series

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

Oleksandr Tyshchenko posted 1 patch 2 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20240206123814.2308837-1-olekstysh@gmail.com
There is a newer version of this series
docs/man/xl-disk-configuration.5.pod.in | 24 ++++++++++++++++++++++++
tools/golang/xenlight/helpers.gen.go    |  6 ++++++
tools/golang/xenlight/types.gen.go      |  1 +
tools/include/libxl.h                   |  7 +++++++
tools/libs/light/libxl_arm.c            |  4 ++--
tools/libs/light/libxl_disk.c           | 14 ++++++++++++++
tools/libs/light/libxl_types.idl        |  1 +
tools/libs/util/libxlu_disk_l.l         |  3 +++
8 files changed, 58 insertions(+), 2 deletions(-)
[PATCH V2] libxl: Add "grant_usage" parameter for virtio disk devices
Posted by Oleksandr Tyshchenko 2 months, 3 weeks ago 
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Allow administrators to control whether Xen grant mappings for
the virtio disk devices should be used. By default (when new
parameter is not specified), the existing behavior is retained
(we enable grants if backend-domid != 0).

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
In addition to "libxl: arm: Add grant_usage parameter for virtio devices"
https://github.com/xen-project/xen/commit/c14254065ff4826e34f714e1790eab5217368c38

I wonder, whether I had to also include autogenerated changes to:
 - tools/libs/util/libxlu_disk_l.c
 - tools/libs/util/libxlu_disk_l.h

 V2:
  - clarify documentation to match the implementation
  - apply a default value if "grant_usage" is missing in the Xenstore
    in libxl__disk_from_xenstore()
---
 docs/man/xl-disk-configuration.5.pod.in | 24 ++++++++++++++++++++++++
 tools/golang/xenlight/helpers.gen.go    |  6 ++++++
 tools/golang/xenlight/types.gen.go      |  1 +
 tools/include/libxl.h                   |  7 +++++++
 tools/libs/light/libxl_arm.c            |  4 ++--
 tools/libs/light/libxl_disk.c           | 14 ++++++++++++++
 tools/libs/light/libxl_types.idl        |  1 +
 tools/libs/util/libxlu_disk_l.l         |  3 +++
 8 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/docs/man/xl-disk-configuration.5.pod.in b/docs/man/xl-disk-configuration.5.pod.in
index bc945cc517..55e78005cf 100644
--- a/docs/man/xl-disk-configuration.5.pod.in
+++ b/docs/man/xl-disk-configuration.5.pod.in
@@ -404,6 +404,30 @@ Virtio frontend driver (virtio-blk) to be used. Please note, the virtual
 device (vdev) is not passed to the guest in that case, but it still must be
 specified for the internal purposes.
 
+=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
+
+1, 0
+
+=item Mandatory
+
+No
+
+=item Default value
+
+If this option is missing, then the default grant setting will be used,
+i.e. "grant_usage=1" if backend-domid != 0 or "grant_usage=0" otherwise.
+
+=back
+
 =back
 
 =head1 COLO Parameters
diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index 35e209ff1b..768ab0f566 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -1879,6 +1879,9 @@ x.ActiveDisk = C.GoString(xc.active_disk)
 x.HiddenDisk = C.GoString(xc.hidden_disk)
 if err := x.Trusted.fromC(&xc.trusted);err != nil {
 return fmt.Errorf("converting field Trusted: %v", err)
+}
+if err := x.GrantUsage.fromC(&xc.grant_usage);err != nil {
+return fmt.Errorf("converting field GrantUsage: %v", err)
 }
 
  return nil}
@@ -1927,6 +1930,9 @@ if x.HiddenDisk != "" {
 xc.hidden_disk = C.CString(x.HiddenDisk)}
 if err := x.Trusted.toC(&xc.trusted); err != nil {
 return fmt.Errorf("converting field Trusted: %v", err)
+}
+if err := x.GrantUsage.toC(&xc.grant_usage); err != nil {
+return fmt.Errorf("converting field GrantUsage: %v", err)
 }
 
  return nil
diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
index 7907aa8999..0b712d2aa4 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -740,6 +740,7 @@ ColoExport string
 ActiveDisk string
 HiddenDisk string
 Trusted Defbool
+GrantUsage Defbool
 }
 
 type DeviceNic struct {
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index f1652b1664..2b69e08466 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -578,6 +578,13 @@
  */
 #define LIBXL_HAVE_DEVICE_DISK_SPECIFICATION 1
 
+/*
+ * LIBXL_HAVE_DISK_GRANT_USAGE indicates that the libxl_device_disk
+ * has 'grant_usage' field to specify the usage of Xen grants for
+ * the specification 'virtio'.
+ */
+#define LIBXL_HAVE_DISK_GRANT_USAGE 1
+
 /*
  * LIBXL_HAVE_CONSOLE_ADD_XENSTORE indicates presence of the function
  * libxl_console_add_xenstore() in libxl.
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 1539191774..1cb89fa584 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -1372,12 +1372,12 @@ next_resize:
             libxl_device_disk *disk = &d_config->disks[i];
 
             if (disk->specification == LIBXL_DISK_SPECIFICATION_VIRTIO) {
-                if (disk->backend_domid != LIBXL_TOOLSTACK_DOMID)
+                if (libxl_defbool_val(disk->grant_usage))
                     iommu_needed = true;
 
                 FDT( make_virtio_mmio_node(gc, fdt, disk->base, disk->irq,
                                            disk->backend_domid,
-                                           disk->backend_domid != LIBXL_TOOLSTACK_DOMID) );
+                                           libxl_defbool_val(disk->grant_usage)) );
             }
         }
 
diff --git a/tools/libs/light/libxl_disk.c b/tools/libs/light/libxl_disk.c
index ea3623dd6f..ed02b655a3 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,15 @@ 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(DEBUG, "Missing xenstore node %s/grant_usage, using default value", libxl_path);
+            libxl_defbool_set(&disk->grant_usage,
+                              disk->backend_domid != LIBXL_TOOLSTACK_DOMID);
+        } else
+            libxl_defbool_set(&disk->grant_usage, strtoul(tmp, NULL, 0));
     }
 
     disk->vdev = xs_read(ctx->xsh, XBT_NULL,
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 899ad30969..6d76f25528 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -803,6 +803,7 @@ libxl_device_disk = Struct("device_disk", [
     ("active_disk", string),
     ("hidden_disk", string),
     ("trusted", libxl_defbool),
+    ("grant_usage", libxl_defbool),
     ])
 
 libxl_device_nic = Struct("device_nic", [
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); }
+
  /* the target magic parameter, eats the rest of the string */
 
 target=.*	{ STRIP(','); SAVESTRING("target", pdev_path, FROMEQUALS); }
-- 
2.34.1
Re: [PATCH V2] libxl: Add "grant_usage" parameter for virtio disk devices
Posted by Anthony PERARD 2 months, 2 weeks ago 
On Tue, Feb 06, 2024 at 02:38:14PM +0200, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Allow administrators to control whether Xen grant mappings for
> the virtio disk devices should be used. By default (when new
> parameter is not specified), the existing behavior is retained
> (we enable grants if backend-domid != 0).
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
> In addition to "libxl: arm: Add grant_usage parameter for virtio devices"
> https://github.com/xen-project/xen/commit/c14254065ff4826e34f714e1790eab5217368c38
> 
> I wonder, whether I had to also include autogenerated changes to:
>  - tools/libs/util/libxlu_disk_l.c
>  - tools/libs/util/libxlu_disk_l.h

Well, that could be done on commit. The changes are going to be needed
to be committed as they may not be regenerated to include the new feature
in a build.

> ---
> diff --git a/tools/libs/light/libxl_disk.c b/tools/libs/light/libxl_disk.c
> index ea3623dd6f..ed02b655a3 100644
> --- a/tools/libs/light/libxl_disk.c
> +++ b/tools/libs/light/libxl_disk.c
> @@ -623,6 +628,15 @@ 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(DEBUG, "Missing xenstore node %s/grant_usage, using default value", libxl_path);

Is this information useful for debugging?

It should be easy to find out if the grant_usage node is present or not
by looking at xenstore, and I don't think libxl is going to make use of
that information after this point, so I don't think that's going to be
very useful.

> +            libxl_defbool_set(&disk->grant_usage,
> +                              disk->backend_domid != LIBXL_TOOLSTACK_DOMID);
> +        } else
> +            libxl_defbool_set(&disk->grant_usage, strtoul(tmp, NULL, 0));

Per coding style, it's better to have both side of an if..else to have
{}-block or none of them. So could you add a {} block in the else, or
remove the {} from the true side if we remove the LOG()?

Thanks,

-- 
Anthony PERARD
Re: [PATCH V2] libxl: Add "grant_usage" parameter for virtio disk devices
Posted by Oleksandr Tyshchenko 2 months, 2 weeks ago 

On 13.02.24 14:14, Anthony PERARD wrote:

Hello Anthony

> On Tue, Feb 06, 2024 at 02:38:14PM +0200, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Allow administrators to control whether Xen grant mappings for
>> the virtio disk devices should be used. By default (when new
>> parameter is not specified), the existing behavior is retained
>> (we enable grants if backend-domid != 0).
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>> In addition to "libxl: arm: Add grant_usage parameter for virtio devices"
>> https://urldefense.com/v3/__https://github.com/xen-project/xen/commit/c14254065ff4826e34f714e1790eab5217368c38__;!!GF_29dbcQIUBPA!172qt30uI7DpsZcK-IpZu25JoQScPeF7do_MD-KEqJlo8gaH-TV1P_H2CYfsGI0j_l0GdUPPO4BjyUD2Q86Lk2IlF5zCiFWMAw$ [github[.]com]
>>
>> I wonder, whether I had to also include autogenerated changes to:
>>   - tools/libs/util/libxlu_disk_l.c
>>   - tools/libs/util/libxlu_disk_l.h
> 
> Well, that could be done on commit. The changes are going to be needed
> to be committed as they may not be regenerated to include the new feature
> in a build.

Thanks. As V3 is needed anyway, I will include them.


> 
>> ---
>> diff --git a/tools/libs/light/libxl_disk.c b/tools/libs/light/libxl_disk.c
>> index ea3623dd6f..ed02b655a3 100644
>> --- a/tools/libs/light/libxl_disk.c
>> +++ b/tools/libs/light/libxl_disk.c
>> @@ -623,6 +628,15 @@ 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(DEBUG, "Missing xenstore node %s/grant_usage, using default value", libxl_path);
> 
> Is this information useful for debugging?
> 
> It should be easy to find out if the grant_usage node is present or not
> by looking at xenstore, and I don't think libxl is going to make use of
> that information after this point, so I don't think that's going to be
> very useful.

It is not very useful, will drop the log.


> 
>> +            libxl_defbool_set(&disk->grant_usage,
>> +                              disk->backend_domid != LIBXL_TOOLSTACK_DOMID);
>> +        } else
>> +            libxl_defbool_set(&disk->grant_usage, strtoul(tmp, NULL, 0));
> 
> Per coding style, it's better to have both side of an if..else to have
> {}-block or none of them. So could you add a {} block in the else, or
> remove the {} from the true side if we remove the LOG()?


Will do.

> 
> Thanks,
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.