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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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
>