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

From: Rahul Singh <Rahul.Singh@arm.com>
To: Julien Grall <julien@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>,
	Bertrand Marquis <Bertrand.Marquis@arm.com>,
	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 14:01:36 +0000	[thread overview]
Message-ID: <788C40AD-A1DA-4DD3-931E-01CCA72EA2C1@arm.com> (raw)
In-Reply-To: <caeb00e7-6239-1d9d-ae78-008830562239@xen.org>

Hi Julien,

> On 23 Aug 2022, at 1:48 pm, Julien Grall <julien@xen.org> 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@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.
> 
> 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