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

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxxx> 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.

Cheers,

--
Julien Grall