Re: [PATCH v2 7/7] xen/arm: introduce new xen,enhanced property value

[Julien Grall <julien@xxxxxxx>]

On 25/08/2022 08:39, Bertrand Marquis wrote:
> Hi,
>
> > On 25 Aug 2022, at 02:10, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> >
> > On Wed, 24 Aug 2022, Julien Grall wrote:
> > > On 24/08/2022 22:59, Stefano Stabellini wrote:
> > > > On Wed, 24 Aug 2022, Rahul Singh wrote:
> > > > > > On 24 Aug 2022, at 4:36 pm, Julien Grall <julien@xxxxxxx> wrote:
> > > > > > On 24/08/2022 15:42, Rahul Singh wrote:
> > > > > > > > On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xxxxxxx> wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > On 24/08/2022 13:15, Rahul Singh wrote:
> > > > > > > > > Hi Julien,
> > > > > > > > Hi Rahul,
> > > > > > > >
> > > > > > > > > Please let me know your view on this.
> > > > > > > > > diff --git a/xen/arch/arm/domain_build.c
> > > > > > > > > b/xen/arch/arm/domain_build.c
> > > > > > > > > index bfe7bc6b36..a1e23eee59 100644
> > > > > > > > > --- a/xen/arch/arm/domain_build.c
> > > > > > > > > +++ b/xen/arch/arm/domain_build.c
> > > > > > > > > @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct
> > > > > > > > > domain *d,
> > > > > > > > >     if ( rc == -EILSEQ ||
> > > > > > > > >       rc == -ENODATA ||
> > > > > > > > >       (rc == 0 && !strcmp(dom0less_enhanced, “enabled”)) )
> > > > > > > > > -  {
> > > > > > > > > -    if ( hardware_domain )
> > > > > > > > >         kinfo.dom0less_enhanced = true;
> > > > > > > > > -    else
> > > > > > > > > -      panic(“Tried to use xen,enhanced without dom0\n”);
> > > > > > > > > -  }
> > > > > > > > You can't use "xen,enhanced" without dom0. In fact, you will end up
> > > > > > > > to dereference NULL in alloc_xenstore_evtchn(). That's because
> > > > > > > > "xen,enhanced" means the domain will be able to use Xenstored.
> > > > > > > >
> > > > > > > > Now if you want to support your feature without a dom0. Then I think
> > > > > > > > we want to introduce an option which would be the same as
> > > > > > > > "xen,enhanced" but doesn't expose Xenstored.
> > > > > > > If we modify the patch as below we can use the "xen,enhanced" for
> > > > > > > domUs without dom0.
> > > > > > > I tested the patch and its works fine. Do you see any issue with this
> > > > > > > approach?
> > > > > > Yes. For two reasons:
> > > > > > 1) It is muddying the meaning of "xen,enhanced". In particular a user
> > > > > > may not realize that Xenstore is not available if dom0 is not present.
> > > > > > 2) It would be more complicated to handle the case where Xenstored lives
> > > > > > in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet,
> > > > > > but I don't want to close the door.
> > > > > >
> > > > > > So if you want to support create "xen,xen" without all the rest. Then I
> > > > > > think we need a different property value. I don't have a good suggestion
> > > > > > for the name.
> > > > > Is that okay if we use the earlier approach, when user set  "xen,enhanced
> > > > > = evtchn” we will not call alloc_xenstore_evtchn()
> > > > > but we create hypervisor node with all fields.
> > > > Thinking more about this, today xen,enhanced has the implication that:
> > > >
> > > > - the guest will get a regular and complete "xen,xen" node in device tree
> > > > - xenstore and PV drivers will be available (full Xen interfaces support)
> > > >
> > > > We don't necessarely imply that dom0 is required (from a domU point of
> > > > view) but we do imply that xenstore+evtchn+gnttab will be available to
> > > > the domU.
> > > >
> > > > Now, static event channels are different. They don't require xenstore
> > > > and they don't require gnttab.
> > > >
> > > > It is as if the current xen,enhanced node actually meant:
> > > >
> > > >    xen,enhanced = "xenstore,gnttab,evtchn";
> > > Correct.
> > >
> > > > and now we are only enabling a subset:
> > > >
> > > >    xen,enhanced = "evtchn";
> > > >
> > > > Is that a correct understanding?
> > > Yes with some cavears (see below).
> > >
> > > > If so, we can clarify that:
> > > >
> > > >    xen,enhanced;
> > > >
> > > > it is a convenient shortend for:
> > > >
> > > >    xen,enhanced = "xenstore,gnttab,evtchn";
> > > >
> > > > and that other combinations are also acceptable, e.g.:
> > > >
> > > >    xen,enhanced = "gnttab";
> > > >    xen,enhanced = "evtchn";
> > > >    xen,enhanced = "evtchn,gnttab";
> > > >
> > > > It is OK to panic if the user specifies an option that is currently
> > > > unsupported (e.g. "gnttab").
> > > So today, if you create the node "xen,xen", the guest will expect to be able
> > > to use both grant-table and event channel.
> > >
> > > Therefore, in the list above, the only configuration we can sensibly support
> > > without any major rework is "evtchn,gnttab".
> > >
> > > If we want to support "evtchn" or "gnttab" only. Then we likely need to define
> > > a new binding (or new version) because neither "regs" nor "interrupts" are
> > > optional (although a guest OS is free to ignore them).
> > Yes I think you are right. I also broadly agree with the rest of your
> > reply.
> >
> > Thinking about it and given the above, we only need 2 "levels" of
> > enhancement:
> >
> > 1) everything: xenstore, gnttab, evtchn
> > 2) gnttab, evtchn, but not xenstore
> >
> > Nothing else is really possible because, as Julien pointed out,
> > "xen,enhanced" implies the xen,xen node in the domU device tree and in
> > turn that node implies both evtchn and gnttab.
> So we could say that xen,enhanced always includes gnttab and Xenstore is 
> optional.
Not really, Xenstore has always been part of the story in Xen. So I 
think making it optional for "xen,enhanced" is going to make more 
difficult for user to understand what the meaning of the option (in 
particular that in the future we may want to support Xenstored in a 
separate domain).
> > So I think we just need to add a way to express 2). We could do
> > something like:
> >
> >   xen,enhanced = "evtchn,gnttab";
> I am a bit puzzled here as gnttab is always there.
What do you mean?

> > Or we could use a new separate option like Julien initially suggested,
> > e.g.:
> >
> >   xen,enhanced-no-xenstore;
> >
> > "xen,enhanced-no-xenstore" is a terrible name actually, but just to
> > explain what I am thinking :-)
> I think most common use case will be to have all, so make sense to allow to 
> disable Xenstore.
>
> How about:
> xen,enhanced = “no-xenstore” ?
I would be fine with it.

> An other solution is to keep xen,enhanced as it is and introduce a new option:
> Xen,no-xenstore
I don't like the idea of introducing yet another option.

> At the end Xenstore cannot be used if there is no Dom0 and that we can detect 
> easily.
> Also there is no solution at this stage to have an other domain then Dom0 
> providing
> Xenstore (maybe in the long term someone will want to introduce that and we 
> will need
> a way to specify which domain is handling it).
>
> So I still think that we could just say that Xenstore can only be active if 
> there is a Dom0
> and just disable Xenstore automatically if it is not the case.
See above about disabling Xenstore automatically.

> If there is a dom0 and someone wants a guest without Xenstore, then we would 
> need to
> have the no-xenstore support.
> But is it a use case ?
Do you mean when "xen,enhanced" is specified? If yes, this could be 
useful if one want to limit the interface exposed to the guest.
> All in all, enhance dom0less was not supported before 4.17 so we will not 
> create any
> backward compatibility issue.
I agree we still have the flexibility to change.

Cheers,

--
Julien Grall