On Mon, 19 Jan 2026 at 12:00, Alex Bennée <[email protected]> wrote:
>
> The GIC should always be a little-endian device as big-endian
> behaviour is a function of the current CPU configuration not the
> system as a whole. This allows us to keep the MSI data in plain host
> order rather then potentially truncating with multiple byte swaps of
> different sizes.
>
> Signed-off-by: Alex Bennée <[email protected]>
> ---
> hw/intc/arm_gicv3_its_common.c | 4 ++--
> hw/intc/arm_gicv3_its_kvm.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_its_common.c b/hw/intc/arm_gicv3_its_common.c
> index e946e3fb87b..60a5abd8d3e 100644
> --- a/hw/intc/arm_gicv3_its_common.c
> +++ b/hw/intc/arm_gicv3_its_common.c
> @@ -81,7 +81,7 @@ static MemTxResult gicv3_its_trans_write(void *opaque,
> hwaddr offset,
> if (offset == 0x0040 && ((size == 2) || (size == 4))) {
> GICv3ITSState *s = ARM_GICV3_ITS_COMMON(opaque);
> GICv3ITSCommonClass *c = ARM_GICV3_ITS_COMMON_GET_CLASS(s);
> - int ret = c->send_msi(s, le64_to_cpu(value), attrs.requester_id);
> + int ret = c->send_msi(s, value, attrs.requester_id);
>
> if (ret <= 0) {
> qemu_log_mask(LOG_GUEST_ERROR,
> diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
> index ae12d41eee1..a8d6d4fb540 100644
> --- a/hw/intc/arm_gicv3_its_kvm.c
> +++ b/hw/intc/arm_gicv3_its_kvm.c
> @@ -58,7 +58,7 @@ static int kvm_its_send_msi(GICv3ITSState *s, uint32_t
> value, uint16_t devid)
>
> msi.address_lo = extract64(s->gits_translater_gpa, 0, 32);
> msi.address_hi = extract64(s->gits_translater_gpa, 32, 32);
> - msi.data = le32_to_cpu(value);
> + msi.data = value;
> msi.flags = KVM_MSI_VALID_DEVID;
> msi.devid = devid;
> memset(msi.pad, 0, sizeof(msi.pad));
Having looked at this code a bit more carefully and figured out
that the send_msi method isn't calling core QEMU code but just
one bit of our ITS implementation talking to another, I agree
that these changes are correct:
* the kernel KVM_SIGNAL_MSI ioctl expects the data field to
be in host byte order
* the send_msi() method is entirely up to us how to
specify, and "host byte order" is the least surprising
one for a function call
* write methods of a MemoryRegionOps always get their
data in host byte order
So we should write this up in the commit message. This is
technically a bug fix (albeit one which only affects anybody
adventurous enough to run KVM on big-endian AArch64 :-))
so we could probably cc: stable too.
I also noticed while looking through the code that we
only seem to implement the send_msi method on the KVM
ITS. This works because TCG passes in its own MemoryRegionOps
for the "translation" MemoryRegion. But that means all
this base class code that calls a send_msi method on
the subclass is used only by KVM. We could tidy this up by:
* moving gicv3_its_trans_{read,write} from the base class
to the KVM subclass
* having gicv3_its_init_mmio() not have fallback code
for tops == NULL
* directly call kvm_its_send_msi() instead of indirecting
through a send_msi method
* remove the send_msi method pointer entirely
Second side note: kvm_irqchip_send_msi() in accel/kvm/kvm-all.c
also has a rather dodgy looking le32_to_cpu() call in it,
which is probably the inspiration for this one in the Arm code.
Luckily, we never use that function on Arm, and its only
callers are in always-LE-host targets. But all those callsites
seem to assume the msi.data field should be in host order
so this is probably something that somebody who cares about
x86 hosts might like to tidy up.
thanks
-- PMM
