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

[Julien Grall <julien@xxxxxxx>]

Hi Ayan,

On 06/03/2026 09:51, Halder, Ayan Kumar wrote:
> 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.
Xen upstream has to support various use cases. One of the use-case is 
Linux distributions where they want one Xen binary booting on multiple 
HW. So "gicv3_info.nr_lrs" will have to stay forever.
Now with CONFIG_GICV3_NR_LRS in place, I am concerned that someone will 
try to configure the value to let say 16 but their HW support only 4. I 
can't find any check at boot, so any problem will still occur at runtime.
> > 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.
What about implementing it as RAZ-WI? This would make any issue more 
reliable/obvious (if you have multiple index accessing the same LR, then 
you could end up overwriting an existing virtual interrupt).
Cheers,

--
Julien Grall