Currently, we add grant mapping related device tree properties if the backend domain is not Dom0. While Dom0 is privileged and can do foreign mapping for the entire guest memory, it is still okay for Dom0 to access guest's memory via grant mappings and hence map only what is required.
This commit adds another parameter for virtio devices, with which they can do forced grant mappings irrespective of the backend domain id.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- docs/man/xl.cfg.5.pod.in | 4 ++++ tools/libs/light/libxl_arm.c | 21 ++++++++++++--------- tools/libs/light/libxl_types.idl | 1 + tools/libs/light/libxl_virtio.c | 11 +++++++++++ tools/xl/xl_parse.c | 2 ++ 5 files changed, 30 insertions(+), 9 deletions(-)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in index 10f37990be57..4879f136aab8 100644 --- a/docs/man/xl.cfg.5.pod.in +++ b/docs/man/xl.cfg.5.pod.in @@ -1616,6 +1616,10 @@ properties in the Device Tree, the type field must be set to "virtio,device". Specifies the transport mechanism for the Virtio device, only "mmio" is supported for now.
+=item B<forced_grant=BOOLEAN> + +Allows Xen Grant memory mapping to be done from Dom0. + =back
=item B<tee="STRING"> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c index 97c80d7ed0fa..ec2f1844e9b3 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 forced_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 (forced_grant || backend_domid != LIBXL_TOOLSTACK_DOMID) { 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 forced_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, forced_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 forced_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, forced_grant); if (res) return res;
/* Add device specific nodes */ @@ -1363,7 +1365,7 @@ 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, false) ); } }
@@ -1373,12 +1375,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 (virtio->forced_grant || virtio->backend_domid != LIBXL_TOOLSTACK_DOMID) iommu_needed = true;
FDT( make_virtio_mmio_node_device(gc, fdt, virtio->base, virtio->irq, virtio->type, - virtio->backend_domid) ); + virtio->backend_domid, + virtio->forced_grant) ); }
/* diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl index c10292e0d7e3..cfbcd717dc7f 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), + ("forced_grant", bool), ("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 faada49e184e..e1f15344ef97 100644 --- a/tools/libs/light/libxl_virtio.c +++ b/tools/libs/light/libxl_virtio.c @@ -48,11 +48,13 @@ static int libxl__set_xenstore_virtio(libxl__gc *gc, uint32_t domid, 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, "forced_grant", GCSPRINTF("%u", virtio->forced_grant));
flexarray_append_pair(front, "irq", GCSPRINTF("%u", virtio->irq)); flexarray_append_pair(front, "base", GCSPRINTF("%#"PRIx64, virtio->base)); flexarray_append_pair(front, "type", GCSPRINTF("%s", virtio->type)); flexarray_append_pair(front, "transport", GCSPRINTF("%s", transport)); + flexarray_append_pair(front, "forced_grant", GCSPRINTF("%u", virtio->forced_grant));
return 0; } @@ -104,6 +106,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/forced_grant", be_path), &tmp); + if (rc) goto out; + + if (tmp) { + virtio->forced_grant = 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..3e34da099785 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -1215,6 +1215,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("forced_grant", token, oparg)) { + virtio->forced_grant = strtoul(oparg, NULL, 0); } else { fprintf(stderr, "Unknown string "%s" in virtio spec\n", token); return -1;
On 30.03.23 11:43, Viresh Kumar wrote:
Hello Viresh
Currently, we add grant mapping related device tree properties if the backend domain is not Dom0. While Dom0 is privileged and can do foreign mapping for the entire guest memory, it is still okay for Dom0 to access guest's memory via grant mappings and hence map only what is required.
ok, probably makes sense
This commit adds another parameter for virtio devices, with which they can do forced grant mappings irrespective of the backend domain id.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
In general patch lgtm, just a few comments below
docs/man/xl.cfg.5.pod.in | 4 ++++ tools/libs/light/libxl_arm.c | 21 ++++++++++++--------- tools/libs/light/libxl_types.idl | 1 + tools/libs/light/libxl_virtio.c | 11 +++++++++++ tools/xl/xl_parse.c | 2 ++ 5 files changed, 30 insertions(+), 9 deletions(-)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in index 10f37990be57..4879f136aab8 100644 --- a/docs/man/xl.cfg.5.pod.in +++ b/docs/man/xl.cfg.5.pod.in @@ -1616,6 +1616,10 @@ properties in the Device Tree, the type field must be set to "virtio,device". Specifies the transport mechanism for the Virtio device, only "mmio" is supported for now. +=item B<forced_grant=BOOLEAN>
+Allows Xen Grant memory mapping to be done from Dom0.
Asumming it is disabled by default, I would add the following:
The default is (0) false.
- =falback
=item B<tee="STRING"> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c index 97c80d7ed0fa..ec2f1844e9b3 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 forced_grant)@@ -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 (forced_grant || backend_domid != LIBXL_TOOLSTACK_DOMID) { 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 forced_grant)
- res = make_virtio_mmio_node_common(gc, fdt, base, irq, backend_domid);
- res = make_virtio_mmio_node_common(gc, fdt, base, irq, backend_domid, forced_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 forced_grant)
- res = make_virtio_mmio_node_common(gc, fdt, base, irq, backend_domid);
- res = make_virtio_mmio_node_common(gc, fdt, base, irq, backend_domid, forced_grant); if (res) return res;
/* Add device specific nodes */ @@ -1363,7 +1365,7 @@ 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, false) ); } }@@ -1373,12 +1375,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 (virtio->forced_grant || virtio->backend_domid != LIBXL_TOOLSTACK_DOMID) iommu_needed = true;FDT( make_virtio_mmio_node_device(gc, fdt, virtio->base, virtio->irq, virtio->type,
virtio->backend_domid) );
virtio->backend_domid,virtio->forced_grant) ); }/* diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl index c10292e0d7e3..cfbcd717dc7f 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),
- ("forced_grant", bool),
If I remember correctly when making any updates here we also need to regenerate golang bindings.
("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 faada49e184e..e1f15344ef97 100644 --- a/tools/libs/light/libxl_virtio.c +++ b/tools/libs/light/libxl_virtio.c @@ -48,11 +48,13 @@ static int libxl__set_xenstore_virtio(libxl__gc *gc, uint32_t domid, 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, "forced_grant", GCSPRINTF("%u", virtio->forced_grant));
flexarray_append_pair(front, "irq", GCSPRINTF("%u", virtio->irq)); flexarray_append_pair(front, "base", GCSPRINTF("%#"PRIx64, virtio->base)); flexarray_append_pair(front, "type", GCSPRINTF("%s", virtio->type)); flexarray_append_pair(front, "transport", GCSPRINTF("%s", transport));
- flexarray_append_pair(front, "forced_grant", GCSPRINTF("%u", virtio->forced_grant));
return 0; } @@ -104,6 +106,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/forced_grant", be_path), &tmp);- if (rc) goto out;
- if (tmp) {
virtio->forced_grant = strtoul(tmp, NULL, 0);- }
I would add "else" case, something like:
{ LOG(DEBUG, "Missing xenstore node %s/forced_grant, assuming it is disabled", libxl_path); virtio->forced_grant = 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..3e34da099785 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -1215,6 +1215,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("forced_grant", token, oparg)) {
virtio->forced_grant = strtoul(oparg, NULL, 0); } else { fprintf(stderr, "Unknown string \"%s\" in virtio spec\n", token); return -1;
On 04-04-23, 21:16, Oleksandr Tyshchenko wrote:
ok, probably makes sense
While testing both foreign and grant mappings I stumbled upon another related problem. How do I control the creation of iommu node from guest configuration file, irrespective of the domain backend is running at ? This is what we have right now:
- always create iommu nodes if backend-dom != 0 - always create iommu nodes if forced_grant == 1
what I need to cover is - don't create iommu nodes irrespective of the domain
This is required if you want to test both foreign and grant memory allocations, with different guests kernels. i.e. one guest kernel for device with grant mappings and another guest for device with foreign mappings. There is no way, that I know of, to disable the creation of iommu nodes. Of course we would want to use the same images for kernel and other stuff, so this needs to be controlled from guest configuration file.
On 05-04-23, 05:12, Viresh Kumar wrote:
On 04-04-23, 21:16, Oleksandr Tyshchenko wrote:
ok, probably makes sense
While testing both foreign and grant mappings I stumbled upon another related problem. How do I control the creation of iommu node from guest configuration file, irrespective of the domain backend is running at ? This is what we have right now:
- always create iommu nodes if backend-dom != 0
- always create iommu nodes if forced_grant == 1
what I need to cover is
- don't create iommu nodes irrespective of the domain
This is required if you want to test both foreign and grant memory allocations, with different guests kernels. i.e. one guest kernel for device with grant mappings and another guest for device with foreign mappings. There is no way, that I know of, to disable the creation of iommu nodes. Of course we would want to use the same images for kernel and other stuff, so this needs to be controlled from guest configuration file.
Any input on this please ?
Hello Viresh
[sorry for the possible format issues]
On Fri, May 5, 2023 at 9:19 AM Viresh Kumar viresh.kumar@linaro.org wrote:
On 05-04-23, 05:12, Viresh Kumar wrote:
On 04-04-23, 21:16, Oleksandr Tyshchenko wrote:
ok, probably makes sense
While testing both foreign and grant mappings I stumbled upon another related problem. How do I control the creation of iommu node from guest configuration file, irrespective of the domain backend is running at ? This is what we have right now:
- always create iommu nodes if backend-dom != 0
- always create iommu nodes if forced_grant == 1
what I need to cover is
- don't create iommu nodes irrespective of the domain
This is required if you want to test both foreign and grant memory allocations, with different guests kernels. i.e. one guest kernel for device with grant mappings and another guest for device with foreign mappings. There is no way, that I know of, to disable the creation of iommu nodes. Of course we would want to use the same images for kernel and other stuff, so this needs to be controlled from guest configuration file.
Any input on this please ?
I was going to propose an idea, but I have just realized that you already voiced it here [1] )) So what you proposed there sounds reasonable to me.
I will just rephrase it according to my understanding:
We probably need to consider transforming your "forced_grant" to something three-state, for example "grant_usage" (or "use_grant" as you suggested) which could be "default behaviour" or "always disabled", or "always enabled".
With "grant_usage=default" we will get exact what we have at the moment (only create iommu nodes if backend-domid != 0) With "grant_usage=disabled" we will force grants to be always disabled (don't create iommu nodes irrespective of the domain) With "grant_usage=enabled" we will force grants to be always enabled (always create iommu nodes irrespective of the domain)
[1] https://lore.kernel.org/xen-devel/20230505093835.jcbwo6zjk5hcjvsm@vireshk-i7...
-- viresh
On 05-05-23, 16:11, Oleksandr Tyshchenko wrote:
I was going to propose an idea, but I have just realized that you already voiced it here [1] )) So what you proposed there sounds reasonable to me.
I will just rephrase it according to my understanding:
We probably need to consider transforming your "forced_grant" to something three-state, for example "grant_usage" (or "use_grant" as you suggested) which could be "default behaviour" or "always disabled", or "always enabled".
With "grant_usage=default" we will get exact what we have at the moment (only create iommu nodes if backend-domid != 0) With "grant_usage=disabled" we will force grants to be always disabled (don't create iommu nodes irrespective of the domain) With "grant_usage=enabled" we will force grants to be always enabled (always create iommu nodes irrespective of the domain)
I was wondering if "grant_usage=default" can be interpreted by the absence of the grant_usage parameter. So just grant_usage=1 or 0, which would mean enabled or disabled and if grant_usage isn't passed, we would switch back to default.
On Thu, Mar 30, 2023 at 02:13:08PM +0530, Viresh Kumar wrote:
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in index 10f37990be57..4879f136aab8 100644 --- a/docs/man/xl.cfg.5.pod.in +++ b/docs/man/xl.cfg.5.pod.in @@ -1616,6 +1616,10 @@ properties in the Device Tree, the type field must be set to "virtio,device". Specifies the transport mechanism for the Virtio device, only "mmio" is supported for now. +=item B<forced_grant=BOOLEAN>
+Allows Xen Grant memory mapping to be done from Dom0.
I think this description is missing context. I'm not sure what it would means "from dom0" without reading the patch. Also, it says "allows", maybe this doesn't convey the meaning of "forced". How about something like:
Always use grant mapping, even when the backend is run in dom0. (grant are already used if the backend is in another domain.)
=back =item B<tee="STRING"> diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c index faada49e184e..e1f15344ef97 100644 --- a/tools/libs/light/libxl_virtio.c +++ b/tools/libs/light/libxl_virtio.c @@ -48,11 +48,13 @@ static int libxl__set_xenstore_virtio(libxl__gc *gc, uint32_t domid, 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, "forced_grant", GCSPRINTF("%u", virtio->forced_grant));
flexarray_append_pair(front, "irq", GCSPRINTF("%u", virtio->irq)); flexarray_append_pair(front, "base", GCSPRINTF("%#"PRIx64, virtio->base)); flexarray_append_pair(front, "type", GCSPRINTF("%s", virtio->type)); flexarray_append_pair(front, "transport", GCSPRINTF("%s", transport));
- flexarray_append_pair(front, "forced_grant", GCSPRINTF("%u", virtio->forced_grant));
This "forced_grant" feels weird to me in the protocol, I feel like this use of grant or not could be handled by the backend. For example in "blkif" protocol, there's plenty of "feature-*" which allows both front-end and back-end to advertise which feature they can or want to use. But maybe the fact that the device tree needs to be modified to be able to accommodate grant mapping means that libxl needs to ask the backend to use grant or not, and the frontend needs to now if it needs to use grant.
return 0; } @@ -104,6 +106,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/forced_grant", be_path), &tmp);- if (rc) goto out;
- if (tmp) {
virtio->forced_grant = 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..3e34da099785 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -1215,6 +1215,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("forced_grant", token, oparg)) {
virtio->forced_grant = strtoul(oparg, NULL, 0);
Maybe store only !!strtoul() ? I don't think having values other that 0 or 1 is going to be good.
} else { fprintf(stderr, "Unknown string \"%s\" in virtio spec\n", token); return -1;
Thanks,
Hi Anthony,
On 02-05-23, 15:44, Anthony PERARD wrote:
diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c index faada49e184e..e1f15344ef97 100644 --- a/tools/libs/light/libxl_virtio.c +++ b/tools/libs/light/libxl_virtio.c @@ -48,11 +48,13 @@ static int libxl__set_xenstore_virtio(libxl__gc *gc, uint32_t domid, 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, "forced_grant", GCSPRINTF("%u", virtio->forced_grant));
flexarray_append_pair(front, "irq", GCSPRINTF("%u", virtio->irq)); flexarray_append_pair(front, "base", GCSPRINTF("%#"PRIx64, virtio->base)); flexarray_append_pair(front, "type", GCSPRINTF("%s", virtio->type)); flexarray_append_pair(front, "transport", GCSPRINTF("%s", transport));
- flexarray_append_pair(front, "forced_grant", GCSPRINTF("%u", virtio->forced_grant));
This "forced_grant" feels weird to me in the protocol, I feel like this use of grant or not could be handled by the backend. For example in "blkif" protocol, there's plenty of "feature-*" which allows both front-end and back-end to advertise which feature they can or want to use. But maybe the fact that the device tree needs to be modified to be able to accommodate grant mapping means that libxl needs to ask the backend to use grant or not, and the frontend needs to now if it needs to use grant.
I am not sure if I fully understand what you are suggesting here.
The eventual fronend drivers (like drivers/i2c/busses/i2c-virtio.c) aren't Xen aware and the respective virtio protocol doesn't talk about how memory is mapped for the guest. The guest kernel allows both memory mapping models and the decision is made based on the presence or absence of the iommu node in the DT.
The backends in our case are hypervisor agnostic and aren't part of Xen or any other hypervisor. I am not sure how the backend can provide the mapping information to Xen, with which the creation of the iommu DT node can be controlled.
Also, as I communicated in another email, the currently suggested option in this patch, "forced_grant", isn't enough for us. We also need a way to disable grant mappings. Right now we are creating iommu nodes by default all the time, if the backend domain isn't Dom0.
What I need probably is something like: "use_grant", where setting it to 1 will always create the iommu node and setting it to 0 will not, irrespective of the backend domain, and this overrides the current model of defaulting to create the node when not mapped by Dom0.
On Fri, May 05, 2023 at 03:08:35PM +0530, Viresh Kumar wrote:
Hi Anthony,
On 02-05-23, 15:44, Anthony PERARD wrote:
diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c index faada49e184e..e1f15344ef97 100644 --- a/tools/libs/light/libxl_virtio.c +++ b/tools/libs/light/libxl_virtio.c @@ -48,11 +48,13 @@ static int libxl__set_xenstore_virtio(libxl__gc *gc, uint32_t domid, 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, "forced_grant", GCSPRINTF("%u", virtio->forced_grant));
flexarray_append_pair(front, "irq", GCSPRINTF("%u", virtio->irq)); flexarray_append_pair(front, "base", GCSPRINTF("%#"PRIx64, virtio->base)); flexarray_append_pair(front, "type", GCSPRINTF("%s", virtio->type)); flexarray_append_pair(front, "transport", GCSPRINTF("%s", transport));
- flexarray_append_pair(front, "forced_grant", GCSPRINTF("%u", virtio->forced_grant));
This "forced_grant" feels weird to me in the protocol, I feel like this use of grant or not could be handled by the backend. For example in "blkif" protocol, there's plenty of "feature-*" which allows both front-end and back-end to advertise which feature they can or want to use. But maybe the fact that the device tree needs to be modified to be able to accommodate grant mapping means that libxl needs to ask the backend to use grant or not, and the frontend needs to now if it needs to use grant.
I am not sure if I fully understand what you are suggesting here.
I guess the way virtio devices are implemented in libxl suggest to me that the are just Xen PV devices. So I guess some documentation in the tree would be useful, maybe some comments in libxl_virtio.c.
The eventual fronend drivers (like drivers/i2c/busses/i2c-virtio.c) aren't Xen aware and the respective virtio protocol doesn't talk about how memory is mapped for the guest. The guest kernel allows both memory mapping models and the decision is made based on the presence or absence of the iommu node in the DT.
So, virtio's frontend don't know about xenstore? In this case, there's no need to have all those nodes in xenstore under the frontend path.
I guess the nodes for the backends are at least somewhat useful for libxl to reload the configuration of the virtio device. But even that isn't probably useful if we can't hot-plug or hot-unplug virtio devices.
Are the xenstore node for the backend actually been used by a virtio backend?
Cheers,
On 09-05-23, 16:05, Anthony PERARD wrote:
I guess the way virtio devices are implemented in libxl suggest to me that the are just Xen PV devices. So I guess some documentation in the tree would be useful, maybe some comments in libxl_virtio.c.
Our use case is just PV devices, not sure if someone else may want to use it differently.
I will add the comment.
The eventual fronend drivers (like drivers/i2c/busses/i2c-virtio.c) aren't Xen aware and the respective virtio protocol doesn't talk about how memory is mapped for the guest. The guest kernel allows both memory mapping models and the decision is made based on the presence or absence of the iommu node in the DT.
So, virtio's frontend don't know about xenstore?
Right.
In this case, there's no need to have all those nodes in xenstore under the frontend path.
Yeah, I ended up copying them from disk or kbd I guess, but I am not using them for sure.
I guess the nodes for the backends are at least somewhat useful for libxl to reload the configuration of the virtio device. But even that isn't probably useful if we can't hot-plug or hot-unplug virtio devices.
Are the xenstore node for the backend actually been used by a virtio backend?
Yes, I am using them on the backend side to read device information whenever a new guest comes in with a bunch of devices.
stratos-dev@op-lists.linaro.org
-
Anthony PERARD -
Oleksandr Tyshchenko -
Viresh Kumar