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

From: Bertrand Marquis <Bertrand.Marquis@arm.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Rahul Singh <Rahul.Singh@arm.com>, Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>, Wei Liu <wl@xen.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 3/7] xen/evtchn: restrict the maximum number of evtchn supported for domUs
Date: Tue, 23 Aug 2022 11:44:20 +0000	[thread overview]
Message-ID: <B1DDE1F8-2906-40BF-B6A9-E18DDA883681@arm.com> (raw)
In-Reply-To: <fdfe8a77-34a3-252f-6aab-1850cc30c7a3@suse.com>

Hi Jan,

> On 23 Aug 2022, at 12:35, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 23.08.2022 12:39, Rahul Singh wrote:
>> Hi Jan,
>> 
>>> On 23 Aug 2022, at 9:29 am, Jan Beulich <jbeulich@suse.com> 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