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

[Julien Grall <julien@xxxxxxx>]

Hi Ayan,

On 06/03/2026 17:19, Halder, Ayan Kumar wrote:
> On 06/03/2026 10:40, Julien Grall wrote:
> > Hi Ayan,
> Hi Julien,
> > 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.
> We can default to using gicv3_info.nr_lrs if the user does not set a 
> value in GICV3_NR_LRS. This is what I am trying to do in the patch.
I am still missing something. Why can't we just write GICV3_NR_LRS in 
gicv3_info.nr_lrs? This would simplify the code and avoid unnecessary churn.
> > 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.
> If the user has set it to an incorrect value, then imo Xen should trust 
> the value that user has set. And then any malfunction caused will be the 
> responsibility of the user.
> > IOW, we want to put the responsibility on the user to provide the
> correct values.
I think this is the same things as the device-tree. We could trust the 
user didn't shoot itself in the foot (e.g. asking for a feature which 
doesn't exist). But at least for Xen on Arm, we try to be nice and tell 
the user that something is wrong early.
I don't see why we should diverge here. This is defense in depth which 
would save a crash during steady state if this was missed. I am assuming 
that it is better to crash while your car is parked than while you are 
driving ;).
> > > > 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? 
> You mean
>
>   case 4: (lr >= lrs) ? 0 : READ_SYSREG_LR(4); /* read */
>
>   case 4 : (lr >= lrs) ? :  WRITE_SYSREG_LR(val, 4); /* write */
I was thinking to do the check before the 'switch' as i don't think it 
needs to be duplicated per 'case'.
Cheers,

--
Julien Grall