[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 1/2] xen/arm: Improve handling of nr_spis
On 11/03/2025 14:26, Bertrand Marquis wrote: > > > Hi Michal, > >> On 11 Mar 2025, at 12:06, Orzel, Michal <michal.orzel@xxxxxxx> wrote: >> >> >> >> On 11/03/2025 11:12, Bertrand Marquis wrote: >>> >>> >>>> On 11 Mar 2025, at 10:59, Orzel, Michal <Michal.Orzel@xxxxxxx> wrote: >>>> >>>> >>>> >>>> On 11/03/2025 10:30, Bertrand Marquis wrote: >>>>> >>>>> >>>>> Hi Michal, >>>>> >>>>>> On 11 Mar 2025, at 10:04, Michal Orzel <michal.orzel@xxxxxxx> 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@xxxxxxx> >>>>>> --- >>>>>> 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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |