Re: [PATCH v2 3/7] xen/evtchn: restrict the maximum number of evtchn supported for domUs

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,

> On 23 Aug 2022, at 1:48 pm, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Jan,
> 
> On 23/08/2022 12:35, Jan Beulich wrote:
>> On 23.08.2022 12:39, Rahul Singh wrote:
>>> Hi Jan,
>>> 
>>>> On 23 Aug 2022, at 9:29 am, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> 
>>>> On 23.08.2022 09:56, Julien Grall wrote:
>>>>> On 22/08/2022 14:49, Jan Beulich wrote:
>>>>>> On 19.08.2022 12:02, Rahul Singh wrote:
>>>>>>> --- a/xen/arch/arm/domain_build.c
>>>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>>>> @@ -3277,7 +3277,7 @@ void __init create_domUs(void)
>>>>>>>          struct xen_domctl_createdomain d_cfg = {
>>>>>>>              .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE,
>>>>>>>              .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
>>>>>>> -            .max_evtchn_port = -1,
>>>>>>> +            .max_evtchn_port = MAX_EVTCHNS_PORT,
>>>>>>>              .max_grant_frames = -1,
>>>>>>>              .max_maptrack_frames = -1,
>>>>>>>              .grant_opts = 
>>>>>>> XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
>>>>>>> --- a/xen/include/xen/sched.h
>>>>>>> +++ b/xen/include/xen/sched.h
>>>>>>> @@ -76,6 +76,9 @@ extern domid_t hardware_domid;
>>>>>>>  /* Maximum number of event channels for any ABI. */
>>>>>>>  #define MAX_NR_EVTCHNS MAX(EVTCHN_2L_NR_CHANNELS, 
>>>>>>> EVTCHN_FIFO_NR_CHANNELS)
>>>>>>> 
>>>>>>> +/* Maximum number of event channels supported for domUs. */
>>>>>>> +#define MAX_EVTCHNS_PORT 4096
>>>>>> 
>>>>>> I'm afraid the variable name doesn't express its purpose, and the
>>>>>> comment also claims wider applicability than is actually the case.
>>>>>> It's also not clear whether the constant really needs to live in
>>>>>> the already heavily overloaded xen/sched.h.
>>>>> 
>>>>> IMHO, I think the value would be better hardcoded with an explanation on
>>>>> top how we chose the default value.
>>>> 
>>>> Indeed that might be best, at least as long as no 2nd party appears.
>>>> What I was actually considering a valid reason for having a constant
>>>> in a header was the case of other arches also wanting to support
>>>> dom0less, at which point they likely ought to use the same value
>>>> without needing to duplicate any commentary or alike.
>>> 
>>> 
>>> If everyone is  okay I will modify the patch as below:
>> Well, I'm not an Arm maintainer, so my view might not matter, but
>> if this was a change to code I was a maintainer for, I'd object.
>> You enforce a limit here which you can't know whether it might
>> cause issues to anyone.
> 
> I understand the theory and in general I am not in favor of restricting a 
> limit without any data. However, here, I think we have all the data necessary 
> that would justify the limit.
> 
> In order to use event channels, a guest needs to know which PPI is used to 
> notify the guest.
> 
> Until recently, we didn't expose the node to dom0less domUs (this was 
> introduced when adding support for PV devices). So a guest couldn't discover 
> that event channels are used. That said, if the guest figured out the PPI 
> (the value can be guessed) then it could potentially use the event channels.
> 
> However, for Xen on Arm, we are not supporting any guest that don't use the 
> firmware tables (e.g. device tree/ACPI). So for such use case, I don't care 
> if it breaks because they are relying on unstable information.
> 
> What I care about is any user that follow the rules. We only started to 
> advertise Xen via Device-Tree to dom0less domUs after 4.16. So I think this 
> is fine to restrict the limit now because we haven't released 4.17 yet.
> 
> Regarding the default limit, I think it is better to stay consistent with 
> libxl and also use 1023. If an admin wants more event channels, then we could 
> introduce per-domain property to overwrite the default.
> 
> It should not be too difficult to add, but I don't think this is a must.
> So I will let Rahul to decide whether he has time to add it.

I prefer that we will first finish merging the ongoing event channel series.
I have created the task in our backlog, Arm will handle this task in the near 
future.

Regards,
Rahul