Re: [RFC PATCH] xen: gic-v3: Introduce CONFIG_GICV3_NR_LRS

[Julien Grall <julien@xxxxxxx>]

Hi Ayan,

On 05/03/2026 19:43, Ayan Kumar Halder wrote:
> Set GICV3_NR_LRS as per the number of list registers in the supported
> hardware. This ensures:
>
> 1. In gicv3_save_lrs()/gicv3_restore_lrs(), use the number of list
> registers from GICV3_NR_LRS (if defined) instead of gicv3_info.nr_lrs.
> This ensures that if the hardware does not support more than 4 LRs
> (for example), the code accessing LR 4-15 is never reached. The
> compiler can eliminate the unsupported cases as the switch case uses a
> constant conditional.
>
> 2. Similarly In gicv3_ich_read_lr()/gicv3_ich_write_lr() , we can
> justify that the unsupported LRs (4-15) will never be reached as Xen
> will panic if the runtime value (lr) exceeds GICV3_NR_LRS. Some
> compiler can eliminate the code accessing LR 4-15.
> In this situation, using panic() is better than accessing a list
> register which is not present in the hardware
>
> 3. Whenever GICV3_NR_LRS is defined, the default condition and the
> related BUG() cannot be reached at all.
I am not sure how this is better. You will still crash Xen is 'lr' >= 
GICV3_NR_LRS. Can you provide some details?
> > As part of functional safety effort, we are trying to enable system
> integrator to configure Xen for a specific platform with a predefind
> set of GICv3 list registers. So that we can minimize the chance of
> runtime issues and reduce the codesize that will execute.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> ---
>   xen/arch/arm/Kconfig  |  9 +++++++++
>   xen/arch/arm/gic-v3.c | 12 ++++++++++--
>   2 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 2f2b501fda..6540013f97 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -276,6 +276,15 @@ config PCI_PASSTHROUGH
>   
>   endmenu
>   
> +config GICV3_NR_LRS
> +       int "Number of GICv3 Link Registers supported" if EXPERT
> +       depends on GICV3
> +       range 0 16
> +       default 0
> +       help
> +         Controls the number of Link registers to be accessed.
> +         Keep it set to 0 to use a value obtained from a hardware register.
> +
>   menu "ARM errata workaround via the alternative framework"
>         depends on HAS_ALTERNATIVE
>   
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index bc07f97c16..fb2985fd52 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -51,6 +51,8 @@ static DEFINE_PER_CPU(void __iomem*, rbase);
>   #define GICD                   (gicv3.map_dbase)
>   #define GICD_RDIST_BASE        (this_cpu(rbase))
>   #define GICD_RDIST_SGI_BASE    (GICD_RDIST_BASE + SZ_64K)
> +#define lrs                    (CONFIG_GICV3_NR_LRS ?: \
> +                                gicv3_info.nr_lrs)
We should avoid lowercase define, in particular with generic names like 
'lrs'. I think in this case, I would rather update gicv3_info.nr_lrs:
gicv3_info.nr_lrs = min(gv3_info.nr_lrs, CONFIG_GICV3_NR_LRS);

This would solve another problem where you don't sanity check that the 
system effectively support CONFIG_GICV3_NR_LRS.
> @@ -121,7 +123,7 @@ static inline void gicv3_save_lrs(struct vcpu *v)
>   static inline void gicv3_restore_lrs(const struct vcpu *v)
>   {
>       /* Fall through for all the cases */
> -    switch ( gicv3_info.nr_lrs )
> +    switch ( lrs )
>       {
>       case 16:
>           WRITE_SYSREG_LR(v->arch.gic.v3.lr[15], 15);
> @@ -178,6 +180,9 @@ static inline void gicv3_restore_lrs(const struct vcpu *v)
>   
>   static uint64_t gicv3_ich_read_lr(int lr)
>   {
> +    if ( lr >= lrs )
> +        panic("Unsupported number of LRs\n");
Do we really have to panic in production build? Wouldn't it be better to 
return '0' and maybe use ASSERT for a crash in debug build? Same below.
> +
>       switch ( lr )
>       {
>       case 0: return READ_SYSREG_LR(0);
> @@ -203,6 +208,9 @@ static uint64_t gicv3_ich_read_lr(int lr)
>   
>   static void gicv3_ich_write_lr(int lr, uint64_t val)
>   {
> +    if ( lr >= lrs )
> +        panic("Unsupported number of LRs\n");
> +
>       switch ( lr )
>       {
>       case 0:
Cheers,

--
Julien Grall