Hi Michal,
> On 11 Mar 2025, at 12:06, Orzel, Michal <michal.or...@amd.com> wrote:
>
>
>
> On 11/03/2025 11:12, Bertrand Marquis wrote:
>>
>>
>>> On 11 Mar 2025, at 10:59, Orzel, Michal <michal.or...@amd.com> wrote:
>>>
>>>
>>>
>>> On 11/03/2025 10:30, Bertrand Marquis wrote:
>>>>
>>>>
>>>> Hi Michal,
>>>>
>>>>> On 11 Mar 2025, at 10:04, Michal Orzel <michal.or...@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.or...@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
>
