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

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

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).
> In practice xenstore requires both gnttab and evtchn, I don't know if we
> want to write that down in the device tree bindings. We could panic if
> the user specifies: xen,enhanced = "xenstore,evtchn";
I think the interface for dom0less domUs is quite messy at the moment. 
Even if we don't advertise the support for event channel and 
grant-table, hypercalls. They are still accessible if the guest wish to 
do so.
If we decide to introduce "evtchn", "gnttab" & co then we should also 
make sure that if "evtchn" is not specified then we are not allowing the 
guest to allocate any event channel (or map the grant-table).
Otherwise, this is pointless if we try to tell the user "evtchn", 
"gnttab"...
And just to be clear, I would be perfectly happy to break anyone trying
to use event channel without "xen,enanced" because we didn't advertise 
the feature. So they should not use it.
Cheers,

--
Julien Grall