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 3:57 pm, Rahul Singh <Rahul.Singh@xxxxxxx> wrote:
> 
> 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.

If we don’t want to create the new property we can use "xen,enhanced = evtchn” 
to 
enable the event-channel interfacefor dom0less guests.

Regards,
Rahul