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 09:41:49 +0000	[thread overview]
Message-ID: <579C8A74-055D-445B-9955-750107DC80CF@arm.com> (raw)
In-Reply-To: <96618b21-7cb5-d160-75b3-953ccdc75ac5@xen.org>

Hi Julien,

> On 23 Aug 2022, at 8:56 am, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul and Jan,
> 
> On 22/08/2022 14:49, Jan Beulich wrote:
>> On 19.08.2022 12:02, Rahul Singh wrote:
>>> Static event channel support will be added for dom0less domains.
> 
> I am not sure how this sentence is related to this patch. You...
> 
>>> Restrict the maximum number of evtchn supported for domUs to avoid
>>> allocating a large amount of memory in Xen.
> 
> ... still need the limit to prevent a domain using more memory because at the moment they are unlimited.

Ok. I will remove the sentence.
> 
>> Please clarify here how you arrived at 4096 and why you expect no
>> dom0less DomU would ever want to have more. The limit, after all,
>> is far below that of FIFO event channels.
> 
> I will reply on this because I suggested the limit. A dom0less DomU is exactly the same as a DomU created by the toolstack. The default is 1023 (I originally thought it was 4096).
> 
> I would expect that is 1023 is going to be fine by default also for dom0less domU as on Arm we don't bind physical interrupts to event channels. So the only big use for them is for inter-domain communication.
> 

I will add this information in commit msg.

> Therefore, I think it should be ok to default to 1023 if we want consistency.
> 
> If someone needs more than 1023, we could introduce a per-domain device-tree property to override the default maximum.
> 
>>> --- 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.

Ack. 

Regards,
Rahul