Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,

> On 11 May 2022, at 2:11 pm, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Rahul,
> 
> On 11/05/2022 11:53, Rahul Singh wrote:
>>> On 11 May 2022, at 10:18 am, Bertrand Marquis <Bertrand.Marquis@xxxxxxx> 
>>> wrote:
>>> 
>>> Hi Julien,
>>> 
>>>> On 11 May 2022, at 10:10, Julien Grall <julien@xxxxxxx> wrote:
>>>> 
>>>> Hi Bertrand,
>>>> 
>>>> On 11/05/2022 09:46, Bertrand Marquis wrote:
>>>>>> On 11 May 2022, at 09:38, Julien Grall <julien@xxxxxxx> wrote:
>>>>>> 
>>>>>> Hi Bertrand,
>>>>>> 
>>>>>> On 11/05/2022 08:46, Bertrand Marquis wrote:
>>>>>>>> On 10 May 2022, at 17:35, Julien Grall <julien@xxxxxxx> wrote:
>>>>>>>> 
>>>>>>>> Hi Rahul,
>>>>>>>> 
>>>>>>>> On 10/05/2022 17:30, Rahul Singh wrote:
>>>>>>>>>> + rc = evtchn_alloc_unbound(&alloc);
>>>>>>>>>> + if ( rc )
>>>>>>>>>> + {
>>>>>>>>>> + printk("Failed allocating event channel for domain\n");
>>>>>>>>>> + return rc;
>>>>>>>>>> + }
>>>>>>>>>> +
>>>>>>>>>> + d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port;
>>>>>>>>>> +
>>>>>>>>>> + return 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> static int __init construct_domU(struct domain *d,
>>>>>>>>>> const struct dt_device_node *node)
>>>>>>>>>> {
>>>>>>>>>> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct 
>>>>>>>>>> domain *d,
>>>>>>>>>> if ( rc < 0 )
>>>>>>>>>> return rc;
>>>>>>>>>> 
>>>>>>>>>> + if ( kinfo.dom0less_enhanced )
>>>>>>>>> I think we need to do something like this to fix the error.
>>>>>>>>> if ( hardware_domain && kinfo.dom0less_enhanced )
>>>>>>>>> {
>>>>>>>>> }
>>>>>>>> 
>>>>>>>> Is there any use case to use "dom0less_enhanced" without dom0 (or a 
>>>>>>>> domain servicing Xenstored)?
>>>>>>>> 
>>>>>>> Just being curious here but would it even be possible to have non dom0 
>>>>>>> domain servicing xenstored ?
>>>>>> 
>>>>>> You can build Xenstored against mini-os and configure the init script to 
>>>>>> launch xenstored as a domain.
>>>>> So dom0 is not mandatory or should mini-os be started as Dom0 for this to 
>>>>> work ?
>>>> 
>>>> In order to allocate the event channel, you need to know the ID of the 
>>>> domain where Xenstored will run. Stefano's patch is relying on Xenstored 
>>>> to be run in Domain 0.
>>>> 
>>>> This would need to be updated if we want to run it in a separate domain.
>>> 
>>> Ok then Dom0 is mandatory at the moment, I am ok with that.
>>> 
>>>> 
>>>>>> 
>>>>>>>> If not, then I would consider to forbid this case and return an error.
>>>>>>> One way or an other we need to solve the crash but if it is forbidden 
>>>>>>> we must prevent coming to this step earlier as it means the 
>>>>>>> configuration is wrong.
>>>>>> 
>>>>>> I think this should be checked when parsing the configuration.
>>>>> If dom0 is mandatory yes, we should still make sure that this code cannot 
>>>>> be reached so an ASSERT would be nice here at least in case someone tries 
>>>>> to activate this code without dom0 (which might happen when we will push 
>>>>> the serie for static event channels).
>>>> 
>>>> I am fine with an ASSERT().
>>>> 
>>>> Are you saying that dom0less_enhanced will be set to true for the static 
>>>> event channel series?
>>>> 
>>>> If yes, then I think dom0less_enhanced will need to be an enum so we know 
>>>> what part of Xen is exposed.
>>> 
>>> No it won’t, we just need some of the changes done but without setting 
>>> dom0less_enhanced.
>>> @Rahul: can you confirm.
>>> 
>> We need to set the "xen,enhanced” enabled for dom0less domU to enable
>> the event-channel interface in dom0less guest. If we did not set this 
>> property we can’t
>> use the event-channel interface in dom0less domUs guests.
> 
> Is this because the domU will not know which PPI will be used for 
> notification?

Yes you are right if we don’t use "xen,enhanced” for domUs, domUs will not know 
which PPI will be used.
Also if we don’t use "xen,enhanced”  there is no hypervisor node created for 
Linux and if there is
no hypervisor node that means no xen support detected.

> 
> The property "xen,enhanced" with an empty string (or with the value 
> "enabled") is meant to indicate that PV drivers will be usable in the domain.
> 
> AFAIU, you are suggesting to change the meaning based on dom0 whether has 
> been created. I don't particularly like that because a user may spent a while 
> to understand why Xenstored doesn't work.
> 
> The current proposal for xen,enhanced allows us to define new values if we 
> wanted to only enabled selected interfaces. AFAIU, in your case, you only 
> want to expose the event channel interface, so I would create a new value to 
> indicate that the event channel interface is exposed. Xen would then create 
> only the part for the event channel (i.e. no extended regions, grant 
> tables...).

Ok. I will create the new property something like “xen,evtchn” to enable the 
event-channel for dom0less guests. 
Based on “xen,evtchn” property I will create the hypervisor node with only PPI 
interrupt property.

Regards,
Rahul