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(-)
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;
--
2.31.1.272.g89b43f80a514On 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,
--
Anthony PERARD
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.
--
viresh
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,
--
Anthony PERARD
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. -- viresh
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)
> {
> 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),
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. -- viresh
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 ? -- viresh
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 > -- Regards, Oleksandr Tyshchenko
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. -- viresh
On 08.05.23 06:09, Viresh Kumar wrote: > 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. I don't think this is a good idea. I think you'd need the 3rd state already in the interface between xl and libxl. Juergen
© 2016 - 2024 Red Hat, Inc.