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

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

On 14/04/2026 03:06, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
> On 18/03/2026 23:09, Ayan Kumar Halder wrote:
> > One key requirement of Xen functional safety is to reduce the number
> > of lines of code to be safety certified. Besides, a safety certified
> > Xen requires a static hardware configuration to be defined. This static
> > hardware configuration is described as per the test hardware/emulator
> > hardware configuration against which Xen is verified.
> >
> > Introduce GICV3_NR_LRS with the two aims in mind:
> Out of interest, why is this limited to GICv3?
This was just my starting point of investigation. My intention is to 
have as much as a static defined hardware configuration, so that the 
code that cannot be tested on the hardware can be eliminated by one of 
the many ways (DCE, Kconfig or actual code removal).
The advantage of having a static defined configuration is that the 
system integrator will have the full control on how to configure Xen for 
a specific hardware platform. And we try to reduce as much as possible 
any code that cannot be used due to hardware limitations.
> > 1. User should set the number of GICV3 list registers as per the test
> > hardware so that the unwanted code can be removed using GCC's dead
> > code elimination or preprocessor's config.
> We discussed this offline, I am not fully convinced you can rely on 
> dead code elimination to always remove the BUG() in 
> gicv3_ich_read_lr(). If you want to rely on dead code eliminitation, 
> then you will want to call a function which have a prototype defined 
> but not implemented (similar to what we do for bitops with 
> __bad_atomic_read()) which would fail a link time if the compiler 
> didn't remove the code.
If you are ok, we can break this into 2 patches

1. Introduce GICV3_NR_LRS and make sure it is used consistently in the 
code. IOW, it should address the comments that Luca and you provided.
2. Implement a way for compiler to do DCE based on GICV3_NR_LRS.

> > 2. By doing #1, one can ensure that there is no untested code due to
> > unsupported hardware platform and thus there is no safety impact due
> > to untested code.
> >
> > However if the user does not set GICV3_NR_LRS, then it is set to 0.
> > Thus Xen will fallback to the default scenario (i.e. read the hardware
> > register to determine the number of LRS).
> >
> > 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. RAZ/WI for the unsupported LRs.
> >
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> > ---
> > Changelog:
> >
> > v1 - 1. s/lrs/LRS
> > 2. Implement RAZ/WI instead of panic
> >
> > Few comments which were not addressed
> > 1. Do "gicv3_info.nr_lrs to LRS" in gicv3_hyp_init() and keep the code
> > unchanged in gicv3_save_lrs()/gicv3_restore_lrs() -- This prevents the
> > compiler from doing dead code elimination as the switch condition cannot
> > be evaluated at compile time.
> > I am not sure how to get around this issue.
> >
> >   xen/arch/arm/Kconfig  |  9 +++++++++
> >   xen/arch/arm/gic-v3.c | 14 ++++++++++++--
> >   2 files changed, 21 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
> Supported by who? The hardware? Xen? Asking, because I could forsee an 
> integrator wanted to limit the number of LRs to something smaller than 
> what the HW supports (in a lot of cases, 2 LRs is sufficient).
Ack

 ... "Number of GICv3 Link Registers used" if EXPERT

So it implies a decision to be taken by the system integrator. Does it 
sound ok ?
> > +    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.
> I still strongly think that if GICV3_NR_LRS is set, then it needs to 
> be checked against the value read by the hardware
Ack.
> > +
> >   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..eaae95eb4d 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)
> IMHO, LRS is a little bit vague. What about MAX_LRS? Or maybe NR_LRS?
NR_LRS .
> >     /*
> >    * Saves all 16(Max) LR registers. Though number of LRs implemented
> > @@ -59,7 +61,7 @@ static DEFINE_PER_CPU(void __iomem*, rbase);
> >   static inline void gicv3_save_lrs(struct vcpu *v)
> >   {
> >       /* Fall through for all the cases */
> > -    switch ( gicv3_info.nr_lrs )
> > +    switch ( LRS )
> >       {
> >       case 16:
> >           v->arch.gic.v3.lr[15] = READ_SYSREG_LR(15);
> > @@ -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,10 @@ static inline void gicv3_restore_lrs(const 
> > struct vcpu *v)
> >     static uint64_t gicv3_ich_read_lr(int lr)
> >   {
> > +    /* RAZ for unsupported LR */
> > +    if ( lr >= LRS )
> > +        return 0;
> > +
> >       switch ( lr )
> >       {
> >       case 0: return READ_SYSREG_LR(0);
> > @@ -203,6 +209,10 @@ static uint64_t gicv3_ich_read_lr(int lr)
> >     static void gicv3_ich_write_lr(int lr, uint64_t val)
> >   {
> > +    /* WI for unsupported LR */
> AFAIU, this path is really not meant to happen. So I don't think we 
> want to silently ignore the write as it would mean an interrupt is 
> missing. I think in debug build, we probably want to use 
> ASSERT_UNREACHABLE(). If we want something for production as well, 
> then we could instead use WARN().
Ack. I will use WARN().

- Ayan

> Cheers,