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

["Halder, Ayan Kumar" <ayankuma@xxxxxxx>]

On 06/03/2026 09:24, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
> 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);
But we want to enforce the user to set CONFIG_GICV3_NR_LRS , so that we 
don't have to rely on gicv3_info.nr_lrs.
The only reason to use gicv3_info.nr_lrs is for backward compatibility 
i.e. when the user forgot to set the config and as a result it used the 
default value as 0. We don't want to allow this for the safety use cases.
> 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.
You are right, we may not need this. One option I am thinking is ...
> > +
> >       switch ( lr )
switch ( lr & (lrs - 1) )

This ensures that we do not hit the unsupported cases.

- Ayan