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. thanks -- PMM
