| Message ID | 20250311090409.122577-2-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm: better handling of nr_spis | expand |
Hi Michal, > On 11 Mar 2025, at 10:04, Michal Orzel <michal.orzel@amd.com> wrote: > > At the moment, we print a warning about max number of IRQs supported by > GIC bigger than vGIC only for hardware domain. This check is not hwdom > special, and should be made common. Also, in case of user not specifying > nr_spis for dom0less domUs, we should take into account max number of > IRQs supported by vGIC if it's smaller than for GIC. > > Introduce VGIC_MAX_IRQS macro and use it instead of hardcoded 992 value. > Fix calculation of nr_spis for dom0less domUs and make the GIC/vGIC max > IRQs comparison common. > > Signed-off-by: Michal Orzel <michal.orzel@amd.com> > --- > xen/arch/arm/dom0less-build.c | 2 +- > xen/arch/arm/domain_build.c | 9 ++------- > xen/arch/arm/gic.c | 3 +++ > xen/arch/arm/include/asm/vgic.h | 3 +++ > 4 files changed, 9 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c > index 31f31c38da3f..9a84fee94119 100644 > --- a/xen/arch/arm/dom0less-build.c > +++ b/xen/arch/arm/dom0less-build.c > @@ -1018,7 +1018,7 @@ void __init create_domUs(void) > { > int vpl011_virq = GUEST_VPL011_SPI; > > - d_cfg.arch.nr_spis = gic_number_lines() - 32; > + d_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32; I would suggest to introduce a static inline gic_nr_spis in a gic header ... > > /* > * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map is > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 7cc141ef75e9..b99c4e3a69bf 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -2371,13 +2371,8 @@ void __init create_dom0(void) > > /* The vGIC for DOM0 is exactly emulating the hardware GIC */ > dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; > - /* > - * Xen vGIC supports a maximum of 992 interrupt lines. > - * 32 are substracted to cover local IRQs. > - */ > - dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; > - if ( gic_number_lines() > 992 ) > - printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n"); > + /* 32 are substracted to cover local IRQs */ > + dom0_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32; and reuse it here to make sure the value used is always the same. > dom0_cfg.arch.tee_type = tee_get_type(); > dom0_cfg.max_vcpus = dom0_max_vcpus(); > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index acf61a4de373..e80fe0ca2421 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -251,6 +251,9 @@ void __init gic_init(void) > panic("Failed to initialize the GIC drivers\n"); > /* Clear LR mask for cpu0 */ > clear_cpu_lr_mask(); > + > + if ( gic_number_lines() > VGIC_MAX_IRQS ) > + printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded\n"); I am a bit unsure with this one. If this is the case, it means any gicv2 or gicv3 init detected an impossible value and any usage of gic_number_lines would be using an impossible value. Shouldn't this somehow result in a panic as the gic detection was wrong ? Do you think we can continue to work safely if this value is over VGIC_MAX_IRQS. There are other places using gic_number_lines like in irq.c. Cheers Bertrand > } > > void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi) > diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h > index e309dca1ad01..c549e5840bfa 100644 > --- a/xen/arch/arm/include/asm/vgic.h > +++ b/xen/arch/arm/include/asm/vgic.h > @@ -329,6 +329,9 @@ extern void vgic_check_inflight_irqs_pending(struct vcpu *v, > */ > #define vgic_num_irqs(d) ((d)->arch.vgic.nr_spis + 32) > > +/* Maximum number of IRQs supported by vGIC */ > +#define VGIC_MAX_IRQS 992U > + > /* > * Allocate a guest VIRQ > * - spi == 0 => allocate a PPI. It will be the same on every vCPU > -- > 2.25.1 >
On 11/03/2025 10:30, Bertrand Marquis wrote: > > > Hi Michal, > >> On 11 Mar 2025, at 10:04, Michal Orzel <michal.orzel@amd.com> wrote: >> >> At the moment, we print a warning about max number of IRQs supported by >> GIC bigger than vGIC only for hardware domain. This check is not hwdom >> special, and should be made common. Also, in case of user not specifying >> nr_spis for dom0less domUs, we should take into account max number of >> IRQs supported by vGIC if it's smaller than for GIC. >> >> Introduce VGIC_MAX_IRQS macro and use it instead of hardcoded 992 value. >> Fix calculation of nr_spis for dom0less domUs and make the GIC/vGIC max >> IRQs comparison common. >> >> Signed-off-by: Michal Orzel <michal.orzel@amd.com> >> --- >> xen/arch/arm/dom0less-build.c | 2 +- >> xen/arch/arm/domain_build.c | 9 ++------- >> xen/arch/arm/gic.c | 3 +++ >> xen/arch/arm/include/asm/vgic.h | 3 +++ >> 4 files changed, 9 insertions(+), 8 deletions(-) >> >> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c >> index 31f31c38da3f..9a84fee94119 100644 >> --- a/xen/arch/arm/dom0less-build.c >> +++ b/xen/arch/arm/dom0less-build.c >> @@ -1018,7 +1018,7 @@ void __init create_domUs(void) >> { >> int vpl011_virq = GUEST_VPL011_SPI; >> >> - d_cfg.arch.nr_spis = gic_number_lines() - 32; >> + d_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32; > > I would suggest to introduce a static inline gic_nr_spis in a gic header ... Why GIC and not vGIC? This is domain's nr_spis, so vGIC. But then, why static inline if the value does not change and is domain agnostic? I struggle to find a good name for this macro. Maybe (in vgic.h): #define vgic_def_nr_spis (min(gic_number_lines(), VGIC_MAX_IRQS) - 32) to denote default nr_spis if not set by the user? > >> >> /* >> * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map is >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index 7cc141ef75e9..b99c4e3a69bf 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -2371,13 +2371,8 @@ void __init create_dom0(void) >> >> /* The vGIC for DOM0 is exactly emulating the hardware GIC */ >> dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; >> - /* >> - * Xen vGIC supports a maximum of 992 interrupt lines. >> - * 32 are substracted to cover local IRQs. >> - */ >> - dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; >> - if ( gic_number_lines() > 992 ) >> - printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n"); >> + /* 32 are substracted to cover local IRQs */ >> + dom0_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32; > > and reuse it here to make sure the value used is always the same. > >> dom0_cfg.arch.tee_type = tee_get_type(); >> dom0_cfg.max_vcpus = dom0_max_vcpus(); >> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index acf61a4de373..e80fe0ca2421 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -251,6 +251,9 @@ void __init gic_init(void) >> panic("Failed to initialize the GIC drivers\n"); >> /* Clear LR mask for cpu0 */ >> clear_cpu_lr_mask(); >> + >> + if ( gic_number_lines() > VGIC_MAX_IRQS ) >> + printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded\n"); > > I am a bit unsure with this one. > If this is the case, it means any gicv2 or gicv3 init detected an impossible value and > any usage of gic_number_lines would be using an impossible value. Why impossible? GIC can support up to 1020 IRQs. Our vGIC can support up to 992 IRQs. > > Shouldn't this somehow result in a panic as the gic detection was wrong ? > Do you think we can continue to work safely if this value is over VGIC_MAX_IRQS. > There are other places using gic_number_lines like in irq.c. I can add a panic, sure. This would be a change in behavior because we used this check for hwdom unconditionally. I'd need others opinion for this one. ~Michal
> On 11 Mar 2025, at 10:59, Orzel, Michal <Michal.Orzel@amd.com> wrote: > > > > On 11/03/2025 10:30, Bertrand Marquis wrote: >> >> >> Hi Michal, >> >>> On 11 Mar 2025, at 10:04, Michal Orzel <michal.orzel@amd.com> wrote: >>> >>> At the moment, we print a warning about max number of IRQs supported by >>> GIC bigger than vGIC only for hardware domain. This check is not hwdom >>> special, and should be made common. Also, in case of user not specifying >>> nr_spis for dom0less domUs, we should take into account max number of >>> IRQs supported by vGIC if it's smaller than for GIC. >>> >>> Introduce VGIC_MAX_IRQS macro and use it instead of hardcoded 992 value. >>> Fix calculation of nr_spis for dom0less domUs and make the GIC/vGIC max >>> IRQs comparison common. >>> >>> Signed-off-by: Michal Orzel <michal.orzel@amd.com> >>> --- >>> xen/arch/arm/dom0less-build.c | 2 +- >>> xen/arch/arm/domain_build.c | 9 ++------- >>> xen/arch/arm/gic.c | 3 +++ >>> xen/arch/arm/include/asm/vgic.h | 3 +++ >>> 4 files changed, 9 insertions(+), 8 deletions(-) >>> >>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c >>> index 31f31c38da3f..9a84fee94119 100644 >>> --- a/xen/arch/arm/dom0less-build.c >>> +++ b/xen/arch/arm/dom0less-build.c >>> @@ -1018,7 +1018,7 @@ void __init create_domUs(void) >>> { >>> int vpl011_virq = GUEST_VPL011_SPI; >>> >>> - d_cfg.arch.nr_spis = gic_number_lines() - 32; >>> + d_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32; >> >> I would suggest to introduce a static inline gic_nr_spis in a gic header ... > Why GIC and not vGIC? This is domain's nr_spis, so vGIC. yes vGIC sorry. > But then, why static inline if the value does not change and is domain agnostic? > I struggle to find a good name for this macro. Maybe (in vgic.h): > #define vgic_def_nr_spis (min(gic_number_lines(), VGIC_MAX_IRQS) - 32) > to denote default nr_spis if not set by the user? Yes that would work. My point is to prevent to have 2 definitions in 2 different source file and a risk to forget to update one and not the other (let say if some day we change 32 in 64). > >> >>> >>> /* >>> * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map is >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index 7cc141ef75e9..b99c4e3a69bf 100644 >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -2371,13 +2371,8 @@ void __init create_dom0(void) >>> >>> /* The vGIC for DOM0 is exactly emulating the hardware GIC */ >>> dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; >>> - /* >>> - * Xen vGIC supports a maximum of 992 interrupt lines. >>> - * 32 are substracted to cover local IRQs. >>> - */ >>> - dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; >>> - if ( gic_number_lines() > 992 ) >>> - printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n"); >>> + /* 32 are substracted to cover local IRQs */ >>> + dom0_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32; >> >> and reuse it here to make sure the value used is always the same. >> >>> dom0_cfg.arch.tee_type = tee_get_type(); >>> dom0_cfg.max_vcpus = dom0_max_vcpus(); >>> >>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >>> index acf61a4de373..e80fe0ca2421 100644 >>> --- a/xen/arch/arm/gic.c >>> +++ b/xen/arch/arm/gic.c >>> @@ -251,6 +251,9 @@ void __init gic_init(void) >>> panic("Failed to initialize the GIC drivers\n"); >>> /* Clear LR mask for cpu0 */ >>> clear_cpu_lr_mask(); >>> + >>> + if ( gic_number_lines() > VGIC_MAX_IRQS ) >>> + printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded\n"); >> >> I am a bit unsure with this one. >> If this is the case, it means any gicv2 or gicv3 init detected an impossible value and >> any usage of gic_number_lines would be using an impossible value. > Why impossible? GIC can support up to 1020 IRQs. Our vGIC can support up to 992 > IRQs. Maybe unsupported is a better wording, my point is that it could lead to non working system if say something uses irq 1000. > >> >> Shouldn't this somehow result in a panic as the gic detection was wrong ? >> Do you think we can continue to work safely if this value is over VGIC_MAX_IRQS. >> There are other places using gic_number_lines like in irq.c. > I can add a panic, sure. This would be a change in behavior because we used this > check for hwdom unconditionally. I'd need others opinion for this one. ok. Cheers Bertrand > > ~Michal
On 11/03/2025 11:12, Bertrand Marquis wrote: > > >> On 11 Mar 2025, at 10:59, Orzel, Michal <Michal.Orzel@amd.com> wrote: >> >> >> >> On 11/03/2025 10:30, Bertrand Marquis wrote: >>> >>> >>> Hi Michal, >>> >>>> On 11 Mar 2025, at 10:04, Michal Orzel <michal.orzel@amd.com> wrote: >>>> >>>> At the moment, we print a warning about max number of IRQs supported by >>>> GIC bigger than vGIC only for hardware domain. This check is not hwdom >>>> special, and should be made common. Also, in case of user not specifying >>>> nr_spis for dom0less domUs, we should take into account max number of >>>> IRQs supported by vGIC if it's smaller than for GIC. >>>> >>>> Introduce VGIC_MAX_IRQS macro and use it instead of hardcoded 992 value. >>>> Fix calculation of nr_spis for dom0less domUs and make the GIC/vGIC max >>>> IRQs comparison common. >>>> >>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com> >>>> --- >>>> xen/arch/arm/dom0less-build.c | 2 +- >>>> xen/arch/arm/domain_build.c | 9 ++------- >>>> xen/arch/arm/gic.c | 3 +++ >>>> xen/arch/arm/include/asm/vgic.h | 3 +++ >>>> 4 files changed, 9 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c >>>> index 31f31c38da3f..9a84fee94119 100644 >>>> --- a/xen/arch/arm/dom0less-build.c >>>> +++ b/xen/arch/arm/dom0less-build.c >>>> @@ -1018,7 +1018,7 @@ void __init create_domUs(void) >>>> { >>>> int vpl011_virq = GUEST_VPL011_SPI; >>>> >>>> - d_cfg.arch.nr_spis = gic_number_lines() - 32; >>>> + d_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32; >>> >>> I would suggest to introduce a static inline gic_nr_spis in a gic header ... >> Why GIC and not vGIC? This is domain's nr_spis, so vGIC. > > yes vGIC sorry. > >> But then, why static inline if the value does not change and is domain agnostic? >> I struggle to find a good name for this macro. Maybe (in vgic.h): >> #define vgic_def_nr_spis (min(gic_number_lines(), VGIC_MAX_IRQS) - 32) >> to denote default nr_spis if not set by the user? > > Yes that would work. My point is to prevent to have 2 definitions in 2 different > source file and a risk to forget to update one and not the other (let say if some > day we change 32 in 64). > >> >>> >>>> >>>> /* >>>> * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map is >>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>>> index 7cc141ef75e9..b99c4e3a69bf 100644 >>>> --- a/xen/arch/arm/domain_build.c >>>> +++ b/xen/arch/arm/domain_build.c >>>> @@ -2371,13 +2371,8 @@ void __init create_dom0(void) >>>> >>>> /* The vGIC for DOM0 is exactly emulating the hardware GIC */ >>>> dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; >>>> - /* >>>> - * Xen vGIC supports a maximum of 992 interrupt lines. >>>> - * 32 are substracted to cover local IRQs. >>>> - */ >>>> - dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; >>>> - if ( gic_number_lines() > 992 ) >>>> - printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n"); >>>> + /* 32 are substracted to cover local IRQs */ >>>> + dom0_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32; >>> >>> and reuse it here to make sure the value used is always the same. >>> >>>> dom0_cfg.arch.tee_type = tee_get_type(); >>>> dom0_cfg.max_vcpus = dom0_max_vcpus(); >>>> >>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >>>> index acf61a4de373..e80fe0ca2421 100644 >>>> --- a/xen/arch/arm/gic.c >>>> +++ b/xen/arch/arm/gic.c >>>> @@ -251,6 +251,9 @@ void __init gic_init(void) >>>> panic("Failed to initialize the GIC drivers\n"); >>>> /* Clear LR mask for cpu0 */ >>>> clear_cpu_lr_mask(); >>>> + >>>> + if ( gic_number_lines() > VGIC_MAX_IRQS ) >>>> + printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded\n"); >>> >>> I am a bit unsure with this one. >>> If this is the case, it means any gicv2 or gicv3 init detected an impossible value and >>> any usage of gic_number_lines would be using an impossible value. >> Why impossible? GIC can support up to 1020 IRQs. Our vGIC can support up to 992 >> IRQs. > > Maybe unsupported is a better wording, my point is that it could lead to non working system > if say something uses irq 1000. Actually, I took a look at the code and I don't think we should panic (i.e. we should keep things as they are today). In your example, if something uses IRQ > VGIC_MAX_IRQS that is bigger than gic_number_lines(), then we will receive error when mapping this IRQ to guest (but you don't have to use such device and in the future we may enable IRQ re-mapping). That's why in all the places related to domains, we use vgic_num_irqs() and not gic_number_lines(). The latter is only used for IRQs routed to Xen. ~Michal
Hi Michal, > On 11 Mar 2025, at 12:06, Orzel, Michal <michal.orzel@amd.com> wrote: > > > > On 11/03/2025 11:12, Bertrand Marquis wrote: >> >> >>> On 11 Mar 2025, at 10:59, Orzel, Michal <Michal.Orzel@amd.com> wrote: >>> >>> >>> >>> On 11/03/2025 10:30, Bertrand Marquis wrote: >>>> >>>> >>>> Hi Michal, >>>> >>>>> On 11 Mar 2025, at 10:04, Michal Orzel <michal.orzel@amd.com> wrote: >>>>> >>>>> At the moment, we print a warning about max number of IRQs supported by >>>>> GIC bigger than vGIC only for hardware domain. This check is not hwdom >>>>> special, and should be made common. Also, in case of user not specifying >>>>> nr_spis for dom0less domUs, we should take into account max number of >>>>> IRQs supported by vGIC if it's smaller than for GIC. >>>>> >>>>> Introduce VGIC_MAX_IRQS macro and use it instead of hardcoded 992 value. >>>>> Fix calculation of nr_spis for dom0less domUs and make the GIC/vGIC max >>>>> IRQs comparison common. >>>>> >>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com> >>>>> --- >>>>> xen/arch/arm/dom0less-build.c | 2 +- >>>>> xen/arch/arm/domain_build.c | 9 ++------- >>>>> xen/arch/arm/gic.c | 3 +++ >>>>> xen/arch/arm/include/asm/vgic.h | 3 +++ >>>>> 4 files changed, 9 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c >>>>> index 31f31c38da3f..9a84fee94119 100644 >>>>> --- a/xen/arch/arm/dom0less-build.c >>>>> +++ b/xen/arch/arm/dom0less-build.c >>>>> @@ -1018,7 +1018,7 @@ void __init create_domUs(void) >>>>> { >>>>> int vpl011_virq = GUEST_VPL011_SPI; >>>>> >>>>> - d_cfg.arch.nr_spis = gic_number_lines() - 32; >>>>> + d_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32; >>>> >>>> I would suggest to introduce a static inline gic_nr_spis in a gic header ... >>> Why GIC and not vGIC? This is domain's nr_spis, so vGIC. >> >> yes vGIC sorry. >> >>> But then, why static inline if the value does not change and is domain agnostic? >>> I struggle to find a good name for this macro. Maybe (in vgic.h): >>> #define vgic_def_nr_spis (min(gic_number_lines(), VGIC_MAX_IRQS) - 32) >>> to denote default nr_spis if not set by the user? >> >> Yes that would work. My point is to prevent to have 2 definitions in 2 different >> source file and a risk to forget to update one and not the other (let say if some >> day we change 32 in 64). >> >>> >>>> >>>>> >>>>> /* >>>>> * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map is >>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>>>> index 7cc141ef75e9..b99c4e3a69bf 100644 >>>>> --- a/xen/arch/arm/domain_build.c >>>>> +++ b/xen/arch/arm/domain_build.c >>>>> @@ -2371,13 +2371,8 @@ void __init create_dom0(void) >>>>> >>>>> /* The vGIC for DOM0 is exactly emulating the hardware GIC */ >>>>> dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; >>>>> - /* >>>>> - * Xen vGIC supports a maximum of 992 interrupt lines. >>>>> - * 32 are substracted to cover local IRQs. >>>>> - */ >>>>> - dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; >>>>> - if ( gic_number_lines() > 992 ) >>>>> - printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n"); >>>>> + /* 32 are substracted to cover local IRQs */ >>>>> + dom0_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32; >>>> >>>> and reuse it here to make sure the value used is always the same. >>>> >>>>> dom0_cfg.arch.tee_type = tee_get_type(); >>>>> dom0_cfg.max_vcpus = dom0_max_vcpus(); >>>>> >>>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >>>>> index acf61a4de373..e80fe0ca2421 100644 >>>>> --- a/xen/arch/arm/gic.c >>>>> +++ b/xen/arch/arm/gic.c >>>>> @@ -251,6 +251,9 @@ void __init gic_init(void) >>>>> panic("Failed to initialize the GIC drivers\n"); >>>>> /* Clear LR mask for cpu0 */ >>>>> clear_cpu_lr_mask(); >>>>> + >>>>> + if ( gic_number_lines() > VGIC_MAX_IRQS ) >>>>> + printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded\n"); >>>> >>>> I am a bit unsure with this one. >>>> If this is the case, it means any gicv2 or gicv3 init detected an impossible value and >>>> any usage of gic_number_lines would be using an impossible value. >>> Why impossible? GIC can support up to 1020 IRQs. Our vGIC can support up to 992 >>> IRQs. >> >> Maybe unsupported is a better wording, my point is that it could lead to non working system >> if say something uses irq 1000. > Actually, I took a look at the code and I don't think we should panic (i.e. we > should keep things as they are today). In your example, if something uses IRQ > > VGIC_MAX_IRQS that is bigger than gic_number_lines(), then we will receive error > when mapping this IRQ to guest (but you don't have to use such device and in the > future we may enable IRQ re-mapping). That's why in all the places related to > domains, we use vgic_num_irqs() and not gic_number_lines(). The latter is only > used for IRQs routed to Xen. So you will get an error later such an IRQ is mapped to a guest. Tracking why this is might not be easy. Anyway this is a hardware value that the user has no power on so if we panic it would mean Xen would never run on the given hardware without any chance for the user to workaround the problem so maybe the Warning is the best we can do here. Maybe others have an other idea here but i will not object to this. Cheers Bertrand > > ~Michal >
On 11/03/2025 14:26, Bertrand Marquis wrote: > > > Hi Michal, > >> On 11 Mar 2025, at 12:06, Orzel, Michal <michal.orzel@amd.com> wrote: >> >> >> >> On 11/03/2025 11:12, Bertrand Marquis wrote: >>> >>> >>>> On 11 Mar 2025, at 10:59, Orzel, Michal <Michal.Orzel@amd.com> wrote: >>>> >>>> >>>> >>>> On 11/03/2025 10:30, Bertrand Marquis wrote: >>>>> >>>>> >>>>> Hi Michal, >>>>> >>>>>> On 11 Mar 2025, at 10:04, Michal Orzel <michal.orzel@amd.com> wrote: >>>>>> >>>>>> At the moment, we print a warning about max number of IRQs supported by >>>>>> GIC bigger than vGIC only for hardware domain. This check is not hwdom >>>>>> special, and should be made common. Also, in case of user not specifying >>>>>> nr_spis for dom0less domUs, we should take into account max number of >>>>>> IRQs supported by vGIC if it's smaller than for GIC. >>>>>> >>>>>> Introduce VGIC_MAX_IRQS macro and use it instead of hardcoded 992 value. >>>>>> Fix calculation of nr_spis for dom0less domUs and make the GIC/vGIC max >>>>>> IRQs comparison common. >>>>>> >>>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com> >>>>>> --- >>>>>> xen/arch/arm/dom0less-build.c | 2 +- >>>>>> xen/arch/arm/domain_build.c | 9 ++------- >>>>>> xen/arch/arm/gic.c | 3 +++ >>>>>> xen/arch/arm/include/asm/vgic.h | 3 +++ >>>>>> 4 files changed, 9 insertions(+), 8 deletions(-) >>>>>> >>>>>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c >>>>>> index 31f31c38da3f..9a84fee94119 100644 >>>>>> --- a/xen/arch/arm/dom0less-build.c >>>>>> +++ b/xen/arch/arm/dom0less-build.c >>>>>> @@ -1018,7 +1018,7 @@ void __init create_domUs(void) >>>>>> { >>>>>> int vpl011_virq = GUEST_VPL011_SPI; >>>>>> >>>>>> - d_cfg.arch.nr_spis = gic_number_lines() - 32; >>>>>> + d_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32; >>>>> >>>>> I would suggest to introduce a static inline gic_nr_spis in a gic header ... >>>> Why GIC and not vGIC? This is domain's nr_spis, so vGIC. >>> >>> yes vGIC sorry. >>> >>>> But then, why static inline if the value does not change and is domain agnostic? >>>> I struggle to find a good name for this macro. Maybe (in vgic.h): >>>> #define vgic_def_nr_spis (min(gic_number_lines(), VGIC_MAX_IRQS) - 32) >>>> to denote default nr_spis if not set by the user? >>> >>> Yes that would work. My point is to prevent to have 2 definitions in 2 different >>> source file and a risk to forget to update one and not the other (let say if some >>> day we change 32 in 64). >>> >>>> >>>>> >>>>>> >>>>>> /* >>>>>> * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map is >>>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>>>>> index 7cc141ef75e9..b99c4e3a69bf 100644 >>>>>> --- a/xen/arch/arm/domain_build.c >>>>>> +++ b/xen/arch/arm/domain_build.c >>>>>> @@ -2371,13 +2371,8 @@ void __init create_dom0(void) >>>>>> >>>>>> /* The vGIC for DOM0 is exactly emulating the hardware GIC */ >>>>>> dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; >>>>>> - /* >>>>>> - * Xen vGIC supports a maximum of 992 interrupt lines. >>>>>> - * 32 are substracted to cover local IRQs. >>>>>> - */ >>>>>> - dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; >>>>>> - if ( gic_number_lines() > 992 ) >>>>>> - printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n"); >>>>>> + /* 32 are substracted to cover local IRQs */ >>>>>> + dom0_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32; >>>>> >>>>> and reuse it here to make sure the value used is always the same. >>>>> >>>>>> dom0_cfg.arch.tee_type = tee_get_type(); >>>>>> dom0_cfg.max_vcpus = dom0_max_vcpus(); >>>>>> >>>>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >>>>>> index acf61a4de373..e80fe0ca2421 100644 >>>>>> --- a/xen/arch/arm/gic.c >>>>>> +++ b/xen/arch/arm/gic.c >>>>>> @@ -251,6 +251,9 @@ void __init gic_init(void) >>>>>> panic("Failed to initialize the GIC drivers\n"); >>>>>> /* Clear LR mask for cpu0 */ >>>>>> clear_cpu_lr_mask(); >>>>>> + >>>>>> + if ( gic_number_lines() > VGIC_MAX_IRQS ) >>>>>> + printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded\n"); >>>>> >>>>> I am a bit unsure with this one. >>>>> If this is the case, it means any gicv2 or gicv3 init detected an impossible value and >>>>> any usage of gic_number_lines would be using an impossible value. >>>> Why impossible? GIC can support up to 1020 IRQs. Our vGIC can support up to 992 >>>> IRQs. >>> >>> Maybe unsupported is a better wording, my point is that it could lead to non working system >>> if say something uses irq 1000. >> Actually, I took a look at the code and I don't think we should panic (i.e. we >> should keep things as they are today). In your example, if something uses IRQ > >> VGIC_MAX_IRQS that is bigger than gic_number_lines(), then we will receive error >> when mapping this IRQ to guest (but you don't have to use such device and in the >> future we may enable IRQ re-mapping). That's why in all the places related to >> domains, we use vgic_num_irqs() and not gic_number_lines(). The latter is only >> used for IRQs routed to Xen. > > So you will get an error later such an IRQ is mapped to a guest. Tracking why this is might not > be easy. I did check and we get a nice error that I find good enough, e.g.: (XEN) the vIRQ number 260 is too high for domain 0 (max = 256) (XEN) Unable to map IRQ260 to d0 ~Michal
> On 11 Mar 2025, at 14:33, Orzel, Michal <michal.orzel@amd.com> wrote: > > > > On 11/03/2025 14:26, Bertrand Marquis wrote: >> >> >> Hi Michal, >> >>> On 11 Mar 2025, at 12:06, Orzel, Michal <michal.orzel@amd.com> wrote: >>> >>> >>> >>> On 11/03/2025 11:12, Bertrand Marquis wrote: >>>> >>>> >>>>> On 11 Mar 2025, at 10:59, Orzel, Michal <Michal.Orzel@amd.com> wrote: >>>>> >>>>> >>>>> >>>>> On 11/03/2025 10:30, Bertrand Marquis wrote: >>>>>> >>>>>> >>>>>> Hi Michal, >>>>>> >>>>>>> On 11 Mar 2025, at 10:04, Michal Orzel <michal.orzel@amd.com> wrote: >>>>>>> >>>>>>> At the moment, we print a warning about max number of IRQs supported by >>>>>>> GIC bigger than vGIC only for hardware domain. This check is not hwdom >>>>>>> special, and should be made common. Also, in case of user not specifying >>>>>>> nr_spis for dom0less domUs, we should take into account max number of >>>>>>> IRQs supported by vGIC if it's smaller than for GIC. >>>>>>> >>>>>>> Introduce VGIC_MAX_IRQS macro and use it instead of hardcoded 992 value. >>>>>>> Fix calculation of nr_spis for dom0less domUs and make the GIC/vGIC max >>>>>>> IRQs comparison common. >>>>>>> >>>>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com> >>>>>>> --- >>>>>>> xen/arch/arm/dom0less-build.c | 2 +- >>>>>>> xen/arch/arm/domain_build.c | 9 ++------- >>>>>>> xen/arch/arm/gic.c | 3 +++ >>>>>>> xen/arch/arm/include/asm/vgic.h | 3 +++ >>>>>>> 4 files changed, 9 insertions(+), 8 deletions(-) >>>>>>> >>>>>>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c >>>>>>> index 31f31c38da3f..9a84fee94119 100644 >>>>>>> --- a/xen/arch/arm/dom0less-build.c >>>>>>> +++ b/xen/arch/arm/dom0less-build.c >>>>>>> @@ -1018,7 +1018,7 @@ void __init create_domUs(void) >>>>>>> { >>>>>>> int vpl011_virq = GUEST_VPL011_SPI; >>>>>>> >>>>>>> - d_cfg.arch.nr_spis = gic_number_lines() - 32; >>>>>>> + d_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32; >>>>>> >>>>>> I would suggest to introduce a static inline gic_nr_spis in a gic header ... >>>>> Why GIC and not vGIC? This is domain's nr_spis, so vGIC. >>>> >>>> yes vGIC sorry. >>>> >>>>> But then, why static inline if the value does not change and is domain agnostic? >>>>> I struggle to find a good name for this macro. Maybe (in vgic.h): >>>>> #define vgic_def_nr_spis (min(gic_number_lines(), VGIC_MAX_IRQS) - 32) >>>>> to denote default nr_spis if not set by the user? >>>> >>>> Yes that would work. My point is to prevent to have 2 definitions in 2 different >>>> source file and a risk to forget to update one and not the other (let say if some >>>> day we change 32 in 64). >>>> >>>>> >>>>>> >>>>>>> >>>>>>> /* >>>>>>> * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map is >>>>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>>>>>> index 7cc141ef75e9..b99c4e3a69bf 100644 >>>>>>> --- a/xen/arch/arm/domain_build.c >>>>>>> +++ b/xen/arch/arm/domain_build.c >>>>>>> @@ -2371,13 +2371,8 @@ void __init create_dom0(void) >>>>>>> >>>>>>> /* The vGIC for DOM0 is exactly emulating the hardware GIC */ >>>>>>> dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; >>>>>>> - /* >>>>>>> - * Xen vGIC supports a maximum of 992 interrupt lines. >>>>>>> - * 32 are substracted to cover local IRQs. >>>>>>> - */ >>>>>>> - dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; >>>>>>> - if ( gic_number_lines() > 992 ) >>>>>>> - printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n"); >>>>>>> + /* 32 are substracted to cover local IRQs */ >>>>>>> + dom0_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32; >>>>>> >>>>>> and reuse it here to make sure the value used is always the same. >>>>>> >>>>>>> dom0_cfg.arch.tee_type = tee_get_type(); >>>>>>> dom0_cfg.max_vcpus = dom0_max_vcpus(); >>>>>>> >>>>>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >>>>>>> index acf61a4de373..e80fe0ca2421 100644 >>>>>>> --- a/xen/arch/arm/gic.c >>>>>>> +++ b/xen/arch/arm/gic.c >>>>>>> @@ -251,6 +251,9 @@ void __init gic_init(void) >>>>>>> panic("Failed to initialize the GIC drivers\n"); >>>>>>> /* Clear LR mask for cpu0 */ >>>>>>> clear_cpu_lr_mask(); >>>>>>> + >>>>>>> + if ( gic_number_lines() > VGIC_MAX_IRQS ) >>>>>>> + printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded\n"); >>>>>> >>>>>> I am a bit unsure with this one. >>>>>> If this is the case, it means any gicv2 or gicv3 init detected an impossible value and >>>>>> any usage of gic_number_lines would be using an impossible value. >>>>> Why impossible? GIC can support up to 1020 IRQs. Our vGIC can support up to 992 >>>>> IRQs. >>>> >>>> Maybe unsupported is a better wording, my point is that it could lead to non working system >>>> if say something uses irq 1000. >>> Actually, I took a look at the code and I don't think we should panic (i.e. we >>> should keep things as they are today). In your example, if something uses IRQ > >>> VGIC_MAX_IRQS that is bigger than gic_number_lines(), then we will receive error >>> when mapping this IRQ to guest (but you don't have to use such device and in the >>> future we may enable IRQ re-mapping). That's why in all the places related to >>> domains, we use vgic_num_irqs() and not gic_number_lines(). The latter is only >>> used for IRQs routed to Xen. >> >> So you will get an error later such an IRQ is mapped to a guest. Tracking why this is might not >> be easy. > I did check and we get a nice error that I find good enough, e.g.: > (XEN) the vIRQ number 260 is too high for domain 0 (max = 256) > (XEN) Unable to map IRQ260 to d0 Agree this is enough. If the user does not get why it says 256 it will be able to find the warning earlier in the logs. Thanks for digging Cheers Bertrand > > ~Michal
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c index 31f31c38da3f..9a84fee94119 100644 --- a/xen/arch/arm/dom0less-build.c +++ b/xen/arch/arm/dom0less-build.c @@ -1018,7 +1018,7 @@ void __init create_domUs(void) { int vpl011_virq = GUEST_VPL011_SPI; - d_cfg.arch.nr_spis = gic_number_lines() - 32; + d_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32; /* * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map is diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 7cc141ef75e9..b99c4e3a69bf 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -2371,13 +2371,8 @@ void __init create_dom0(void) /* The vGIC for DOM0 is exactly emulating the hardware GIC */ dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; - /* - * Xen vGIC supports a maximum of 992 interrupt lines. - * 32 are substracted to cover local IRQs. - */ - dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; - if ( gic_number_lines() > 992 ) - printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n"); + /* 32 are substracted to cover local IRQs */ + dom0_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32; dom0_cfg.arch.tee_type = tee_get_type(); dom0_cfg.max_vcpus = dom0_max_vcpus(); diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index acf61a4de373..e80fe0ca2421 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -251,6 +251,9 @@ void __init gic_init(void) panic("Failed to initialize the GIC drivers\n"); /* Clear LR mask for cpu0 */ clear_cpu_lr_mask(); + + if ( gic_number_lines() > VGIC_MAX_IRQS ) + printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded\n"); } void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi) diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h index e309dca1ad01..c549e5840bfa 100644 --- a/xen/arch/arm/include/asm/vgic.h +++ b/xen/arch/arm/include/asm/vgic.h @@ -329,6 +329,9 @@ extern void vgic_check_inflight_irqs_pending(struct vcpu *v, */ #define vgic_num_irqs(d) ((d)->arch.vgic.nr_spis + 32) +/* Maximum number of IRQs supported by vGIC */ +#define VGIC_MAX_IRQS 992U + /* * Allocate a guest VIRQ * - spi == 0 => allocate a PPI. It will be the same on every vCPU
At the moment, we print a warning about max number of IRQs supported by GIC bigger than vGIC only for hardware domain. This check is not hwdom special, and should be made common. Also, in case of user not specifying nr_spis for dom0less domUs, we should take into account max number of IRQs supported by vGIC if it's smaller than for GIC. Introduce VGIC_MAX_IRQS macro and use it instead of hardcoded 992 value. Fix calculation of nr_spis for dom0less domUs and make the GIC/vGIC max IRQs comparison common. Signed-off-by: Michal Orzel <michal.orzel@amd.com> --- xen/arch/arm/dom0less-build.c | 2 +- xen/arch/arm/domain_build.c | 9 ++------- xen/arch/arm/gic.c | 3 +++ xen/arch/arm/include/asm/vgic.h | 3 +++ 4 files changed, 9 insertions(+), 8 deletions(-)