| Message ID | 1a8c49dcc237187cbb9fccaafe1e6533fe68381c.1660902588.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/evtchn: implement static event channel signaling | expand |
On 19.08.2022 12:02, Rahul Singh wrote: > Static event channel support will be added for dom0less domains. > Restrict the maximum number of evtchn supported for domUs to avoid > allocating a large amount of memory in Xen. 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. > --- 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. Jan
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,
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. Jan
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
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: diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 3fd1186b53..fde133cd94 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -3277,7 +3277,13 @@ 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, + /* + * The default of 1023 should be sufficient for domUs guests + * because on ARM we don't bind physical interrupts to event + * channels. The only use of the evtchn port is inter-domain + * communications. + */ + .max_evtchn_port = 1023, .max_grant_frames = -1, .max_maptrack_frames = -1, .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version), Regards, Rahul
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. Jan > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 3fd1186b53..fde133cd94 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -3277,7 +3277,13 @@ 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, > + /* > + * The default of 1023 should be sufficient for domUs guests > + * because on ARM we don't bind physical interrupts to event > + * channels. The only use of the evtchn port is inter-domain > + * communications. > + */ > + .max_evtchn_port = 1023, > .max_grant_frames = -1, > .max_maptrack_frames = -1, > .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version), > > Regards, > Rahul >
Hi Jan, > On 23 Aug 2022, at 12:35, Jan Beulich <jbeulich@suse.com> 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. The limit was agreed and discussed between him and Julien and I agree with them (if any more views were required). Not quite sure if your mail was to request an other maintainer to confirm but done anyway. Bertrand > > Jan > >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index 3fd1186b53..fde133cd94 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -3277,7 +3277,13 @@ 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, >> + /* >> + * The default of 1023 should be sufficient for domUs guests >> + * because on ARM we don't bind physical interrupts to event >> + * channels. The only use of the evtchn port is inter-domain >> + * communications. >> + */ >> + .max_evtchn_port = 1023, >> .max_grant_frames = -1, >> .max_maptrack_frames = -1, >> .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version), >> >> Regards, >> Rahul
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. Cheers,
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
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 3fd1186b53..6d447367be 100644 --- 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), diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index e2b3b6daa3..e5cc4f3e3e 100644 --- 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 + #define EVTCHNS_PER_BUCKET (PAGE_SIZE / next_power_of_2(sizeof(struct evtchn))) #define EVTCHNS_PER_GROUP (BUCKETS_PER_GROUP * EVTCHNS_PER_BUCKET) #define NR_EVTCHN_GROUPS DIV_ROUND_UP(MAX_NR_EVTCHNS, EVTCHNS_PER_GROUP)
Static event channel support will be added for dom0less domains. Restrict the maximum number of evtchn supported for domUs to avoid allocating a large amount of memory in Xen. Signed-off-by: Rahul Singh <rahul.singh@arm.com> --- Changes in v2: - new patch in the version --- --- xen/arch/arm/domain_build.c | 2 +- xen/include/xen/sched.h | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-)