[PATCH for-11.2 0/4] hw/intc/arm_gicv5: Misc minor fixes

[PATCH for-11.2 0/4] hw/intc/arm_gicv5: Misc minor fixes

[Peter Maydell]

This is the handful of patches from the "fixes/tweaks to the existing
code" section at the start of my gicv5 series which didn't get
reviewed and which I think are fixes that we should put into
11.2. (The first one isn't strictly a fix but it's also pretty
simple.) Everything else in the gicv5 series is either already
upstream or else to be deferred to 12.0.

thanks
-- PMM

Peter Maydell (4):
  hw/intc/arm_gicv5: Use cached IRS_IST_BASER.VALID state
  hw/intc/arm_gicv5: Free memory in finalize
  target/arm: Only allow GICv5 on CPUs without AArch32 at EL1..3
  hw/intc/arm_gicv5: Use correct type for value in lpi_cache_get_hppi()

 target/arm/cpu-features.h  |  8 ++++++++
 hw/intc/arm_gicv5.c        | 20 +++++++++++++++-----
 hw/intc/arm_gicv5_common.c |  3 +++
 target/arm/cpu.c           | 37 +++++++------------------------------
 4 files changed, 33 insertions(+), 35 deletions(-)

-- 
2.43.0

[PATCH for-11.2 1/4] hw/intc/arm_gicv5: Use cached IRS_IST_BASER.VALID state

[Peter Maydell]

When the guest writes to IRS_IST_BASER we effectively cache the value
of the VALID bit in the GICv5ISTConfig valid field entry.  In a few
places we check whether the IST is valid by doing a bit extract on
the register value; switch those to look at cfg->structure instead.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gicv5.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/intc/arm_gicv5.c b/hw/intc/arm_gicv5.c
index 5d0a50ab6aa..d0a8574916b 100644
--- a/hw/intc/arm_gicv5.c
+++ b/hw/intc/arm_gicv5.c
@@ -1222,8 +1222,7 @@ static void irs_map_l2_istr_write(GICv5 *s, GICv5Domain domain, uint64_t value)
     uint64_t l1_iste;
     MemTxResult res;
 
-    if (!FIELD_EX64(cs->irs_ist_baser[domain], IRS_IST_BASER, VALID) ||
-        !cfg->structure) {
+    if (!cfg->valid || !cfg->structure) {
         /* WI if no IST set up or it is not 2-level */
         return;
     }
@@ -1293,7 +1292,7 @@ static void irs_ist_baser_write(GICv5 *s, GICv5Domain domain, uint64_t value)
 {
     GICv5Common *cs = ARM_GICV5_COMMON(s);
 
-    if (FIELD_EX64(cs->irs_ist_baser[domain], IRS_IST_BASER, VALID)) {
+    if (s->phys_lpi_config[domain].valid) {
         /* If VALID is set, ADDR is RO and we can only update VALID */
         bool valid = FIELD_EX64(value, IRS_IST_BASER, VALID);
         if (valid) {
@@ -1604,7 +1603,7 @@ static bool config_writel(GICv5 *s, GICv5Domain domain, hwaddr offset,
                             deposit64(cs->irs_ist_baser[domain], 32, 32, data));
         return true;
     case A_IRS_IST_CFGR:
-        if (FIELD_EX64(cs->irs_ist_baser[domain], IRS_IST_BASER, VALID)) {
+        if (s->phys_lpi_config[domain].valid) {
             qemu_log_mask(LOG_GUEST_ERROR,
                           "guest tried to write IRS_IST_CFGR for %s config frame "
                           "while IST_BASER.VALID set\n", domain_name[domain]);
-- 
2.43.0

Re: [PATCH for-11.2 1/4] hw/intc/arm_gicv5: Use cached IRS_IST_BASER.VALID state

[Jim MacArthur]

On Tue, Nov 03, 2026 at 05:26:13PM +0000, Peter Maydell wrote:
> When the guest writes to IRS_IST_BASER we effectively cache the value
> of the VALID bit in the GICv5ISTConfig valid field entry.  In a few
> places we check whether the IST is valid by doing a bit extract on
> the register value; switch those to look at cfg->structure instead.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/arm_gicv5.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/intc/arm_gicv5.c b/hw/intc/arm_gicv5.c
> index 5d0a50ab6aa..d0a8574916b 100644
> --- a/hw/intc/arm_gicv5.c
> +++ b/hw/intc/arm_gicv5.c
> @@ -1222,8 +1222,7 @@ static void irs_map_l2_istr_write(GICv5 *s, GICv5Domain domain, uint64_t value)
>      uint64_t l1_iste;
>      MemTxResult res;
>  
> -    if (!FIELD_EX64(cs->irs_ist_baser[domain], IRS_IST_BASER, VALID) ||
> -        !cfg->structure) {
> +    if (!cfg->valid || !cfg->structure) {
>          /* WI if no IST set up or it is not 2-level */
>          return;
>      }
> @@ -1293,7 +1292,7 @@ static void irs_ist_baser_write(GICv5 *s, GICv5Domain domain, uint64_t value)
>  {
>      GICv5Common *cs = ARM_GICV5_COMMON(s);
>  
> -    if (FIELD_EX64(cs->irs_ist_baser[domain], IRS_IST_BASER, VALID)) {
> +    if (s->phys_lpi_config[domain].valid) {
>          /* If VALID is set, ADDR is RO and we can only update VALID */
>          bool valid = FIELD_EX64(value, IRS_IST_BASER, VALID);
>          if (valid) {
> @@ -1604,7 +1603,7 @@ static bool config_writel(GICv5 *s, GICv5Domain domain, hwaddr offset,
>                              deposit64(cs->irs_ist_baser[domain], 32, 32, data));
>          return true;
>      case A_IRS_IST_CFGR:
> -        if (FIELD_EX64(cs->irs_ist_baser[domain], IRS_IST_BASER, VALID)) {
> +        if (s->phys_lpi_config[domain].valid) {
>              qemu_log_mask(LOG_GUEST_ERROR,
>                            "guest tried to write IRS_IST_CFGR for %s config frame "
>                            "while IST_BASER.VALID set\n", domain_name[domain]);

That makes sense.

Reviewed-by: Jim MacArthur <jim.macarthur@linaro.org>

[PATCH for-11.2 2/4] hw/intc/arm_gicv5: Free memory in finalize

[Peter Maydell]

The GICv5 allocates memory and creates hashtables during realize and
as part of its operation.  Free those resources in finalize.

Since the GICv5 is created at QEMU startup and remains in existence
for the lifetime of the process, this isn't an in-practice leak,
only a theoretical one.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gicv5.c        | 11 +++++++++++
 hw/intc/arm_gicv5_common.c |  3 +++
 2 files changed, 14 insertions(+)

diff --git a/hw/intc/arm_gicv5.c b/hw/intc/arm_gicv5.c
index d0a8574916b..fd6456cd8aa 100644
--- a/hw/intc/arm_gicv5.c
+++ b/hw/intc/arm_gicv5.c
@@ -1970,6 +1970,17 @@ static void gicv5_init(Object *obj)
 
 static void gicv5_finalize(Object *obj)
 {
+    GICv5 *s = ARM_GICV5(obj);
+
+    for (int i = 0; i < NUM_GICV5_DOMAINS; i++) {
+        GHashTable *ht = s->phys_lpi_config[i].lpi_cache;
+
+        if (ht) {
+            g_hash_table_destroy(ht);
+        }
+
+        g_free(s->hppi[i]);
+    }
 }
 
 static void gicv5_class_init(ObjectClass *oc, const void *data)
diff --git a/hw/intc/arm_gicv5_common.c b/hw/intc/arm_gicv5_common.c
index 2920b11cc39..cfadfee4f05 100644
--- a/hw/intc/arm_gicv5_common.c
+++ b/hw/intc/arm_gicv5_common.c
@@ -112,6 +112,9 @@ static void gicv5_common_init(Object *obj)
 
 static void gicv5_common_finalize(Object *obj)
 {
+    GICv5Common *cs = ARM_GICV5_COMMON(obj);
+
+    g_free(cs->spi);
 }
 
 static void gicv5_common_realize(DeviceState *dev, Error **errp)
-- 
2.43.0

Re: [PATCH for-11.2 2/4] hw/intc/arm_gicv5: Free memory in finalize

[Jim MacArthur]

On Tue, Nov 03, 2026 at 05:26:14PM +0000, Peter Maydell wrote:
> The GICv5 allocates memory and creates hashtables during realize and
> as part of its operation.  Free those resources in finalize.
> 
> Since the GICv5 is created at QEMU startup and remains in existence
> for the lifetime of the process, this isn't an in-practice leak,
> only a theoretical one.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/arm_gicv5.c        | 11 +++++++++++
>  hw/intc/arm_gicv5_common.c |  3 +++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/hw/intc/arm_gicv5.c b/hw/intc/arm_gicv5.c
> index d0a8574916b..fd6456cd8aa 100644
> --- a/hw/intc/arm_gicv5.c
> +++ b/hw/intc/arm_gicv5.c
> @@ -1970,6 +1970,17 @@ static void gicv5_init(Object *obj)
>  
>  static void gicv5_finalize(Object *obj)
>  {
> +    GICv5 *s = ARM_GICV5(obj);
> +
> +    for (int i = 0; i < NUM_GICV5_DOMAINS; i++) {
> +        GHashTable *ht = s->phys_lpi_config[i].lpi_cache;
> +
> +        if (ht) {
> +            g_hash_table_destroy(ht);
> +        }
> +
> +        g_free(s->hppi[i]);
> +    }
>  }
>  
>  static void gicv5_class_init(ObjectClass *oc, const void *data)
> diff --git a/hw/intc/arm_gicv5_common.c b/hw/intc/arm_gicv5_common.c
> index 2920b11cc39..cfadfee4f05 100644
> --- a/hw/intc/arm_gicv5_common.c
> +++ b/hw/intc/arm_gicv5_common.c
> @@ -112,6 +112,9 @@ static void gicv5_common_init(Object *obj)
>  
>  static void gicv5_common_finalize(Object *obj)
>  {
> +    GICv5Common *cs = ARM_GICV5_COMMON(obj);
> +
> +    g_free(cs->spi);
>  }
>  

Reviewed-by: Jim MacArthur <jim.macarthur@linaro.org>

[PATCH for-11.2 3/4] target/arm: Only allow GICv5 on CPUs without AArch32 at EL1..3

[Peter Maydell]

We previously allowed the GICv5 on any 64-bit CPU, with a hack where
enabling the GICv5 silently disabled AArch32 for EL1..3.

Now that we have a separate 'max-v8' and 'max-v9' where max-v9
correctly doesn't implement AArch32 at EL1..3, we can fix the "when
do we expose the has_gcie property" condition so that we only allow
the GICv5 when AArch32 doesn't exist above EL0, and remove the hack
disabling AArch32.

We could instead have defined a new ARM_FEATURE_V9, but "allow the
feature to be present on CPUs where it doesn't actively break
anything" fits the way we've generally handled things in QEMU, and it
avoids having a new V9 feature bit that we only use for this one
thing and where it's not clear when we should be setting it for KVM,
hvf, etc accelerators.

In practice the only CPUs which will allow GICv5 now are max, max-v9
and a64fx.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu-features.h |  8 ++++++++
 target/arm/cpu.c          | 37 +++++++------------------------------
 2 files changed, 15 insertions(+), 30 deletions(-)

diff --git a/target/arm/cpu-features.h b/target/arm/cpu-features.h
index 96ec12f64f4..b0edec28378 100644
--- a/target/arm/cpu-features.h
+++ b/target/arm/cpu-features.h
@@ -1130,6 +1130,14 @@ static inline bool isar_feature_aa64_aa32_el3(const ARMISARegisters *id)
     return FIELD_EX64_IDREG(id, ID_AA64PFR0, EL3) >= 2;
 }
 
+static inline bool isar_feature_aa64_aa32_above_el0(const ARMISARegisters *id)
+{
+    /* True if any EL above EL0 has AArch32 support */
+    return isar_feature_aa64_aa32_el1(id) ||
+        isar_feature_aa64_aa32_el2(id) ||
+        isar_feature_aa64_aa32_el3(id);
+}
+
 static inline bool isar_feature_aa64_ras(const ARMISARegisters *id)
 {
     return FIELD_EX64_IDREG(id, ID_AA64PFR0, RAS) != 0;
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 71681447b21..dde03ffdd6f 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1596,8 +1596,13 @@ static void arm_cpu_post_init(Object *obj)
                                        &cpu->rvbar_prop,
                                        OBJ_PROP_FLAG_READWRITE);
 
-        /* We only allow GICv5 on a 64-bit v8 CPU */
-        if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
+        /*
+         * We only allow GICv5 on a CPU with no AArch32 support above EL0
+         * (which is typically a v9 CPU), because GICv5 defines no AArch32
+         * system registers to control it.
+         */
+        if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) &&
+            !cpu_isar_feature(aa64_aa32_above_el0, cpu)) {
             qdev_property_add_static(DEVICE(obj), &arm_cpu_has_gcie_property);
         }
     }
@@ -2274,34 +2279,6 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
 
     /* Report FEAT_GCIE in our ID registers if property was set */
     FIELD_DP64_IDREG(isar, ID_AA64PFR2, GCIE, cpu->has_gcie);
-    if (cpu_isar_feature(aa64_gcie, cpu)) {
-        if (!arm_feature(env, ARM_FEATURE_AARCH64)) {
-            /*
-             * We only create the have_gcie property for AArch64 CPUs,
-             * but the user might have tried aarch64=off with has_gcie=on.
-             */
-            error_setg(errp, "Cannot both enable has_gcie and disable aarch64");
-            return;
-        }
-
-        /*
-         * FEAT_GCIE implies Armv9, which implies no AArch32 above EL0.
-         * Usually we don't strictly insist on this kind of feature
-         * dependency, but in this case we enforce it, because the
-         * GICv5 CPU interface has no AArch32 versions of its system
-         * registers, so interrupts wouldn't work if we allowed AArch32
-         * in EL1 or above. Downgrade "AArch32 and AArch64" to "AArch64".
-         */
-        if (cpu_isar_feature(aa64_aa32_el3, cpu)) {
-            FIELD_DP64_IDREG(isar, ID_AA64PFR0, EL3, 1);
-        }
-        if (cpu_isar_feature(aa64_aa32_el2, cpu)) {
-            FIELD_DP64_IDREG(isar, ID_AA64PFR0, EL2, 1);
-        }
-        if (cpu_isar_feature(aa64_aa32_el1, cpu)) {
-            FIELD_DP64_IDREG(isar, ID_AA64PFR0, EL1, 1);
-        }
-    }
 
     if (cpu_isar_feature(aa64_mte, cpu)) {
         /*
-- 
2.43.0

Re: [PATCH for-11.2 3/4] target/arm: Only allow GICv5 on CPUs without AArch32 at EL1..3

[Philippe Mathieu-Daudé]

On 3/11/26 18:26, Peter Maydell wrote:
> We previously allowed the GICv5 on any 64-bit CPU, with a hack where
> enabling the GICv5 silently disabled AArch32 for EL1..3.
> 
> Now that we have a separate 'max-v8' and 'max-v9' where max-v9
> correctly doesn't implement AArch32 at EL1..3, we can fix the "when
> do we expose the has_gcie property" condition so that we only allow
> the GICv5 when AArch32 doesn't exist above EL0, and remove the hack
> disabling AArch32.
> 
> We could instead have defined a new ARM_FEATURE_V9, but "allow the
> feature to be present on CPUs where it doesn't actively break
> anything" fits the way we've generally handled things in QEMU, and it
> avoids having a new V9 feature bit that we only use for this one
> thing and where it's not clear when we should be setting it for KVM,
> hvf, etc accelerators.
> 
> In practice the only CPUs which will allow GICv5 now are max, max-v9
> and a64fx.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/cpu-features.h |  8 ++++++++
>   target/arm/cpu.c          | 37 +++++++------------------------------
>   2 files changed, 15 insertions(+), 30 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>

[PATCH for-11.2 4/4] hw/intc/arm_gicv5: Use correct type for value in lpi_cache_get_hppi()

[Peter Maydell]

lpi_cache_get_hppi() is a function we pass to g_hash_table_foreach()
to iterate over the LPI cache hashtable.  This hash table has keys
which are GINT_TO_POINTER(intid) and values which hold the L2 ISTE
value, allocated as "hashvalue = g_new(uint32_t, 1)".

The function is passed the key and value as type 'gpointer' and has
to cast them to the real types itself, but it incorrectly casts the
value pointer to a uint64_t*.  This works on little-endian hosts
(since we only read specific fields of the L2 ISTE using bitmasking),
but won't work on big-endian, and trips the clang sanitizer.

Fix the pointer type.

Cc: qemu-stable@nongnu.org
Fixes: a0562ada75e ("hw/intc/arm_gicv5: Calculate HPPI in the IRS")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gicv5.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/arm_gicv5.c b/hw/intc/arm_gicv5.c
index fd6456cd8aa..a362349e5ed 100644
--- a/hw/intc/arm_gicv5.c
+++ b/hw/intc/arm_gicv5.c
@@ -390,7 +390,7 @@ typedef struct GetHPPIUserData {
 static void lpi_cache_get_hppi(gpointer key, gpointer value, gpointer user_data)
 {
     uint64_t id = GPOINTER_TO_INT(key);
-    uint64_t l2_iste = *(uint64_t *)value;
+    uint64_t l2_iste = *(uint32_t *)value;
     uint32_t prio, iaffid;
     GetHPPIUserData *ud = user_data;
 
-- 
2.43.0

Re: [PATCH for-11.2 4/4] hw/intc/arm_gicv5: Use correct type for value in lpi_cache_get_hppi()

[Peter Maydell]

On Tue, 3 Nov 2026 at 17:26, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> lpi_cache_get_hppi() is a function we pass to g_hash_table_foreach()
> to iterate over the LPI cache hashtable.  This hash table has keys
> which are GINT_TO_POINTER(intid) and values which hold the L2 ISTE
> value, allocated as "hashvalue = g_new(uint32_t, 1)".
>
> The function is passed the key and value as type 'gpointer' and has
> to cast them to the real types itself, but it incorrectly casts the
> value pointer to a uint64_t*.  This works on little-endian hosts
> (since we only read specific fields of the L2 ISTE using bitmasking),
> but won't work on big-endian, and trips the clang sanitizer.
>
> Fix the pointer type.
>
> Cc: qemu-stable@nongnu.org
> Fixes: a0562ada75e ("hw/intc/arm_gicv5: Calculate HPPI in the IRS")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/arm_gicv5.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/intc/arm_gicv5.c b/hw/intc/arm_gicv5.c
> index fd6456cd8aa..a362349e5ed 100644
> --- a/hw/intc/arm_gicv5.c
> +++ b/hw/intc/arm_gicv5.c
> @@ -390,7 +390,7 @@ typedef struct GetHPPIUserData {
>  static void lpi_cache_get_hppi(gpointer key, gpointer value, gpointer user_data)
>  {
>      uint64_t id = GPOINTER_TO_INT(key);
> -    uint64_t l2_iste = *(uint64_t *)value;
> +    uint64_t l2_iste = *(uint32_t *)value;
>      uint32_t prio, iaffid;
>      GetHPPIUserData *ud = user_data;

Looking at this again we could also make the local variable
be uint32_t; but getting the type wrong on that doesn't matter.

-- PMM

Re: [PATCH for-11.2 4/4] hw/intc/arm_gicv5: Use correct type for value in lpi_cache_get_hppi()

[Philippe Mathieu-Daudé]

On 3/11/26 18:49, Peter Maydell wrote:
> On Tue, 3 Nov 2026 at 17:26, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> lpi_cache_get_hppi() is a function we pass to g_hash_table_foreach()
>> to iterate over the LPI cache hashtable.  This hash table has keys
>> which are GINT_TO_POINTER(intid) and values which hold the L2 ISTE
>> value, allocated as "hashvalue = g_new(uint32_t, 1)".
>>
>> The function is passed the key and value as type 'gpointer' and has
>> to cast them to the real types itself, but it incorrectly casts the
>> value pointer to a uint64_t*.  This works on little-endian hosts
>> (since we only read specific fields of the L2 ISTE using bitmasking),
>> but won't work on big-endian, and trips the clang sanitizer.
>>
>> Fix the pointer type.
>>
>> Cc: qemu-stable@nongnu.org
>> Fixes: a0562ada75e ("hw/intc/arm_gicv5: Calculate HPPI in the IRS")
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>   hw/intc/arm_gicv5.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/intc/arm_gicv5.c b/hw/intc/arm_gicv5.c
>> index fd6456cd8aa..a362349e5ed 100644
>> --- a/hw/intc/arm_gicv5.c
>> +++ b/hw/intc/arm_gicv5.c
>> @@ -390,7 +390,7 @@ typedef struct GetHPPIUserData {
>>   static void lpi_cache_get_hppi(gpointer key, gpointer value, gpointer user_data)
>>   {
>>       uint64_t id = GPOINTER_TO_INT(key);
>> -    uint64_t l2_iste = *(uint64_t *)value;
>> +    uint64_t l2_iste = *(uint32_t *)value;
>>       uint32_t prio, iaffid;
>>       GetHPPIUserData *ud = user_data;
> 
> Looking at this again we could also make the local variable
> be uint32_t; but getting the type wrong on that doesn't matter.

Should we use GUINT_TO_POINTER() when inserting in put_l2_iste()
and recover here with GPOINTER_TO_UINT()?

Re: [PATCH for-11.2 4/4] hw/intc/arm_gicv5: Use correct type for value in lpi_cache_get_hppi()

[Peter Maydell]

On Wed, 4 Nov 2026 at 11:24, Philippe Mathieu-Daudé
<philmd@oss.qualcomm.com> wrote:
>
> On 3/11/26 18:49, Peter Maydell wrote:
> > On Tue, 3 Nov 2026 at 17:26, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>
> >> lpi_cache_get_hppi() is a function we pass to g_hash_table_foreach()
> >> to iterate over the LPI cache hashtable.  This hash table has keys
> >> which are GINT_TO_POINTER(intid) and values which hold the L2 ISTE
> >> value, allocated as "hashvalue = g_new(uint32_t, 1)".
> >>
> >> The function is passed the key and value as type 'gpointer' and has
> >> to cast them to the real types itself, but it incorrectly casts the
> >> value pointer to a uint64_t*.  This works on little-endian hosts
> >> (since we only read specific fields of the L2 ISTE using bitmasking),
> >> but won't work on big-endian, and trips the clang sanitizer.
> >>
> >> Fix the pointer type.
> >>
> >> Cc: qemu-stable@nongnu.org
> >> Fixes: a0562ada75e ("hw/intc/arm_gicv5: Calculate HPPI in the IRS")
> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >> ---
> >>   hw/intc/arm_gicv5.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/hw/intc/arm_gicv5.c b/hw/intc/arm_gicv5.c
> >> index fd6456cd8aa..a362349e5ed 100644
> >> --- a/hw/intc/arm_gicv5.c
> >> +++ b/hw/intc/arm_gicv5.c
> >> @@ -390,7 +390,7 @@ typedef struct GetHPPIUserData {
> >>   static void lpi_cache_get_hppi(gpointer key, gpointer value, gpointer user_data)
> >>   {
> >>       uint64_t id = GPOINTER_TO_INT(key);
> >> -    uint64_t l2_iste = *(uint64_t *)value;
> >> +    uint64_t l2_iste = *(uint32_t *)value;
> >>       uint32_t prio, iaffid;
> >>       GetHPPIUserData *ud = user_data;
> >
> > Looking at this again we could also make the local variable
> > be uint32_t; but getting the type wrong on that doesn't matter.
>
> Should we use GUINT_TO_POINTER() when inserting in put_l2_iste()
> and recover here with GPOINTER_TO_UINT()?

You mean change the type of the hashtable value from "it's a pointer
to a uint32_t that was allocated" to "it's actually the value" ?
We could do that; we'd need to update L2_ISTE_Handle to always have
the actual l2_iste word and not the "maybe we have a pointer to it",
and put_l2_iste() would need to update the hashtable entry always
rather than only when the pending bit changes and we add/remove
an entry. But it would save us that allocate and free of a uint32_t.

The potential downside is that if we ever decided we wanted to use
the architectural IMPDEF option to put implementation-specific
metadata into the L2 ISTE we'd have to reverse that. (See spec
section 4.7.4 -- the ISTE is 4 bytes with no impdef metadata, but
if we wanted metadata then it would get bigger and no longer fit
as a GUINT_TO_POINTER().) But I don't really think it likely we'll
want to go down that route -- in the virtualization parts of the
spec there's scope and intention that the implementation puts metadata
into various in-guest-memory data structures, but for QEMU it's much
easier to put those in host memory instead,  and that's what I did.

I have a feeling that when I wrote this code I was partly under the
incorrect impression that L2_ISTE words were 64 bits (e.g. there's a
comment in get_l2_iste that mentions "the uint64_t pointed to"), and
while I fixed most of that confusion before eventually posting the
original GICv5 patchset, I left the "we have to allocate memory for
the value" part in, not noticing it wasn't really necessary any more.

For 11.2 I think we should stick with the simple fixup of the
type mixup here, though.

thanks
-- PMM

Re: [PATCH for-11.2 4/4] hw/intc/arm_gicv5: Use correct type for value in lpi_cache_get_hppi()

[Philippe Mathieu-Daudé]

On 4/11/26 12:46, Peter Maydell wrote:
> On Wed, 4 Nov 2026 at 11:24, Philippe Mathieu-Daudé
> <philmd@oss.qualcomm.com> wrote:
>>
>> On 3/11/26 18:49, Peter Maydell wrote:
>>> On Tue, 3 Nov 2026 at 17:26, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>
>>>> lpi_cache_get_hppi() is a function we pass to g_hash_table_foreach()
>>>> to iterate over the LPI cache hashtable.  This hash table has keys
>>>> which are GINT_TO_POINTER(intid) and values which hold the L2 ISTE
>>>> value, allocated as "hashvalue = g_new(uint32_t, 1)".
>>>>
>>>> The function is passed the key and value as type 'gpointer' and has
>>>> to cast them to the real types itself, but it incorrectly casts the
>>>> value pointer to a uint64_t*.  This works on little-endian hosts
>>>> (since we only read specific fields of the L2 ISTE using bitmasking),
>>>> but won't work on big-endian, and trips the clang sanitizer.
>>>>
>>>> Fix the pointer type.
>>>>
>>>> Cc: qemu-stable@nongnu.org
>>>> Fixes: a0562ada75e ("hw/intc/arm_gicv5: Calculate HPPI in the IRS")
>>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>>> ---
>>>>    hw/intc/arm_gicv5.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/intc/arm_gicv5.c b/hw/intc/arm_gicv5.c
>>>> index fd6456cd8aa..a362349e5ed 100644
>>>> --- a/hw/intc/arm_gicv5.c
>>>> +++ b/hw/intc/arm_gicv5.c
>>>> @@ -390,7 +390,7 @@ typedef struct GetHPPIUserData {
>>>>    static void lpi_cache_get_hppi(gpointer key, gpointer value, gpointer user_data)
>>>>    {
>>>>        uint64_t id = GPOINTER_TO_INT(key);
>>>> -    uint64_t l2_iste = *(uint64_t *)value;
>>>> +    uint64_t l2_iste = *(uint32_t *)value;
>>>>        uint32_t prio, iaffid;
>>>>        GetHPPIUserData *ud = user_data;
>>>
>>> Looking at this again we could also make the local variable
>>> be uint32_t; but getting the type wrong on that doesn't matter.
>>
>> Should we use GUINT_TO_POINTER() when inserting in put_l2_iste()
>> and recover here with GPOINTER_TO_UINT()?
> 
> You mean change the type of the hashtable value from "it's a pointer
> to a uint32_t that was allocated" to "it's actually the value" ?
> We could do that; we'd need to update L2_ISTE_Handle to always have
> the actual l2_iste word and not the "maybe we have a pointer to it",
> and put_l2_iste() would need to update the hashtable entry always
> rather than only when the pending bit changes and we add/remove
> an entry. But it would save us that allocate and free of a uint32_t.
> 
> The potential downside is that if we ever decided we wanted to use
> the architectural IMPDEF option to put implementation-specific
> metadata into the L2 ISTE we'd have to reverse that. (See spec
> section 4.7.4 -- the ISTE is 4 bytes with no impdef metadata, but
> if we wanted metadata then it would get bigger and no longer fit
> as a GUINT_TO_POINTER().) But I don't really think it likely we'll
> want to go down that route -- in the virtualization parts of the
> spec there's scope and intention that the implementation puts metadata
> into various in-guest-memory data structures, but for QEMU it's much
> easier to put those in host memory instead,  and that's what I did.
> 
> I have a feeling that when I wrote this code I was partly under the
> incorrect impression that L2_ISTE words were 64 bits (e.g. there's a
> comment in get_l2_iste that mentions "the uint64_t pointed to"), and
> while I fixed most of that confusion before eventually posting the
> original GICv5 patchset, I left the "we have to allocate memory for
> the value" part in, not noticing it wasn't really necessary any more.
> 
> For 11.2 I think we should stick with the simple fixup of the
> type mixup here, though.

OK. Preferably declaring the variable as uint32_t (local fixup):
Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>