Re: [XEN PATCH v2 1/5] xen: introduce xen,enhanced dom0less property

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 13 Jan 2022, Luca Fancellu wrote:
> > On 13 Jan 2022, at 00:58, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> > 
> > From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > 
> > 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.)
> > 
> > This patch only parses the property. Next patches will make use of it.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > CC: Julien Grall <julien@xxxxxxx>
> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> > CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> 
> Hi Stefano,
> 
> Subject to Bertrand’s comment on commit message:
> 
> Reviewed-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> 
> Just a small curiosity, why we use the prefix “xen,” for the property? I 
> thought since the node uses
> a “xen,domain" compatible we could use just “enhanced” just like the other 
> properties “vpl011”, “nr_spis”, ...

Julien was the one to suggest the naming but I actually like it.

Yes, if we wanted we could use "enhanced" but I think it is a better
idea to call this property "xen,enhanced" because that way it is easily
distinguishable from properties that could be hypervisor-neutral.

Imagine one day we have a device tree spec for a generic hypervisor
domain. It could have things like memory and even vpl011. However, for
sure it is not going to have "Xen PV drivers enabled". From a device
tree perspective it is better to align properties as much as possible
between similar nodes of different vendors, so ideally "memory" is going
ot be called "memory" for all hypervisor vendors. So it makes sense for
this property to have the "xen," prefix as it is different from the ones
that are more neutral.


> > ---
> > 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
> > +
> > +    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;
> > 
> > -- 
> > 2.25.1
> > 
> > 
>