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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> 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.
> 
>> 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

Bertrand

> 
>> 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