Re: [XEN PATCH 4/7] xen: introduce xen,enhanced dom0less property

[Stefano Stabellini <sstabellini@kernel.org>]

On Tue, 11 Jan 2022, Volodymyr Babchuk wrote:
> Hi Stefano,
> 
> Stefano Stabellini <sstabellini@kernel.org> writes:
> 
> > 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.)
> >
> > This patch only parses the property. Next patches will make use of it.
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > CC: Julien Grall <julien@xen.org>
> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> > CC: Bertrand Marquis <bertrand.marquis@arm.com>
> > ---
> >  docs/misc/arm/device-tree/booting.txt | 18 ++++++++++++++++++
> >  xen/arch/arm/domain_build.c           |  5 +++++
> >  xen/arch/arm/include/asm/kernel.h     |  3 +++
> >  3 files changed, 26 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..96a94fa434 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 *enhanced;
> >      int rc;
> >      u64 mem;
> >  
> > @@ -2978,6 +2979,10 @@ static int __init construct_domU(struct domain *d,
> >  
> >      kinfo.vpl011 = dt_property_read_bool(node, "vpl011");
> >  
> > +    rc = dt_property_read_string(node, "xen,enhanced", &enhanced);
> > +    if ( rc == -EILSEQ || (rc == 0 && !strcmp(enhanced, "enabled")) )
> 
> Are you sure you need to check for -EILSEQ?
> 
> >From documentation for dt_property_read_string:
> 
>  * 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.
> 
> So, for me it looks like you need to check for -ENODATA.

That's interesting, I ran a few tests with:

  fdt set /chosen/domU0 xen,enhanced

and I keep getting:

  (XEN) DEBUG construct_domU 3009 rc=-84 val=<NULL>

And -84 is -EILSEQ.

So I'll check for both -EILSEQ and -ENODATA to be safe.

 
> 
> > +        kinfo.enhanced = true;
> > +
> >      if ( vcpu_create(d, 0) == NULL )
> >          return -ENOMEM;
> >  
> > diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
> > index 874aa108a7..3275f7fbca 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 enhanced;
> 
> Maybe dom0less_enhanced will be better name?

I am OK with that and maybe you are right, it is clearer. I'll keep the
device tree option as "xen,enhanced" unless you meant to also change
that too.


> Or what about dom0? Should it have this option enabled?

Yes, I think it makes sense to set it to true for dom0 too for
consistency.