Hi Stefano,
> On 25 Aug 2022, at 18:44, Stefano Stabellini <sstabell...@kernel.org> wrote:
>
> On Thu, 25 Aug 2022, Bertrand Marquis wrote:
>>> On 25 Aug 2022, at 10:37, Julien Grall <jul...@xen.org> wrote:
>>> On 25/08/2022 08:39, Bertrand Marquis wrote:
>>>> Hi,
>>>>> On 25 Aug 2022, at 02:10, Stefano Stabellini <sstabell...@kernel.org>
>>>>> 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 <jul...@xen.org> wrote:
>>>>>>>>> On 24/08/2022 15:42, Rahul Singh wrote:
>>>>>>>>>>> On 24 Aug 2022, at 1:59 pm, Julien Grall <jul...@xen.org> 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
