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

[Julien Grall <julien@xxxxxxx>]

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.
> 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.
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.
Cheers,

--
Julien Grall