Re: [PATCH v5 8/9] xen/arm: introduce legacy dom0less option for xenstore allocation

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 6 Feb 2025, Orzel, Michal wrote:
> On 06/02/2025 02:08, Stefano Stabellini wrote:
> > The new xenstore page allocation scheme might break older unpatches
> > Linux kernels that do not check for the Xenstore connection status
> > before proceeding with Xenstore initialization.
> > 
> > Introduce a dom0less configuration option to retain the older behavior,
> > which is not compatible with 1:1 mapped guests, but it will work with
> The issue is for static domains in general - not only for 1:1 guests.
> Static domains without direct map will simply fail on acquire_reserved_page().

I'll clarify


> > older legacy kernel versions.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> > ---
> >  docs/misc/arm/device-tree/booting.txt |  5 +++++
> >  xen/arch/arm/dom0less-build.c         | 13 ++++++++++++-
> >  xen/arch/arm/include/asm/kernel.h     | 14 +++++++++++---
> >  3 files changed, 28 insertions(+), 4 deletions(-)
> > 
> > diff --git a/docs/misc/arm/device-tree/booting.txt 
> > b/docs/misc/arm/device-tree/booting.txt
> > index ff70d44462..8fa3da95be 100644
> > --- a/docs/misc/arm/device-tree/booting.txt
> > +++ b/docs/misc/arm/device-tree/booting.txt
> > @@ -222,6 +222,11 @@ with the following properties:
> >      Xen PV interfaces, including grant-table and xenstore, will be
> >      enabled for the VM.
> >  
> > +    - "legacy"
> > +    Same as above, but the way the xenstore page is allocated is not
> > +    compatible with 1:1 mapped guests. On the other hand, it works with
> Same remark about 1:1
>
> > +    older Linux kernels.
> > +
> >      - "disabled"
> >      Xen PV interfaces are disabled.
> >  
> > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> > index 046439eb87..9afdbca8b8 100644
> > --- a/xen/arch/arm/dom0less-build.c
> > +++ b/xen/arch/arm/dom0less-build.c
> > @@ -799,6 +799,13 @@ static int __init construct_domU(struct domain *d,
> >          else
> >              panic("At the moment, Xenstore support requires dom0 to be 
> > present\n");
> >      }
> > +    else if ( rc == 0 && !strcmp(dom0less_enhanced, "legacy") )
> > +    {
> > +        if ( hardware_domain )
> > +            kinfo.dom0less_feature = DOM0LESS_ENHANCED_LEGACY;
> > +        else
> > +            panic("At the moment, Xenstore support requires dom0 to be 
> > present\n");
> > +    }
> >      else if ( rc == 0 && !strcmp(dom0less_enhanced, "no-xenstore") )
> >          kinfo.dom0less_feature = DOM0LESS_ENHANCED_NO_XS;
> >  
> > @@ -848,13 +855,17 @@ static int __init construct_domU(struct domain *d,
> >      if ( rc < 0 )
> >          return rc;
> >  
> > -    if ( kinfo.dom0less_feature & DOM0LESS_XENSTORE )
> > +    if ( kinfo.dom0less_feature & (DOM0LESS_XENSTORE|DOM0LESS_XS_LEGACY) )
> Spaces around | operator.

OK


> 
> >      {
> >          ASSERT(hardware_domain);
> >          rc = alloc_xenstore_evtchn(d);
> >          if ( rc < 0 )
> >              return rc;
> > +        d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL;
> > +    }
> >  
> > +    if ( kinfo.dom0less_feature & DOM0LESS_XENSTORE )
> > +    {
> Can I talk you into moving all of these into separate function e.g. 
> alloc_xenstore_params(struct kernel_info *kinfo)?
> It would simplify construct_domU() in which we tend to just call functions 
> responsible for a given functionality.

OK


> >          rc = alloc_xenstore_page(d);
> >          if ( rc < 0 )
> >              return rc;
> > diff --git a/xen/arch/arm/include/asm/kernel.h 
> > b/xen/arch/arm/include/asm/kernel.h
> > index de3f945ae5..4c2ae0b32b 100644
> > --- a/xen/arch/arm/include/asm/kernel.h
> > +++ b/xen/arch/arm/include/asm/kernel.h
> > @@ -17,16 +17,24 @@
> >   *                          default features (excluding Xenstore) will be
> >   *                          available. Note that an OS *must* not rely on 
> > the
> >   *                          availability of Xen features if this is not 
> > set.
> > - * DOM0LESS_XENSTORE:       Xenstore will be enabled for the VM. This 
> > feature
> > - *                          can't be enabled without the
> > - *                          DOM0LESS_ENHANCED_NO_XS.
> > + * DOM0LESS_XENSTORE:       Xenstore will be enabled for the VM. The
> > + *                          xenstore page allocation is done by Xen at
> > + *                          domain creation. This feature can't be
> > + *                          enabled without the DOM0LESS_ENHANCED_NO_XS.
> > + * DOM0LESS_XS_LEGACY       Xenstore will be enabled for the VM, the
> > + *                          xenstore page allocation will happen in
> > + *                          init-dom0less. This feature can't be enabled
> > + *                          without the DOM0LESS_ENHANCED_NO_XS.
> >   * DOM0LESS_ENHANCED:       Notify the OS it is running on top of Xen. All 
> > the
> >   *                          default features (including Xenstore) will be
> >   *                          available. Note that an OS *must* not rely on 
> > the
> >   *                          availability of Xen features if this is not 
> > set.
> > + * DOM0LESS_ENHANCED_LEGACY:Same as before, but using DOM0LESS_XS_LEGACY.
> NIT: I would just >> all text by one to have a space after :

OK


> >  #define DOM0LESS_ENHANCED_NO_XS  BIT(0, U)
> >  #define DOM0LESS_XENSTORE        BIT(1, U)
> > +#define DOM0LESS_XS_LEGACY       BIT(2, U)
> > +#define DOM0LESS_ENHANCED_LEGACY (DOM0LESS_ENHANCED_NO_XS | 
> > DOM0LESS_XS_LEGACY)
> >  #define DOM0LESS_ENHANCED        (DOM0LESS_ENHANCED_NO_XS | 
> > DOM0LESS_XENSTORE)
> >  
> >  struct kernel_info {
> 
> Otherwise, patch is ok.

Thanks for the review