xen/arch/arm/Kconfig | 9 +++++++++ xen/arch/arm/gic-v3.c | 12 ++++++++++-- 2 files changed, 19 insertions(+), 2 deletions(-)
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.
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@amd.com>
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
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)
/*
* 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,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");
+
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:
--
2.25.1Hi 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@amd.com>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
> 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
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@amd.com>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>> 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
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@amd.com>
>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>> ---
>>> 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
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@amd.com>
>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>> ---
>>>> 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.
>
> 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 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?
You mean
case 4: (lr >= lrs) ? 0 : READ_SYSREG_LR(4); /* read */
case 4 : (lr >= lrs) ? : WRITE_SYSREG_LR(val, 4); /* write */
> 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).
- Ayan
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@amd.com>
>>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>>> ---
>>>>> 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
On 06/03/2026 17:53, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
>
> 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@amd.com>
>>>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>>>> ---
>>>>>> 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.
I am trying to use compiler to do dead code elimination using constant
conditional. Refer the snippet:
static inline void gicv3_save_lrs(struct vcpu *v)
{
/* Fall through for all the cases */
switch ( lrs )
Here the pre-processor evaluates lrs to a compile time constant
(GICV3_NR_LRS). Thus, the compiler eliminates the code to access
ich_lr4_el2, etc when GICV3_NR_LRS == 4.
Is there a better way to achieve DCE ?
>
>>>
>>> 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 ;).
Yes, I am fine to put a ASSERT() or BUG_ON() in gicv3_hyp_init()
>
>>>>> 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'.
Yes, I agree.
- Ayan
© 2016 - 2026 Red Hat, Inc.