| Message ID | ccf5b1402fb7156be0ef33b44f7b114efbe76319.1683791298.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2,1/2] libxl: virtio: Remove unused frontend nodes | expand |
On Thu, May 11, 2023 at 01:20:43PM +0530, Viresh Kumar wrote: > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in > index 24ac92718288..0405f6efe62a 100644 > --- a/docs/man/xl.cfg.5.pod.in > +++ b/docs/man/xl.cfg.5.pod.in > @@ -1619,6 +1619,18 @@ hexadecimal format, without the "0x" prefix and all in lower case, like > Specifies the transport mechanism for the Virtio device, only "mmio" is > supported for now. > > +=item B<grant_usage=STRING> > + > +Specifies the grant usage details for the Virtio device. This can be set to > +following values: > + > +- "default": The default grant setting will be used, enable grants if > + backend-domid != 0. I don't think this "default" setting is useful. We could just describe what the default is when "grant_usage" setting is missing from the configuration. > +- "enabled": The grants are always enabled. > + > +- "disabled": The grants are always disabled. > diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl > index c10292e0d7e3..17228817f9b7 100644 > --- a/tools/libs/light/libxl_types.idl > +++ b/tools/libs/light/libxl_types.idl > @@ -283,6 +283,12 @@ libxl_virtio_transport = Enumeration("virtio_transport", [ > (1, "MMIO"), > ]) > > +libxl_virtio_grant_usage = Enumeration("virtio_grant_usage", [ > + (0, "DEFAULT"), > + (1, "DISABLED"), > + (2, "ENABLED"), libxl already provide this type, it's call "libxl_defbool". It can be set to "default", "false" or "true". > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c > index 97c80d7ed0fa..9cd7dbef0237 100644 > --- a/tools/libs/light/libxl_arm.c > +++ b/tools/libs/light/libxl_arm.c > @@ -1363,22 +1365,29 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_config *d_config, > iommu_needed = true; > > FDT( make_virtio_mmio_node(gc, fdt, disk->base, disk->irq, > - disk->backend_domid) ); > + disk->backend_domid, > + disk->backend_domid != LIBXL_TOOLSTACK_DOMID) ); > } > } > > for (i = 0; i < d_config->num_virtios; i++) { > libxl_device_virtio *virtio = &d_config->virtios[i]; > + bool use_grant = false; > > if (virtio->transport != LIBXL_VIRTIO_TRANSPORT_MMIO) > continue; > > - if (virtio->backend_domid != LIBXL_TOOLSTACK_DOMID) > + if ((virtio->grant_usage == LIBXL_VIRTIO_GRANT_USAGE_ENABLED) || > + ((virtio->grant_usage == LIBXL_VIRTIO_GRANT_USAGE_DEFAULT) && > + (virtio->backend_domid != LIBXL_TOOLSTACK_DOMID))) { I think libxl can select what the default value should be replace with before we start to setup the guest. There's a *_setdefault() phase were we set the correct value when a configuration value hasn't been set and thus a default value is used. I think this can be done in libxl__device_virtio_setdefault(). After that, virtio->grant_usage will be true or false, and that's the value that should be given to the virtio backend via xenstore. > diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c > index eadcb7124c3f..0a0fae967a0f 100644 > --- a/tools/libs/light/libxl_virtio.c > +++ b/tools/libs/light/libxl_virtio.c > @@ -46,11 +46,13 @@ static int libxl__set_xenstore_virtio(libxl__gc *gc, uint32_t domid, > flexarray_t *ro_front) > { > const char *transport = libxl_virtio_transport_to_string(virtio->transport); > + const char *grant_usage = libxl_virtio_grant_usage_to_string(virtio->grant_usage); > > flexarray_append_pair(back, "irq", GCSPRINTF("%u", virtio->irq)); > flexarray_append_pair(back, "base", GCSPRINTF("%#"PRIx64, virtio->base)); > flexarray_append_pair(back, "type", GCSPRINTF("%s", virtio->type)); > flexarray_append_pair(back, "transport", GCSPRINTF("%s", transport)); > + flexarray_append_pair(back, "grant_usage", GCSPRINTF("%s", grant_usage)); Currently, this mean that we store "default" in this node. That mean that both the virtio backend and libxl have to do computation in order to figure out if "default" mean "true" or "false". And both have to find the same result. I don't think this is necessary, and libxl can just tell enabled or disable. This would be done in libxl before we run this function. See previous comment on this patch. Thanks,
On 12-05-23, 11:43, Anthony PERARD wrote: > On Thu, May 11, 2023 at 01:20:43PM +0530, Viresh Kumar wrote: > > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in > > index 24ac92718288..0405f6efe62a 100644 > > --- a/docs/man/xl.cfg.5.pod.in > > +++ b/docs/man/xl.cfg.5.pod.in > > @@ -1619,6 +1619,18 @@ hexadecimal format, without the "0x" prefix and all in lower case, like > > Specifies the transport mechanism for the Virtio device, only "mmio" is > > supported for now. > > > > +=item B<grant_usage=STRING> > > + > > +Specifies the grant usage details for the Virtio device. This can be set to > > +following values: > > + > > +- "default": The default grant setting will be used, enable grants if > > + backend-domid != 0. > > I don't think this "default" setting is useful. We could just describe > what the default is when "grant_usage" setting is missing from the > configuration. This is what I suggested earlier [1], maybe I misunderstood what Juergen said. > > +- "enabled": The grants are always enabled. > > + > > +- "disabled": The grants are always disabled. > > > + if ((virtio->grant_usage == LIBXL_VIRTIO_GRANT_USAGE_ENABLED) || > > + ((virtio->grant_usage == LIBXL_VIRTIO_GRANT_USAGE_DEFAULT) && > > + (virtio->backend_domid != LIBXL_TOOLSTACK_DOMID))) { > > I think libxl can select what the default value should be replace with > before we start to setup the guest. There's a *_setdefault() phase were > we set the correct value when a configuration value hasn't been set and > thus a default value is used. I think this can be done in > libxl__device_virtio_setdefault(). > After that, virtio->grant_usage will be true or false, and that's the > value that should be given to the virtio backend via xenstore. I am not great with Xen's flow of stuff and so would like to clarify a few things here since I am confused. In my case, parse_virtio_config() gets called first followed by libxl__prepare_dtb(), where I need to use the "grant_usage" field. Later libxl__device_virtio_setdefault() gets called, anything done there isn't of much use in my case I guess. Setting the default value of grant_usage in libxl__device_virtio_setdefault() doesn't work for me (since libxl__prepare_dtb() is already called), and I need to set this in parse_virtio_config() only. Currently, virtio->backend_domid is getting set via libxl__resolve_domid() in libxl__device_virtio_setdefault(), which is too late for me, but is working fine, accidentally I think, since the default value of the field is 0, which is same as domain id in my case. I would like to understand though how it works for Disk device for Oleksandr, since they must also face similar issues. I must be doing something wrong here :) Lastly, libxl__virtio_from_xenstore() is never called in my case. Should I just remove it ? I copied it from some other implementation. This is how code looks for me currently, I am sure I need to fix it a bit though, based on reply to above questions. -------------------------8<------------------------- diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in index 24ac92718288..3a40ac8cb322 100644 --- a/docs/man/xl.cfg.5.pod.in +++ b/docs/man/xl.cfg.5.pod.in @@ -1619,6 +1619,14 @@ hexadecimal format, without the "0x" prefix and all in lower case, like Specifies the transport mechanism for the Virtio device, only "mmio" is supported for now. +=item B<grant_usage=BOOLEAN> + +If this option is B<true>, the Xen grants are always enabled. +If this option is B<false>, the Xen grants are always disabled. + +If this option is missing, then the default grant setting will be used, +i.e. enable grants if backend-domid != 0. + =back =item B<tee="STRING"> diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go index 0a203d22321f..bf846dca8ec0 100644 --- a/tools/golang/xenlight/helpers.gen.go +++ b/tools/golang/xenlight/helpers.gen.go @@ -1792,6 +1792,9 @@ func (x *DeviceVirtio) fromC(xc *C.libxl_device_virtio) error { x.BackendDomname = C.GoString(xc.backend_domname) x.Type = C.GoString(xc._type) x.Transport = VirtioTransport(xc.transport) +if err := x.GrantUsage.fromC(&xc.grant_usage);err != nil { +return fmt.Errorf("converting field GrantUsage: %v", err) +} x.Devid = Devid(xc.devid) x.Irq = uint32(xc.irq) x.Base = uint64(xc.base) @@ -1809,6 +1812,9 @@ xc.backend_domname = C.CString(x.BackendDomname)} if x.Type != "" { xc._type = C.CString(x.Type)} xc.transport = C.libxl_virtio_transport(x.Transport) +if err := x.GrantUsage.toC(&xc.grant_usage); err != nil { +return fmt.Errorf("converting field GrantUsage: %v", err) +} xc.devid = C.libxl_devid(x.Devid) xc.irq = C.uint32_t(x.Irq) xc.base = C.uint64_t(x.Base) diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go index a7c17699f80e..e0c6e91bb0ef 100644 --- a/tools/golang/xenlight/types.gen.go +++ b/tools/golang/xenlight/types.gen.go @@ -683,6 +683,7 @@ BackendDomid Domid BackendDomname string Type string Transport VirtioTransport +GrantUsage Defbool Devid Devid Irq uint32 Base uint64 diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c index 97c80d7ed0fa..bc2bd9649b95 100644 --- a/tools/libs/light/libxl_arm.c +++ b/tools/libs/light/libxl_arm.c @@ -922,7 +922,8 @@ static int make_xen_iommu_node(libxl__gc *gc, void *fdt) /* The caller is responsible to complete / close the fdt node */ static int make_virtio_mmio_node_common(libxl__gc *gc, void *fdt, uint64_t base, - uint32_t irq, uint32_t backend_domid) + uint32_t irq, uint32_t backend_domid, + bool grant_usage) { int res; gic_interrupt intr; @@ -945,7 +946,7 @@ static int make_virtio_mmio_node_common(libxl__gc *gc, void *fdt, uint64_t base, res = fdt_property(fdt, "dma-coherent", NULL, 0); if (res) return res; - if (backend_domid != LIBXL_TOOLSTACK_DOMID) { + if (grant_usage) { uint32_t iommus_prop[2]; iommus_prop[0] = cpu_to_fdt32(GUEST_PHANDLE_IOMMU); @@ -959,11 +960,12 @@ static int make_virtio_mmio_node_common(libxl__gc *gc, void *fdt, uint64_t base, } static int make_virtio_mmio_node(libxl__gc *gc, void *fdt, uint64_t base, - uint32_t irq, uint32_t backend_domid) + uint32_t irq, uint32_t backend_domid, + bool grant_usage) { int res; - res = make_virtio_mmio_node_common(gc, fdt, base, irq, backend_domid); + res = make_virtio_mmio_node_common(gc, fdt, base, irq, backend_domid, grant_usage); if (res) return res; return fdt_end_node(fdt); @@ -1019,11 +1021,11 @@ static int make_virtio_mmio_node_gpio(libxl__gc *gc, void *fdt) static int make_virtio_mmio_node_device(libxl__gc *gc, void *fdt, uint64_t base, uint32_t irq, const char *type, - uint32_t backend_domid) + uint32_t backend_domid, bool grant_usage) { int res; - res = make_virtio_mmio_node_common(gc, fdt, base, irq, backend_domid); + res = make_virtio_mmio_node_common(gc, fdt, base, irq, backend_domid, grant_usage); if (res) return res; /* Add device specific nodes */ @@ -1363,7 +1365,8 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_config *d_config, iommu_needed = true; FDT( make_virtio_mmio_node(gc, fdt, disk->base, disk->irq, - disk->backend_domid) ); + disk->backend_domid, + disk->backend_domid != LIBXL_TOOLSTACK_DOMID) ); } } @@ -1373,12 +1376,13 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_config *d_config, if (virtio->transport != LIBXL_VIRTIO_TRANSPORT_MMIO) continue; - if (virtio->backend_domid != LIBXL_TOOLSTACK_DOMID) + if (libxl_defbool_val(virtio->grant_usage)) iommu_needed = true; FDT( make_virtio_mmio_node_device(gc, fdt, virtio->base, virtio->irq, virtio->type, - virtio->backend_domid) ); + virtio->backend_domid, + libxl_defbool_val(virtio->grant_usage)) ); } /* diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl index c10292e0d7e3..c5c0d1f91793 100644 --- a/tools/libs/light/libxl_types.idl +++ b/tools/libs/light/libxl_types.idl @@ -740,6 +740,7 @@ libxl_device_virtio = Struct("device_virtio", [ ("backend_domname", string), ("type", string), ("transport", libxl_virtio_transport), + ("grant_usage", libxl_defbool), ("devid", libxl_devid), # Note that virtio-mmio parameters (irq and base) are for internal # use by libxl and can't be modified. diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c index f8a78e22d156..19d834984777 100644 --- a/tools/libs/light/libxl_virtio.c +++ b/tools/libs/light/libxl_virtio.c @@ -23,8 +23,16 @@ static int libxl__device_virtio_setdefault(libxl__gc *gc, uint32_t domid, libxl_device_virtio *virtio, bool hotplug) { - return libxl__resolve_domid(gc, virtio->backend_domname, - &virtio->backend_domid); + int rc; + + rc = libxl__resolve_domid(gc, virtio->backend_domname, + &virtio->backend_domid); + if (rc < 0) return rc; + + libxl_defbool_setdefault(&virtio->grant_usage, + virtio->backend_domid != LIBXL_TOOLSTACK_DOMID); + + return 0; } static int libxl__device_from_virtio(libxl__gc *gc, uint32_t domid, @@ -48,11 +56,13 @@ static int libxl__set_xenstore_virtio(libxl__gc *gc, uint32_t domid, flexarray_t *ro_front) { const char *transport = libxl_virtio_transport_to_string(virtio->transport); + const char *grant_usage = libxl_defbool_to_string(virtio->grant_usage); flexarray_append_pair(back, "irq", GCSPRINTF("%u", virtio->irq)); flexarray_append_pair(back, "base", GCSPRINTF("%#"PRIx64, virtio->base)); flexarray_append_pair(back, "type", GCSPRINTF("%s", virtio->type)); flexarray_append_pair(back, "transport", GCSPRINTF("%s", transport)); + flexarray_append_pair(back, "grant_usage", GCSPRINTF("%s", grant_usage)); return 0; } @@ -104,6 +114,15 @@ static int libxl__virtio_from_xenstore(libxl__gc *gc, const char *libxl_path, } } + tmp = NULL; + rc = libxl__xs_read_checked(gc, XBT_NULL, + GCSPRINTF("%s/grant_usage", be_path), &tmp); + if (rc) goto out; + + if (tmp) { + libxl_defbool_set(&virtio->grant_usage, strtoul(tmp, NULL, 0)); + } + tmp = NULL; rc = libxl__xs_read_checked(gc, XBT_NULL, GCSPRINTF("%s/type", be_path), &tmp); diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c index 1f6f47daf4e1..aa2bb214091e 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -1208,6 +1208,9 @@ static int parse_virtio_config(libxl_device_virtio *virtio, char *token) char *oparg; int rc; + libxl_defbool_setdefault(&virtio->grant_usage, + virtio->backend_domid != 0); + if (MATCH_OPTION("backend", token, oparg)) { virtio->backend_domname = strdup(oparg); } else if (MATCH_OPTION("type", token, oparg)) { @@ -1215,6 +1218,8 @@ static int parse_virtio_config(libxl_device_virtio *virtio, char *token) } else if (MATCH_OPTION("transport", token, oparg)) { rc = libxl_virtio_transport_from_string(oparg, &virtio->transport); if (rc) return rc; + } else if (MATCH_OPTION("grant_usage", token, oparg)) { + libxl_defbool_set(&virtio->grant_usage, strtoul(oparg, NULL, 0)); } else { fprintf(stderr, "Unknown string \"%s\" in virtio spec\n", token); return -1;
On 15.05.23 14:06, Viresh Kumar wrote: > On 12-05-23, 11:43, Anthony PERARD wrote: >> On Thu, May 11, 2023 at 01:20:43PM +0530, Viresh Kumar wrote: >>> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in >>> index 24ac92718288..0405f6efe62a 100644 >>> --- a/docs/man/xl.cfg.5.pod.in >>> +++ b/docs/man/xl.cfg.5.pod.in >>> @@ -1619,6 +1619,18 @@ hexadecimal format, without the "0x" prefix and all in lower case, like >>> Specifies the transport mechanism for the Virtio device, only "mmio" is >>> supported for now. >>> >>> +=item B<grant_usage=STRING> >>> + >>> +Specifies the grant usage details for the Virtio device. This can be set to >>> +following values: >>> + >>> +- "default": The default grant setting will be used, enable grants if >>> + backend-domid != 0. >> >> I don't think this "default" setting is useful. We could just describe >> what the default is when "grant_usage" setting is missing from the >> configuration. > > This is what I suggested earlier [1], maybe I misunderstood what > Juergen said. I think I just had another opinion. But the one of the maintainer is the one counting in this case IMO. :-) Juergen
On Mon, May 15, 2023 at 05:36:00PM +0530, Viresh Kumar wrote: > On 12-05-23, 11:43, Anthony PERARD wrote: > > On Thu, May 11, 2023 at 01:20:43PM +0530, Viresh Kumar wrote: > > > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in > > > index 24ac92718288..0405f6efe62a 100644 > > > --- a/docs/man/xl.cfg.5.pod.in > > > +++ b/docs/man/xl.cfg.5.pod.in > > > @@ -1619,6 +1619,18 @@ hexadecimal format, without the "0x" prefix and all in lower case, like > > > Specifies the transport mechanism for the Virtio device, only "mmio" is > > > supported for now. > > > > > > +=item B<grant_usage=STRING> > > > + > > > +Specifies the grant usage details for the Virtio device. This can be set to > > > +following values: > > > + > > > +- "default": The default grant setting will be used, enable grants if > > > + backend-domid != 0. > > > > I don't think this "default" setting is useful. We could just describe > > what the default is when "grant_usage" setting is missing from the > > configuration. > > This is what I suggested earlier [1], maybe I misunderstood what > Juergen said. To me, as a user of any program, setting default to a configuration option is when we don't select a configuration option. Like when starting a program for the first time and let it set things up based on the environment if that make senses. But I guess sometime when there's multiple choice, we can select default. Anyway, I've looked in the xl.cfg man page and there's already plenty of example where "default" is an option, so I guess it doesn't really hurt to have the option to choose to not choose. You still need to write in the man page that "default" is the default option, as in the absence of the option in the configuration the default option will be used (unless you managed somehow to make the option mandatory, but is they a reason for that?). In any case, there's going to be a 3-state option between xl and libxl which are going to be default, false, true. It doesn't matter whether a user can write default or not. > > > +- "enabled": The grants are always enabled. > > > + > > > +- "disabled": The grants are always disabled. > > > > > + if ((virtio->grant_usage == LIBXL_VIRTIO_GRANT_USAGE_ENABLED) || > > > + ((virtio->grant_usage == LIBXL_VIRTIO_GRANT_USAGE_DEFAULT) && > > > + (virtio->backend_domid != LIBXL_TOOLSTACK_DOMID))) { > > > > I think libxl can select what the default value should be replace with > > before we start to setup the guest. There's a *_setdefault() phase were > > we set the correct value when a configuration value hasn't been set and > > thus a default value is used. I think this can be done in > > libxl__device_virtio_setdefault(). > > After that, virtio->grant_usage will be true or false, and that's the > > value that should be given to the virtio backend via xenstore. > > I am not great with Xen's flow of stuff and so would like to clarify a > few things here since I am confused. > > In my case, parse_virtio_config() gets called first followed by > libxl__prepare_dtb(), where I need to use the "grant_usage" field. > Later libxl__device_virtio_setdefault() gets called, anything done > there isn't of much use in my case I guess. :-(, I feel like something is missing. I would think that libxl__prepare_dtb() would be called after any _setdefault() function. Maybe something isn't calling setdefault for virtio devices soon enough in libxl. > Setting the default value of grant_usage in > libxl__device_virtio_setdefault() doesn't work for me (since > libxl__prepare_dtb() is already called), and I need to set this in > parse_virtio_config() only. I don't think that `xl` should set any default, that would be better done in libxl. libxl could be used by another program, such as `libvirt`. > Currently, virtio->backend_domid is getting set via > libxl__resolve_domid() in libxl__device_virtio_setdefault(), which is > too late for me, but is working fine, accidentally I think, since the > default value of the field is 0, which is same as domain id in my > case. I would like to understand though how it works for Disk device > for Oleksandr, since they must also face similar issues. I must be > doing something wrong here :) No, I think something is missing for virtio devices. For disk, there's code in initiate_domain_create() which call libxl__disk_devtype.set_default() for every disk, and this happen before libxl__prepare_dtb(). I don't know how other device types are doing this defaulting, I need to search. There's also a special case for nic, a call of libxl__device_nic_set_devids() does call set_default for nics. Otherwise, I think set_default() is called whenever something calls add(). So, for virtio devices in libxl, I think we will also want to call set_default() early. Add a call to libxl__virtio_devtype.set_default() in libxl__domain_config_setdefault() similar to the one done for disk. (For disk, at the moment it is done in initiate_domain_create() but let's use the new libxl__domain_config_setdefault() function for virtio.) This means that libxl__device_virtio_setdefault() would be called twice for each device, but that shouldn't be an issue. Would that work? > Lastly, libxl__virtio_from_xenstore() is never called in my case. > Should I just remove it ? I copied it from some other implementation. I don't think from_xenstore() is normally called when creating a guest, but if we had an `xl` command called "virtio-list", like there's "block-list", then from_xenstore() would be use I think. It could also be use when doing *-detach, even if virtio doesn't have the option. Cheers,
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in index 24ac92718288..0405f6efe62a 100644 --- a/docs/man/xl.cfg.5.pod.in +++ b/docs/man/xl.cfg.5.pod.in @@ -1619,6 +1619,18 @@ hexadecimal format, without the "0x" prefix and all in lower case, like Specifies the transport mechanism for the Virtio device, only "mmio" is supported for now. +=item B<grant_usage=STRING> + +Specifies the grant usage details for the Virtio device. This can be set to +following values: + +- "default": The default grant setting will be used, enable grants if + backend-domid != 0. + +- "enabled": The grants are always enabled. + +- "disabled": The grants are always disabled. + =back =item B<tee="STRING"> diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go index 0a203d22321f..71d9c24e382c 100644 --- a/tools/golang/xenlight/helpers.gen.go +++ b/tools/golang/xenlight/helpers.gen.go @@ -1792,6 +1792,7 @@ func (x *DeviceVirtio) fromC(xc *C.libxl_device_virtio) error { x.BackendDomname = C.GoString(xc.backend_domname) x.Type = C.GoString(xc._type) x.Transport = VirtioTransport(xc.transport) +x.GrantUsage = VirtioGrantUsage(xc.grant_usage) x.Devid = Devid(xc.devid) x.Irq = uint32(xc.irq) x.Base = uint64(xc.base) @@ -1809,6 +1810,7 @@ xc.backend_domname = C.CString(x.BackendDomname)} if x.Type != "" { xc._type = C.CString(x.Type)} xc.transport = C.libxl_virtio_transport(x.Transport) +xc.grant_usage = C.libxl_virtio_grant_usage(x.GrantUsage) xc.devid = C.libxl_devid(x.Devid) xc.irq = C.uint32_t(x.Irq) xc.base = C.uint64_t(x.Base) diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go index a7c17699f80e..8f7234d3494a 100644 --- a/tools/golang/xenlight/types.gen.go +++ b/tools/golang/xenlight/types.gen.go @@ -261,6 +261,13 @@ VirtioTransportUnknown VirtioTransport = 0 VirtioTransportMmio VirtioTransport = 1 ) +type VirtioGrantUsage int +const( +VirtioGrantUsageDefault VirtioGrantUsage = 0 +VirtioGrantUsageDisabled VirtioGrantUsage = 1 +VirtioGrantUsageEnabled VirtioGrantUsage = 2 +) + type Passthrough int const( PassthroughDefault Passthrough = 0 @@ -683,6 +690,7 @@ BackendDomid Domid BackendDomname string Type string Transport VirtioTransport +GrantUsage VirtioGrantUsage Devid Devid Irq uint32 Base uint64 diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c index 97c80d7ed0fa..9cd7dbef0237 100644 --- a/tools/libs/light/libxl_arm.c +++ b/tools/libs/light/libxl_arm.c @@ -922,7 +922,8 @@ static int make_xen_iommu_node(libxl__gc *gc, void *fdt) /* The caller is responsible to complete / close the fdt node */ static int make_virtio_mmio_node_common(libxl__gc *gc, void *fdt, uint64_t base, - uint32_t irq, uint32_t backend_domid) + uint32_t irq, uint32_t backend_domid, + bool use_grant) { int res; gic_interrupt intr; @@ -945,7 +946,7 @@ static int make_virtio_mmio_node_common(libxl__gc *gc, void *fdt, uint64_t base, res = fdt_property(fdt, "dma-coherent", NULL, 0); if (res) return res; - if (backend_domid != LIBXL_TOOLSTACK_DOMID) { + if (use_grant) { uint32_t iommus_prop[2]; iommus_prop[0] = cpu_to_fdt32(GUEST_PHANDLE_IOMMU); @@ -959,11 +960,12 @@ static int make_virtio_mmio_node_common(libxl__gc *gc, void *fdt, uint64_t base, } static int make_virtio_mmio_node(libxl__gc *gc, void *fdt, uint64_t base, - uint32_t irq, uint32_t backend_domid) + uint32_t irq, uint32_t backend_domid, + bool use_grant) { int res; - res = make_virtio_mmio_node_common(gc, fdt, base, irq, backend_domid); + res = make_virtio_mmio_node_common(gc, fdt, base, irq, backend_domid, use_grant); if (res) return res; return fdt_end_node(fdt); @@ -1019,11 +1021,11 @@ static int make_virtio_mmio_node_gpio(libxl__gc *gc, void *fdt) static int make_virtio_mmio_node_device(libxl__gc *gc, void *fdt, uint64_t base, uint32_t irq, const char *type, - uint32_t backend_domid) + uint32_t backend_domid, bool use_grant) { int res; - res = make_virtio_mmio_node_common(gc, fdt, base, irq, backend_domid); + res = make_virtio_mmio_node_common(gc, fdt, base, irq, backend_domid, use_grant); if (res) return res; /* Add device specific nodes */ @@ -1363,22 +1365,29 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_config *d_config, iommu_needed = true; FDT( make_virtio_mmio_node(gc, fdt, disk->base, disk->irq, - disk->backend_domid) ); + disk->backend_domid, + disk->backend_domid != LIBXL_TOOLSTACK_DOMID) ); } } for (i = 0; i < d_config->num_virtios; i++) { libxl_device_virtio *virtio = &d_config->virtios[i]; + bool use_grant = false; if (virtio->transport != LIBXL_VIRTIO_TRANSPORT_MMIO) continue; - if (virtio->backend_domid != LIBXL_TOOLSTACK_DOMID) + if ((virtio->grant_usage == LIBXL_VIRTIO_GRANT_USAGE_ENABLED) || + ((virtio->grant_usage == LIBXL_VIRTIO_GRANT_USAGE_DEFAULT) && + (virtio->backend_domid != LIBXL_TOOLSTACK_DOMID))) { + use_grant = true; iommu_needed = true; + } FDT( make_virtio_mmio_node_device(gc, fdt, virtio->base, virtio->irq, virtio->type, - virtio->backend_domid) ); + virtio->backend_domid, + use_grant) ); } /* diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl index c10292e0d7e3..17228817f9b7 100644 --- a/tools/libs/light/libxl_types.idl +++ b/tools/libs/light/libxl_types.idl @@ -283,6 +283,12 @@ libxl_virtio_transport = Enumeration("virtio_transport", [ (1, "MMIO"), ]) +libxl_virtio_grant_usage = Enumeration("virtio_grant_usage", [ + (0, "DEFAULT"), + (1, "DISABLED"), + (2, "ENABLED"), + ]) + libxl_passthrough = Enumeration("passthrough", [ (0, "default"), (1, "disabled"), @@ -740,6 +746,7 @@ libxl_device_virtio = Struct("device_virtio", [ ("backend_domname", string), ("type", string), ("transport", libxl_virtio_transport), + ("grant_usage", libxl_virtio_grant_usage), ("devid", libxl_devid), # Note that virtio-mmio parameters (irq and base) are for internal # use by libxl and can't be modified. diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c index eadcb7124c3f..0a0fae967a0f 100644 --- a/tools/libs/light/libxl_virtio.c +++ b/tools/libs/light/libxl_virtio.c @@ -46,11 +46,13 @@ static int libxl__set_xenstore_virtio(libxl__gc *gc, uint32_t domid, flexarray_t *ro_front) { const char *transport = libxl_virtio_transport_to_string(virtio->transport); + const char *grant_usage = libxl_virtio_grant_usage_to_string(virtio->grant_usage); flexarray_append_pair(back, "irq", GCSPRINTF("%u", virtio->irq)); flexarray_append_pair(back, "base", GCSPRINTF("%#"PRIx64, virtio->base)); flexarray_append_pair(back, "type", GCSPRINTF("%s", virtio->type)); flexarray_append_pair(back, "transport", GCSPRINTF("%s", transport)); + flexarray_append_pair(back, "grant_usage", GCSPRINTF("%s", grant_usage)); return 0; } @@ -102,6 +104,23 @@ static int libxl__virtio_from_xenstore(libxl__gc *gc, const char *libxl_path, } } + tmp = NULL; + rc = libxl__xs_read_checked(gc, XBT_NULL, + GCSPRINTF("%s/grant_usage", be_path), &tmp); + if (rc) goto out; + + if (!tmp || !strcmp(tmp, "default")) { + virtio->grant_usage = LIBXL_VIRTIO_GRANT_USAGE_DEFAULT; + } else { + if (!strcmp(tmp, "enabled")) { + virtio->grant_usage = LIBXL_VIRTIO_GRANT_USAGE_ENABLED; + } else if (!strcmp(tmp, "disabled")) { + virtio->grant_usage = LIBXL_VIRTIO_GRANT_USAGE_DISABLED; + } else { + return ERROR_INVAL; + } + } + tmp = NULL; rc = libxl__xs_read_checked(gc, XBT_NULL, GCSPRINTF("%s/type", be_path), &tmp); diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c index 1f6f47daf4e1..d2de3abfcd78 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -1215,6 +1215,9 @@ static int parse_virtio_config(libxl_device_virtio *virtio, char *token) } else if (MATCH_OPTION("transport", token, oparg)) { rc = libxl_virtio_transport_from_string(oparg, &virtio->transport); if (rc) return rc; + } else if (MATCH_OPTION("grant_usage", token, oparg)) { + rc = libxl_virtio_grant_usage_from_string(oparg, &virtio->grant_usage); + if (rc) return rc; } else { fprintf(stderr, "Unknown string \"%s\" in virtio spec\n", token); return -1;
Currently, the grant mapping related device tree properties are added if the backend domain is not Dom0. While Dom0 is privileged and can do foreign mapping for the entire guest memory, it is still desired for Dom0 to access guest's memory via grant mappings and hence map only what is required. This commit adds the "grant_usage" parameter for virtio devices, which provides better control over the functionality. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- V1->V2: - Instead of just 0 or 1, the argument can take multiple values now and control the functionality in a better way. - Update .gen.go files as well. - Don't add nodes under frontend path. docs/man/xl.cfg.5.pod.in | 12 ++++++++++++ tools/golang/xenlight/helpers.gen.go | 2 ++ tools/golang/xenlight/types.gen.go | 8 ++++++++ tools/libs/light/libxl_arm.c | 27 ++++++++++++++++++--------- tools/libs/light/libxl_types.idl | 7 +++++++ tools/libs/light/libxl_virtio.c | 19 +++++++++++++++++++ tools/xl/xl_parse.c | 3 +++ 7 files changed, 69 insertions(+), 9 deletions(-)