On 4/11/26 12:46, Peter Maydell wrote:
On Wed, 4 Nov 2026 at 11:24, Philippe Mathieu-Daudé
<[email protected]> wrote:
On 3/11/26 18:49, Peter Maydell wrote:
On Tue, 3 Nov 2026 at 17:26, Peter Maydell <[email protected]> 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: [email protected]
Fixes: a0562ada75e ("hw/intc/arm_gicv5: Calculate HPPI in the IRS")
Signed-off-by: Peter Maydell <[email protected]>
---
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é <[email protected]>
