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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jan,

> On 23 Aug 2022, at 12:35, Jan Beulich <jbeulich@xxxxxxxx> 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.

The limit was agreed and discussed between him and Julien and
I agree with them (if any more views were required).

Not quite sure if your mail was to request an other maintainer to
confirm but done anyway.


Bertrand


> 
> Jan
> 
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 3fd1186b53..fde133cd94 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -3277,7 +3277,13 @@ 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,
>> +            /*
>> +             * The default of 1023 should be sufficient for domUs guests
>> +             * because on ARM we don't bind physical interrupts to event
>> +             * channels. The only use of the evtchn port is inter-domain
>> +             * communications.
>> +             */
>> +            .max_evtchn_port = 1023,
>>             .max_grant_frames = -1,
>>             .max_maptrack_frames = -1,
>>             .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
>> 
>> Regards,
>> Rahul