| Message ID | 20220128213307.2822078-1-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dom0less PV drivers | expand |
Hi Stefano, On 28/01/2022 21:33, Stefano Stabellini wrote: > From: Stefano Stabellini <stefano.stabellini@xilinx.com> > > Introduce a new "xen,enhanced" dom0less property to enable/disable PV > driver interfaces for dom0less guests. Currently only "enabled" and > "disabled" are supported property values (and empty). Leave the option > open to implement further possible values in the future (e.g. > "xenstore" to enable only xenstore.) > > The configurable option is for domUs only. For dom0 we always set the > corresponding property in the Xen code to true (PV interfaces enabled.) > > This patch only parses the property. Next patches will make use of it. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> > CC: Julien Grall <julien@xen.org> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > CC: Bertrand Marquis <bertrand.marquis@arm.com> > --- > Changes in v3: > - improve commit message > > Changes in v2: > - rename kinfo.enhanced to kinfo.dom0less_enhanced > - set kinfo.dom0less_enhanced to true for dom0 > - handle -ENODATA in addition to -EILSEQ > --- > docs/misc/arm/device-tree/booting.txt | 18 ++++++++++++++++++ > xen/arch/arm/domain_build.c | 8 ++++++++ > xen/arch/arm/include/asm/kernel.h | 3 +++ > 3 files changed, 29 insertions(+) > > diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt > index 71895663a4..38c29fb3d8 100644 > --- a/docs/misc/arm/device-tree/booting.txt > +++ b/docs/misc/arm/device-tree/booting.txt > @@ -169,6 +169,24 @@ with the following properties: > Please note that the SPI used for the virtual pl011 could clash with the > physical SPI of a physical device assigned to the guest. > > +- xen,enhanced NIT: I find a bit strange this is added in the middle of the property. Can you either sort the property alphabtically or move this one to the end? > + > + A string property. Possible property values are: > + > + - "enabled" (or missing property value) > + Xen PV interfaces, including grant-table and xenstore, will be > + enabled for the VM. > + > + - "disabled" > + Xen PV interfaces are disabled. > + > + If the xen,enhanced property is present with no value, it defaults > + to "enabled". If the xen,enhanced property is not present, PV > + interfaces are disabled. > + > + In the future other possible property values might be added to > + enable only selected interfaces. > + > - nr_spis > > Optional. A 32-bit integer specifying the number of SPIs (Shared > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 6931c022a2..9144d6c0b6 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -2963,6 +2963,7 @@ static int __init construct_domU(struct domain *d, > const struct dt_device_node *node) > { > struct kernel_info kinfo = {}; > + const char *dom0less_enhanced; > int rc; > u64 mem; > > @@ -2978,6 +2979,12 @@ static int __init construct_domU(struct domain *d, > > kinfo.vpl011 = dt_property_read_bool(node, "vpl011"); > > + rc = dt_property_read_string(node, "xen,enhanced", &dom0less_enhanced); > + if ( rc == -EILSEQ || I think the use an -EILSEQ wants an explanation. In a previous version, you wrote that the value would be returned when: fdt set /chosen/domU0 xen,enhanced But it is not clear why. Can you print pp->value, pp->length, strnlen(..) when this happens? > + rc == -ENODATA || > + (rc == 0 && !strcmp(dom0less_enhanced, "enabled")) ) > + kinfo.dom0less_enhanced = true; > + > if ( vcpu_create(d, 0) == NULL ) > return -ENOMEM; > > @@ -3095,6 +3102,7 @@ static int __init construct_dom0(struct domain *d) > > kinfo.unassigned_mem = dom0_mem; > kinfo.d = d; > + kinfo.dom0less_enhanced = true; This is a bit odd. The name suggests that this is a dom0less specific option. But then you are setting it to dom0. Given that this variable is about enable PV drivers, I think this should be false for dom0. > > rc = kernel_probe(&kinfo, NULL); > if ( rc < 0 ) > diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h > index 874aa108a7..c4dc039b54 100644 > --- a/xen/arch/arm/include/asm/kernel.h > +++ b/xen/arch/arm/include/asm/kernel.h > @@ -36,6 +36,9 @@ struct kernel_info { > /* Enable pl011 emulation */ > bool vpl011; > > + /* Enable PV drivers */ > + bool dom0less_enhanced; > + > /* GIC phandle */ > uint32_t phandle_gic; > Cheers,
On Sat, 29 Jan 2022, Julien Grall wrote: > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > index 6931c022a2..9144d6c0b6 100644 > > --- a/xen/arch/arm/domain_build.c > > +++ b/xen/arch/arm/domain_build.c > > @@ -2963,6 +2963,7 @@ static int __init construct_domU(struct domain *d, > > const struct dt_device_node *node) > > { > > struct kernel_info kinfo = {}; > > + const char *dom0less_enhanced; > > int rc; > > u64 mem; > > @@ -2978,6 +2979,12 @@ static int __init construct_domU(struct domain *d, > > kinfo.vpl011 = dt_property_read_bool(node, "vpl011"); > > + rc = dt_property_read_string(node, "xen,enhanced", > > &dom0less_enhanced); > > + if ( rc == -EILSEQ || > > I think the use an -EILSEQ wants an explanation. In a previous version, you > wrote that the value would be returned when: > > fdt set /chosen/domU0 xen,enhanced > > But it is not clear why. Can you print pp->value, pp->length, strnlen(..) when > this happens? I added in dt_property_read_string: printk("DEBUG %s %d value=%s value[0]=%d length=%u len=%lu\n",__func__,__LINE__,(char*)pp->value, *((char*)pp->value),pp->length, strlen(pp->value)); This is the output: (XEN) DEBUG dt_property_read_string 205 value= value[0]=0 length=0 len=0
Hi Stefano, On 23/03/2022 00:08, Stefano Stabellini wrote: > On Sat, 29 Jan 2022, Julien Grall wrote: >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index 6931c022a2..9144d6c0b6 100644 >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -2963,6 +2963,7 @@ static int __init construct_domU(struct domain *d, >>> const struct dt_device_node *node) >>> { >>> struct kernel_info kinfo = {}; >>> + const char *dom0less_enhanced; >>> int rc; >>> u64 mem; >>> @@ -2978,6 +2979,12 @@ static int __init construct_domU(struct domain *d, >>> kinfo.vpl011 = dt_property_read_bool(node, "vpl011"); >>> + rc = dt_property_read_string(node, "xen,enhanced", >>> &dom0less_enhanced); >>> + if ( rc == -EILSEQ || >> >> I think the use an -EILSEQ wants an explanation. In a previous version, you >> wrote that the value would be returned when: >> >> fdt set /chosen/domU0 xen,enhanced >> >> But it is not clear why. Can you print pp->value, pp->length, strnlen(..) when >> this happens? > > I added in dt_property_read_string: > > printk("DEBUG %s %d value=%s value[0]=%d length=%u len=%lu\n",__func__,__LINE__,(char*)pp->value, *((char*)pp->value),pp->length, strlen(pp->value)); > > This is the output: > (XEN) DEBUG dt_property_read_string 205 value= value[0]=0 length=0 len=0 Thanks posting the log! For convenience, I am copying the comment on top of dt_property_read_string() prototype: * Search for a property in a device tree node and retrieve a null * terminated string value (pointer to data, not a copy). Returns 0 on * success, -EINVAL if the property does not exist, -ENODATA if property * doest not have value, and -EILSEQ if the string is not * null-terminated with the length of the property data. Per your log, the length is NULL so I would have assumed -ENODATA would be returned. Looking at the implementation: const struct dt_property *pp = dt_find_property(np, propname, NULL); if ( !pp ) return -EINVAL; if ( !pp->value ) return -ENODATA; if ( strnlen(pp->value, pp->length) >= pp->length ) return -EILSEQ; We consider that the property when pp->value is NULL. However, AFAICT, we never set pp->value to NULL (see unflatten_dt_node()). So I think there is a bug in the implementation. I would keep the check !pp->value (for hardening purpose) and also return -ENODATA when !pp->length. Most of our device-tree code is from Linux. Looking at v5.17, the bug seems to be present there too. This would want to be fixed there too. Cheers,
On Fri, 25 Mar 2022, Julien Grall wrote: > On 23/03/2022 00:08, Stefano Stabellini wrote: > > On Sat, 29 Jan 2022, Julien Grall wrote: > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > > > index 6931c022a2..9144d6c0b6 100644 > > > > --- a/xen/arch/arm/domain_build.c > > > > +++ b/xen/arch/arm/domain_build.c > > > > @@ -2963,6 +2963,7 @@ static int __init construct_domU(struct domain *d, > > > > const struct dt_device_node *node) > > > > { > > > > struct kernel_info kinfo = {}; > > > > + const char *dom0less_enhanced; > > > > int rc; > > > > u64 mem; > > > > @@ -2978,6 +2979,12 @@ static int __init construct_domU(struct domain > > > > *d, > > > > kinfo.vpl011 = dt_property_read_bool(node, "vpl011"); > > > > + rc = dt_property_read_string(node, "xen,enhanced", > > > > &dom0less_enhanced); > > > > + if ( rc == -EILSEQ || > > > > > > I think the use an -EILSEQ wants an explanation. In a previous version, > > > you > > > wrote that the value would be returned when: > > > > > > fdt set /chosen/domU0 xen,enhanced > > > > > > But it is not clear why. Can you print pp->value, pp->length, strnlen(..) > > > when > > > this happens? > > > > I added in dt_property_read_string: > > > > printk("DEBUG %s %d value=%s value[0]=%d length=%u > > len=%lu\n",__func__,__LINE__,(char*)pp->value, > > *((char*)pp->value),pp->length, strlen(pp->value)); > > > > This is the output: > > (XEN) DEBUG dt_property_read_string 205 value= value[0]=0 length=0 len=0 > > Thanks posting the log! > > For convenience, I am copying the comment on top of dt_property_read_string() > prototype: > > * Search for a property in a device tree node and retrieve a null > * terminated string value (pointer to data, not a copy). Returns 0 on > * success, -EINVAL if the property does not exist, -ENODATA if property > * doest not have value, and -EILSEQ if the string is not > * null-terminated with the length of the property data. > > Per your log, the length is NULL so I would have assumed -ENODATA would be > returned. Looking at the implementation: > > const struct dt_property *pp = dt_find_property(np, propname, NULL); > > if ( !pp ) > return -EINVAL; > if ( !pp->value ) > return -ENODATA; > if ( strnlen(pp->value, pp->length) >= pp->length ) > return -EILSEQ; > > We consider that the property when pp->value is NULL. However, AFAICT, we > never set pp->value to NULL (see unflatten_dt_node()). > > So I think there is a bug in the implementation. I would keep the check > !pp->value (for hardening purpose) and also return -ENODATA when !pp->length. > > Most of our device-tree code is from Linux. Looking at v5.17, the bug seems to > be present there too. This would want to be fixed there too. I have added a patch to fix dt_property_read_string. I am about to send it out as patch of v4 of the series. I'll also follow-up on Linux. I am thinking of keeping the -EILSEQ in domain_build.c for hardening purposes.
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt index 71895663a4..38c29fb3d8 100644 --- a/docs/misc/arm/device-tree/booting.txt +++ b/docs/misc/arm/device-tree/booting.txt @@ -169,6 +169,24 @@ with the following properties: Please note that the SPI used for the virtual pl011 could clash with the physical SPI of a physical device assigned to the guest. +- xen,enhanced + + A string property. Possible property values are: + + - "enabled" (or missing property value) + Xen PV interfaces, including grant-table and xenstore, will be + enabled for the VM. + + - "disabled" + Xen PV interfaces are disabled. + + If the xen,enhanced property is present with no value, it defaults + to "enabled". If the xen,enhanced property is not present, PV + interfaces are disabled. + + In the future other possible property values might be added to + enable only selected interfaces. + - nr_spis Optional. A 32-bit integer specifying the number of SPIs (Shared diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 6931c022a2..9144d6c0b6 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -2963,6 +2963,7 @@ static int __init construct_domU(struct domain *d, const struct dt_device_node *node) { struct kernel_info kinfo = {}; + const char *dom0less_enhanced; int rc; u64 mem; @@ -2978,6 +2979,12 @@ static int __init construct_domU(struct domain *d, kinfo.vpl011 = dt_property_read_bool(node, "vpl011"); + rc = dt_property_read_string(node, "xen,enhanced", &dom0less_enhanced); + if ( rc == -EILSEQ || + rc == -ENODATA || + (rc == 0 && !strcmp(dom0less_enhanced, "enabled")) ) + kinfo.dom0less_enhanced = true; + if ( vcpu_create(d, 0) == NULL ) return -ENOMEM; @@ -3095,6 +3102,7 @@ static int __init construct_dom0(struct domain *d) kinfo.unassigned_mem = dom0_mem; kinfo.d = d; + kinfo.dom0less_enhanced = true; rc = kernel_probe(&kinfo, NULL); if ( rc < 0 ) diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h index 874aa108a7..c4dc039b54 100644 --- a/xen/arch/arm/include/asm/kernel.h +++ b/xen/arch/arm/include/asm/kernel.h @@ -36,6 +36,9 @@ struct kernel_info { /* Enable pl011 emulation */ bool vpl011; + /* Enable PV drivers */ + bool dom0less_enhanced; + /* GIC phandle */ uint32_t phandle_gic;