xen/arch/arm/gic-vgic.c | 11 ++++++++++- xen/arch/arm/gic.c | 10 +++++++++- xen/arch/arm/include/asm/domain.h | 4 ++++ xen/arch/arm/include/asm/gic.h | 8 ++++++++ 4 files changed, 31 insertions(+), 2 deletions(-)
The lr_mask bitmap tracks which List Registers (LRs) are in use for
virtual interrupt injection. Previously, lr_mask always used uint64_t
(8 bytes) to support the maximum number of LRs across both GIC versions.
However, GICv2 and GICv3 have different hardware limits:
- GICv3: ICH_VTR_EL2[3:0] encodes LR count -> max 16 LRs (4 bits)
- GICv2: GICH_VTR[5:0] encodes LR count -> max 64 LRs (6 bits)
This patch introduces conditional compilation to optimize lr_mask size:
- CONFIG_GICV3=y: Use uint16_t (2 bytes) - sufficient for 16 LRs
- CONFIG_GICV3=n: Use uint64_t (8 bytes) - required for 64 LRs
With this, parameter 'lr' in gicv3_ich_read_lr(), gicv3_ich_write_lr()
cannot have a value > 15. Thus, it should not possible to hit the
BUG() in the default case.
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
xen/arch/arm/gic-vgic.c | 11 ++++++++++-
xen/arch/arm/gic.c | 10 +++++++++-
xen/arch/arm/include/asm/domain.h | 4 ++++
xen/arch/arm/include/asm/gic.h | 8 ++++++++
4 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index ea48c5375a..80d3beb225 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -16,8 +16,13 @@
#include <asm/gic.h>
#include <asm/vgic.h>
+#ifdef CONFIG_GICV3
#define lr_all_full() \
- (this_cpu(lr_mask) == (-1ULL >> (64 - gic_get_nr_lrs())))
+ (this_cpu(lr_mask) == ((uint16_t)-1 >> (16 - gic_get_nr_lrs())))
+#else
+#define lr_all_full() \
+ (this_cpu(lr_mask) == ((uint64_t)-1 >> (64 - gic_get_nr_lrs())))
+#endif
#undef GIC_DEBUG
@@ -102,7 +107,11 @@ static unsigned int gic_find_unused_lr(struct vcpu *v,
struct pending_irq *p,
unsigned int lr)
{
+#ifdef CONFIG_GICV3
+ uint16_t *lr_mask = &this_cpu(lr_mask);
+#else
uint64_t *lr_mask = &this_cpu(lr_mask);
+#endif
ASSERT(spin_is_locked(&v->arch.vgic.lock));
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index ee75258fc3..e1121a5bb3 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -29,7 +29,11 @@
#include <asm/vgic.h>
#include <asm/acpi.h>
+#ifdef CONFIG_GICV3
+DEFINE_PER_CPU(uint16_t, lr_mask);
+#else
DEFINE_PER_CPU(uint64_t, lr_mask);
+#endif
#undef GIC_DEBUG
@@ -48,7 +52,7 @@ void register_gic_ops(const struct gic_hw_operations *ops)
static void clear_cpu_lr_mask(void)
{
- this_cpu(lr_mask) = 0ULL;
+ this_cpu(lr_mask) = 0;
}
enum gic_version gic_hw_version(void)
@@ -382,7 +386,11 @@ static void maintenance_interrupt(int irq, void *dev_id)
void gic_dump_info(struct vcpu *v)
{
+#ifdef CONFIG_GICV3
+ printk("GICH_LRs (vcpu %d) mask=%"PRIx16"\n", v->vcpu_id, v->arch.lr_mask);
+#else
printk("GICH_LRs (vcpu %d) mask=%"PRIx64"\n", v->vcpu_id, v->arch.lr_mask);
+#endif
gic_hw_ops->dump_state(v);
}
diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index 758ad807e4..8654ef89ef 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -228,7 +228,11 @@ struct arch_vcpu
/* Holds gic context data */
union gic_state_data gic;
+#ifdef CONFIG_GICV3
+ uint16_t lr_mask;
+#else
uint64_t lr_mask;
+#endif
struct vgic_cpu vgic;
diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
index 8e713aa477..e1559ec98c 100644
--- a/xen/arch/arm/include/asm/gic.h
+++ b/xen/arch/arm/include/asm/gic.h
@@ -237,7 +237,15 @@ enum gic_version {
GIC_V3,
};
+/*
+ * GICv3 supports up to 16 LRs (4 bits in ICH_VTR_EL2), can use uint16_t
+ * GICv2 supports up to 64 LRs (6 bits in GICH_VTR), requires uint64_t
+ */
+#ifdef CONFIG_GICV3
+DECLARE_PER_CPU(uint16_t, lr_mask);
+#else
DECLARE_PER_CPU(uint64_t, lr_mask);
+#endif
extern enum gic_version gic_hw_version(void);
--
2.25.1On 05.03.2026 20:57, Ayan Kumar Halder wrote:
> --- a/xen/arch/arm/include/asm/gic.h
> +++ b/xen/arch/arm/include/asm/gic.h
> @@ -237,7 +237,15 @@ enum gic_version {
> GIC_V3,
> };
>
> +/*
> + * GICv3 supports up to 16 LRs (4 bits in ICH_VTR_EL2), can use uint16_t
> + * GICv2 supports up to 64 LRs (6 bits in GICH_VTR), requires uint64_t
> + */
> +#ifdef CONFIG_GICV3
> +DECLARE_PER_CPU(uint16_t, lr_mask);
> +#else
> DECLARE_PER_CPU(uint64_t, lr_mask);
> +#endif
But GICV2 and GICV3 can be enabled at the same time, at which point you'd still
need 64 bits, I suppose.
Jan
Hi Jan,
On 06/03/2026 08:49, Jan Beulich wrote:
> On 05.03.2026 20:57, Ayan Kumar Halder wrote:
>> --- a/xen/arch/arm/include/asm/gic.h
>> +++ b/xen/arch/arm/include/asm/gic.h
>> @@ -237,7 +237,15 @@ enum gic_version {
>> GIC_V3,
>> };
>>
>> +/*
>> + * GICv3 supports up to 16 LRs (4 bits in ICH_VTR_EL2), can use uint16_t
>> + * GICv2 supports up to 64 LRs (6 bits in GICH_VTR), requires uint64_t
>> + */
>> +#ifdef CONFIG_GICV3
>> +DECLARE_PER_CPU(uint16_t, lr_mask);
>> +#else
>> DECLARE_PER_CPU(uint64_t, lr_mask);
>> +#endif
> But GICV2 and GICV3 can be enabled at the same time, at which point you'd still
> need 64 bits, I suppose.
I see. However, a safety certified Xen which is meant to support GICv3
hardware, will only have GICv3 enabled.
IOW having both GICV2 =y && GICV2 = y is out of scope for the safety use
cases.
- Ayan
On 06/03/2026 10:09, Halder, Ayan Kumar wrote:
> Hi Jan,
>
> On 06/03/2026 08:49, Jan Beulich wrote:
>> On 05.03.2026 20:57, Ayan Kumar Halder wrote:
>>> --- a/xen/arch/arm/include/asm/gic.h
>>> +++ b/xen/arch/arm/include/asm/gic.h
>>> @@ -237,7 +237,15 @@ enum gic_version {
>>> GIC_V3,
>>> };
>>> +/*
>>> + * GICv3 supports up to 16 LRs (4 bits in ICH_VTR_EL2), can use
>>> uint16_t
>>> + * GICv2 supports up to 64 LRs (6 bits in GICH_VTR), requires uint64_t
>>> + */
>>> +#ifdef CONFIG_GICV3
>>> +DECLARE_PER_CPU(uint16_t, lr_mask);
>>> +#else
>>> DECLARE_PER_CPU(uint64_t, lr_mask);
>>> +#endif
>> But GICV2 and GICV3 can be enabled at the same time, at which point
>> you'd still
>> need 64 bits, I suppose.
>
> I see. However, a safety certified Xen which is meant to support GICv3
> hardware, will only have GICv3 enabled.
>
> IOW having both GICV2 =y && GICV2 = y is out of scope for the safety use
> cases.
If the patch is indented to be merged in mainline Xen, then you need to
support the case where both GICV2 = y && GICV3 = y.
If the patch is not intended to be merged in mainline then it should
have been marked as such so we can adjust our review.
Cheers,
--
Julien Grall
On 05/03/2026 7:57 pm, Ayan Kumar Halder wrote: > The lr_mask bitmap tracks which List Registers (LRs) are in use for > virtual interrupt injection. Previously, lr_mask always used uint64_t > (8 bytes) to support the maximum number of LRs across both GIC versions. > > However, GICv2 and GICv3 have different hardware limits: > - GICv3: ICH_VTR_EL2[3:0] encodes LR count -> max 16 LRs (4 bits) > - GICv2: GICH_VTR[5:0] encodes LR count -> max 64 LRs (6 bits) > > This patch introduces conditional compilation to optimize lr_mask size: > - CONFIG_GICV3=y: Use uint16_t (2 bytes) - sufficient for 16 LRs > - CONFIG_GICV3=n: Use uint64_t (8 bytes) - required for 64 LRs > > With this, parameter 'lr' in gicv3_ich_read_lr(), gicv3_ich_write_lr() > cannot have a value > 15. Thus, it should not possible to hit the > BUG() in the default case. > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> What does this actually get you? Because it doesn't actually eliminate the BUG()s you reference. If you really want to go ahead with this patch, then make a lr_mask_t or the right type and don't double-code everything. ~Andrew
Hi Andrew,
On 05/03/2026 23:01, Andrew Cooper wrote:
> On 05/03/2026 7:57 pm, Ayan Kumar Halder wrote:
>> The lr_mask bitmap tracks which List Registers (LRs) are in use for
>> virtual interrupt injection. Previously, lr_mask always used uint64_t
>> (8 bytes) to support the maximum number of LRs across both GIC versions.
>>
>> However, GICv2 and GICv3 have different hardware limits:
>> - GICv3: ICH_VTR_EL2[3:0] encodes LR count -> max 16 LRs (4 bits)
>> - GICv2: GICH_VTR[5:0] encodes LR count -> max 64 LRs (6 bits)
>>
>> This patch introduces conditional compilation to optimize lr_mask size:
>> - CONFIG_GICV3=y: Use uint16_t (2 bytes) - sufficient for 16 LRs
>> - CONFIG_GICV3=n: Use uint64_t (8 bytes) - required for 64 LRs
>>
>> With this, parameter 'lr' in gicv3_ich_read_lr(), gicv3_ich_write_lr()
>> cannot have a value > 15. Thus, it should not possible to hit the
>> BUG() in the default case.
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> What does this actually get you?
>
> Because it doesn't actually eliminate the BUG()s you reference.
As lr is obtained from lr_mask, see the snippet from gic_find_unused_lr()
for_each_set_bit ( used_lr, *lr_mask )
{
struct gic_lr lr_val;
gic_hw_ops->read_lr(used_lr, &lr_val);
if ( lr_val.virq == p->irq )
return used_lr;
}
If lr_mask is 16 bits, then used_lr should not exceed 15. That is my
thinking.
>
> If you really want to go ahead with this patch, then make a lr_mask_t
> or the right type and don't double-code everything.
Ack
- Ayan
On 06/03/2026 9:59 am, Halder, Ayan Kumar wrote:
> Hi Andrew,
>
> On 05/03/2026 23:01, Andrew Cooper wrote:
>> On 05/03/2026 7:57 pm, Ayan Kumar Halder wrote:
>>> The lr_mask bitmap tracks which List Registers (LRs) are in use for
>>> virtual interrupt injection. Previously, lr_mask always used uint64_t
>>> (8 bytes) to support the maximum number of LRs across both GIC
>>> versions.
>>>
>>> However, GICv2 and GICv3 have different hardware limits:
>>> - GICv3: ICH_VTR_EL2[3:0] encodes LR count -> max 16 LRs (4 bits)
>>> - GICv2: GICH_VTR[5:0] encodes LR count -> max 64 LRs (6 bits)
>>>
>>> This patch introduces conditional compilation to optimize lr_mask size:
>>> - CONFIG_GICV3=y: Use uint16_t (2 bytes) - sufficient for 16 LRs
>>> - CONFIG_GICV3=n: Use uint64_t (8 bytes) - required for 64 LRs
>>>
>>> With this, parameter 'lr' in gicv3_ich_read_lr(), gicv3_ich_write_lr()
>>> cannot have a value > 15. Thus, it should not possible to hit the
>>> BUG() in the default case.
>>>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> What does this actually get you?
>>
>> Because it doesn't actually eliminate the BUG()s you reference.
>
> As lr is obtained from lr_mask, see the snippet from gic_find_unused_lr()
>
> for_each_set_bit ( used_lr, *lr_mask )
> {
> struct gic_lr lr_val;
>
> gic_hw_ops->read_lr(used_lr, &lr_val);
> if ( lr_val.virq == p->irq )
> return used_lr;
> }
>
> If lr_mask is 16 bits, then used_lr should not exceed 15. That is my
> thinking.
Sure, but what does that actually get you?
It doesn't get you a compiled difference. It can't DCE the boundary checks.
So right now, it looks like it's complexity for no gain.
~Andrew
Hi Andrew,
On 06/03/2026 10:02, Andrew Cooper wrote:
> On 06/03/2026 9:59 am, Halder, Ayan Kumar wrote:
>> Hi Andrew,
>>
>> On 05/03/2026 23:01, Andrew Cooper wrote:
>>> On 05/03/2026 7:57 pm, Ayan Kumar Halder wrote:
>>>> The lr_mask bitmap tracks which List Registers (LRs) are in use for
>>>> virtual interrupt injection. Previously, lr_mask always used uint64_t
>>>> (8 bytes) to support the maximum number of LRs across both GIC
>>>> versions.
>>>>
>>>> However, GICv2 and GICv3 have different hardware limits:
>>>> - GICv3: ICH_VTR_EL2[3:0] encodes LR count -> max 16 LRs (4 bits)
>>>> - GICv2: GICH_VTR[5:0] encodes LR count -> max 64 LRs (6 bits)
>>>>
>>>> This patch introduces conditional compilation to optimize lr_mask size:
>>>> - CONFIG_GICV3=y: Use uint16_t (2 bytes) - sufficient for 16 LRs
>>>> - CONFIG_GICV3=n: Use uint64_t (8 bytes) - required for 64 LRs
>>>>
>>>> With this, parameter 'lr' in gicv3_ich_read_lr(), gicv3_ich_write_lr()
>>>> cannot have a value > 15. Thus, it should not possible to hit the
>>>> BUG() in the default case.
>>>>
>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>> What does this actually get you?
>>>
>>> Because it doesn't actually eliminate the BUG()s you reference.
>> As lr is obtained from lr_mask, see the snippet from gic_find_unused_lr()
>>
>> for_each_set_bit ( used_lr, *lr_mask )
>> {
>> struct gic_lr lr_val;
>>
>> gic_hw_ops->read_lr(used_lr, &lr_val);
>> if ( lr_val.virq == p->irq )
>> return used_lr;
>> }
>>
>> If lr_mask is 16 bits, then used_lr should not exceed 15. That is my
>> thinking.
> Sure, but what does that actually get you?
Right now, it just gives me a justification why a certain code path
(invoking BUG()) will never be executed. My aim here is to reduce the
chances hitting this runtime BUG(). However ...
>
> It doesn't get you a compiled difference. It can't DCE the boundary checks.
if there can be a better approach, I am open for it.
- Ayan
On 05/03/2026 11:01 pm, Andrew Cooper wrote: > On 05/03/2026 7:57 pm, Ayan Kumar Halder wrote: >> The lr_mask bitmap tracks which List Registers (LRs) are in use for >> virtual interrupt injection. Previously, lr_mask always used uint64_t >> (8 bytes) to support the maximum number of LRs across both GIC versions. >> >> However, GICv2 and GICv3 have different hardware limits: >> - GICv3: ICH_VTR_EL2[3:0] encodes LR count -> max 16 LRs (4 bits) >> - GICv2: GICH_VTR[5:0] encodes LR count -> max 64 LRs (6 bits) >> >> This patch introduces conditional compilation to optimize lr_mask size: >> - CONFIG_GICV3=y: Use uint16_t (2 bytes) - sufficient for 16 LRs >> - CONFIG_GICV3=n: Use uint64_t (8 bytes) - required for 64 LRs >> >> With this, parameter 'lr' in gicv3_ich_read_lr(), gicv3_ich_write_lr() >> cannot have a value > 15. Thus, it should not possible to hit the >> BUG() in the default case. >> >> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> > What does this actually get you? > > Because it doesn't actually eliminate the BUG()s you reference. > > If you really want to go ahead with this patch, then make a lr_mask_t > or the right type and don't double-code everything. Also, this creates an Out-of-Bounds read in vgic_sync_from_lrs() amongst others. ~Andrew
© 2016 - 2026 Red Hat, Inc.