|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH V2 2/2] libxl: arm: Add grant_usage parameter for virtio devices
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;
--
viresh
[1] https://lore.kernel.org/all/016edde8-e47e-a988-e5c1-f5aad0d4b60d@xxxxxxxx/
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |