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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 25 Aug 2022, Bertrand Marquis wrote:
> > On 25 Aug 2022, at 10:37, Julien Grall <julien@xxxxxxx> wrote:
> > 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).
> 
> Sorry wrong formulation, here I was meaning that we just need a solution to 
> disable Xenstore (should still be here by default when supported).
> 
> > 
> >>> 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?
> 
> Asking the user to specify gnttab in the list even though it is not supported 
> to not have it in the list.
> 
> > 
> >>> 
> >>> 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.

We have agreement on this, so I would say let's keep it simple and go
with this option.


> >> 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.
> 
> Right now Xenstore can only work with a dom0 and if someone wants to have an 
> other domain to provide it we would need a way to specify which one in the 
> configuration.
> So in a configuration without dom0, I still think that not enabling Xenstore 
> automatically is ok.
> 
> > 
> >> 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.
> 
> How about the following:
> Xen,enhanced: gnttab, events and Xenstore if there is a dom0
> Xen,enhanced = “[no-]xenstore,[no-]evtchn,[no-]gnttab” for when the user 
> wants to explicitly specify what he wants (and Xen stopping on unsupported 
> configuration).
>    In this I would allow to provide any combinations of the 3

I am OK with what you wrote as well, but considering the additional
complexity that no-gnttab and no-evtchn entail given that they cannot be
actually disabled today, I suggest to keep it simple and go with:

xen,enhanced = "no-xenstore"