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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Stefano,

> On 25 Aug 2022, at 18:44, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> 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"

I agree with that.

Cheers
Bertrand