| Message ID | 20240206123814.2308837-1-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V2] libxl: Add "grant_usage" parameter for virtio disk devices | expand |
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,
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, >
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); }