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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,

> On 23 Aug 2022, at 8:56 am, Julien Grall <julien@xxxxxxx> 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