[PATCH 0/2] KVM: arm64: Fix and test MMIO sign-extending loads

[PATCH 0/2] KVM: arm64: Fix and test MMIO sign-extending loads

[Fuad Tabba]

Hi folks,

A sign-extending load (LDRSB/LDRSH/LDRSW) from emulated MMIO returns a
zero-extended value rather than the sign-extended one the architecture
requires; vcpu_data_host_to_guest() strips the sign bits when it masks
the data to the access width.

If my git archeology is right, the masking dates to 2014 (b30070862edbd,
big-endian support) and has been wrong ever since, but sign-extending
loads from device memory are rare enough that nobody hit it. Patch 1
fixes it; patch 2 adds a selftest so it doesn't regress.

Cheers,
/fuad

Fuad Tabba (2):
  KVM: arm64: Fix sign-extension of MMIO loads
  KVM: arm64: selftests: Add MMIO sign-extending load test

 arch/arm64/kvm/mmio.c                         |   7 +-
 tools/testing/selftests/kvm/Makefile.kvm      |   1 +
 .../selftests/kvm/arm64/mmio_sign_ext.c       | 133 ++++++++++++++++++
 3 files changed, 138 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/arm64/mmio_sign_ext.c


base-commit: 8cd9520d35a6c38db6567e97dd93b1f11f185dc6
-- 
2.39.5

[PATCH 1/2] KVM: arm64: Fix sign-extension of MMIO loads

[Fuad Tabba]

A sign-extending load from MMIO (LDRSB, LDRSH, LDRSW) delivers a
zero-extended value: in kvm_handle_mmio_return(), vcpu_data_host_to_guest()
masks the data to the access width after sign-extension, stripping the
sign bits.

Move vcpu_data_host_to_guest() ahead of sign-extension so the width mask
runs first. trace_kvm_mmio() moves with it and keeps reporting the raw
access-width data.

Fixes: b30070862edbd ("ARM64: KVM: MMIO support BE host running LE code")
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
 arch/arm64/kvm/mmio.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/mmio.c b/arch/arm64/kvm/mmio.c
index e2285ed8c91de..d1c3a352d5a22 100644
--- a/arch/arm64/kvm/mmio.c
+++ b/arch/arm64/kvm/mmio.c
@@ -126,6 +126,10 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu)
 		len = kvm_vcpu_dabt_get_as(vcpu);
 		data = kvm_mmio_read_buf(run->mmio.data, len);
 
+		trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
+			       &data);
+		data = vcpu_data_host_to_guest(vcpu, data, len);
+
 		if (kvm_vcpu_dabt_issext(vcpu) &&
 		    len < sizeof(unsigned long)) {
 			mask = 1U << ((len * 8) - 1);
@@ -135,9 +139,6 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu)
 		if (!kvm_vcpu_dabt_issf(vcpu))
 			data = data & 0xffffffff;
 
-		trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
-			       &data);
-		data = vcpu_data_host_to_guest(vcpu, data, len);
 		vcpu_set_reg(vcpu, kvm_vcpu_dabt_get_rd(vcpu), data);
 	}
 
-- 
2.39.5

Re: [PATCH 1/2] KVM: arm64: Fix sign-extension of MMIO loads

[Marc Zyngier]

On Mon, 22 Jun 2026 20:07:00 +0100,
Fuad Tabba <fuad.tabba@linux.dev> wrote:
> 
> A sign-extending load from MMIO (LDRSB, LDRSH, LDRSW) delivers a
> zero-extended value: in kvm_handle_mmio_return(), vcpu_data_host_to_guest()
> masks the data to the access width after sign-extension, stripping the
> sign bits.
> 
> Move vcpu_data_host_to_guest() ahead of sign-extension so the width mask
> runs first. trace_kvm_mmio() moves with it and keeps reporting the raw
> access-width data.
> 
> Fixes: b30070862edbd ("ARM64: KVM: MMIO support BE host running LE code")
> Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
> ---
>  arch/arm64/kvm/mmio.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/mmio.c b/arch/arm64/kvm/mmio.c
> index e2285ed8c91de..d1c3a352d5a22 100644
> --- a/arch/arm64/kvm/mmio.c
> +++ b/arch/arm64/kvm/mmio.c
> @@ -126,6 +126,10 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu)
>  		len = kvm_vcpu_dabt_get_as(vcpu);
>  		data = kvm_mmio_read_buf(run->mmio.data, len);
>  
> +		trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
> +			       &data);
> +		data = vcpu_data_host_to_guest(vcpu, data, len);

This helper is in charge of the endianness of the read from the
guest's PoV. How can the sign-extension (which happens from the host's
perspective) correctly work if it takes place after the byte swapping?

To me, the real issue appears to be in the that helper, which swaps
the data based on the size of the access instead of the width of the
register.

Or am I once more completely confused with the endianness crap?

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 1/2] KVM: arm64: Fix sign-extension of MMIO loads

[Fuad Tabba]

On Tue, 23 Jun 2026 at 14:08, Marc Zyngier <maz@kernel.org> wrote:
>
> On Mon, 22 Jun 2026 20:07:00 +0100,
> Fuad Tabba <fuad.tabba@linux.dev> wrote:
> >
> > A sign-extending load from MMIO (LDRSB, LDRSH, LDRSW) delivers a
> > zero-extended value: in kvm_handle_mmio_return(), vcpu_data_host_to_guest()
> > masks the data to the access width after sign-extension, stripping the
> > sign bits.
> >
> > Move vcpu_data_host_to_guest() ahead of sign-extension so the width mask
> > runs first. trace_kvm_mmio() moves with it and keeps reporting the raw
> > access-width data.
> >
> > Fixes: b30070862edbd ("ARM64: KVM: MMIO support BE host running LE code")
> > Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
> > ---
> >  arch/arm64/kvm/mmio.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/mmio.c b/arch/arm64/kvm/mmio.c
> > index e2285ed8c91de..d1c3a352d5a22 100644
> > --- a/arch/arm64/kvm/mmio.c
> > +++ b/arch/arm64/kvm/mmio.c
> > @@ -126,6 +126,10 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu)
> >               len = kvm_vcpu_dabt_get_as(vcpu);
> >               data = kvm_mmio_read_buf(run->mmio.data, len);
> >
> > +             trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
> > +                            &data);
> > +             data = vcpu_data_host_to_guest(vcpu, data, len);
>
> This helper is in charge of the endianness of the read from the
> guest's PoV. How can the sign-extension (which happens from the host's
> perspective) correctly work if it takes place after the byte swapping?
>
> To me, the real issue appears to be in the that helper, which swaps
> the data based on the size of the access instead of the width of the
> register.
>
> Or am I once more completely confused with the endianness crap?

Endianess always confuses me too :D

My reading of the ARM ARM is that the byte reversal is keyed on the access
size there too. It lives in Mem{size}, with the register width handled
separately by SignExtend(regsize):

    data = Mem[address, 2];        // byte-reversed by the access size, BE
    X[t] = SignExtend(data, regsize);

So vcpu_data_host_to_guest(..., len) swapping by len matches the Mem-side
reversal. Swapping by the register width would reorder bytes that were never
loaded. An LDRSH into Wt reads 2 bytes but would bswap 4: the halfword
reaches the helper as 0x0180 host-native, cpu_to_be32 turns it into
0x80010000 instead of the 0x8001 cpu_to_be16 gives, and it never sign-extends
to 0xffff8001.

If that reading holds, none of the helper's ops are individually wrong, and
the only bug was the order, with the sign-extend running before the swap and
the width mask then dropping it. But I've gone round in circles on endianness
before (to say the least), so please say if I've done it again.

Cheers,
/fuad

>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

Re: [PATCH 1/2] KVM: arm64: Fix sign-extension of MMIO loads

[Marc Zyngier]

On Tue, 23 Jun 2026 14:51:49 +0100,
Fuad Tabba <fuad.tabba@linux.dev> wrote:
> 
> My reading of the ARM ARM is that the byte reversal is keyed on the access
> size there too. It lives in Mem{size}, with the register width handled
> separately by SignExtend(regsize):
> 
>     data = Mem[address, 2];        // byte-reversed by the access size, BE
>     X[t] = SignExtend(data, regsize);
> 
> So vcpu_data_host_to_guest(..., len) swapping by len matches the Mem-side
> reversal. Swapping by the register width would reorder bytes that were never
> loaded. An LDRSH into Wt reads 2 bytes but would bswap 4: the halfword
> reaches the helper as 0x0180 host-native, cpu_to_be32 turns it into
> 0x80010000 instead of the 0x8001 cpu_to_be16 gives, and it never sign-extends
> to 0xffff8001.
> 
> If that reading holds, none of the helper's ops are individually wrong, and
> the only bug was the order, with the sign-extend running before the swap and
> the width mask then dropping it. But I've gone round in circles on endianness
> before (to say the least), so please say if I've done it again.

That's quite convincing.

And the quoted pseudocode is much easier to reason about than the
current blurb in the commit message. For reference, J1.2.3.111 Mem{}()
is the relevant bit of the M.b spec and clearly shows that the access
is done LE, and only then byteswapped. Can you please repaint the
commit log to describe things in those terms?

Also, can you augment your test to cover for BE accesses from the
guest if the HW supports it?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 1/2] KVM: arm64: Fix sign-extension of MMIO loads

[Fuad Tabba]

On Thu, 25 Jun 2026 at 11:10, Marc Zyngier <maz@kernel.org> wrote:
>
> On Tue, 23 Jun 2026 14:51:49 +0100,
> Fuad Tabba <fuad.tabba@linux.dev> wrote:
> >
> > My reading of the ARM ARM is that the byte reversal is keyed on the access
> > size there too. It lives in Mem{size}, with the register width handled
> > separately by SignExtend(regsize):
> >
> >     data = Mem[address, 2];        // byte-reversed by the access size, BE
> >     X[t] = SignExtend(data, regsize);
> >
> > So vcpu_data_host_to_guest(..., len) swapping by len matches the Mem-side
> > reversal. Swapping by the register width would reorder bytes that were never
> > loaded. An LDRSH into Wt reads 2 bytes but would bswap 4: the halfword
> > reaches the helper as 0x0180 host-native, cpu_to_be32 turns it into
> > 0x80010000 instead of the 0x8001 cpu_to_be16 gives, and it never sign-extends
> > to 0xffff8001.
> >
> > If that reading holds, none of the helper's ops are individually wrong, and
> > the only bug was the order, with the sign-extend running before the swap and
> > the width mask then dropping it. But I've gone round in circles on endianness
> > before (to say the least), so please say if I've done it again.
>
> That's quite convincing.
>
> And the quoted pseudocode is much easier to reason about than the
> current blurb in the commit message. For reference, J1.2.3.111 Mem{}()
> is the relevant bit of the M.b spec and clearly shows that the access
> is done LE, and only then byteswapped. Can you please repaint the
> commit log to describe things in those terms?

Will do.

>
> Also, can you augment your test to cover for BE accesses from the
> guest if the HW supports it?

I'll see what I can come up with...

Cheers,
/fuad



>
> Thanks,
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

[PATCH 2/2] KVM: arm64: selftests: Add MMIO sign-extending load test

[Fuad Tabba]

Add a test for sign-extending MMIO loads (LDRSB, LDRSH, LDRSW) into Xt
and Wt destinations, with and without the sign bit set. The host supplies
the MMIO data and checks the guest register holds the sign-extended value.

Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
 tools/testing/selftests/kvm/Makefile.kvm      |   1 +
 .../selftests/kvm/arm64/mmio_sign_ext.c       | 133 ++++++++++++++++++
 2 files changed, 134 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/arm64/mmio_sign_ext.c

diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
index 9118a5a51b89f..0f5803a1092e1 100644
--- a/tools/testing/selftests/kvm/Makefile.kvm
+++ b/tools/testing/selftests/kvm/Makefile.kvm
@@ -171,6 +171,7 @@ TEST_GEN_PROGS_arm64 += arm64/hello_el2
 TEST_GEN_PROGS_arm64 += arm64/host_sve
 TEST_GEN_PROGS_arm64 += arm64/hypercalls
 TEST_GEN_PROGS_arm64 += arm64/external_aborts
+TEST_GEN_PROGS_arm64 += arm64/mmio_sign_ext
 TEST_GEN_PROGS_arm64 += arm64/page_fault_test
 TEST_GEN_PROGS_arm64 += arm64/psci_test
 TEST_GEN_PROGS_arm64 += arm64/sea_to_user
diff --git a/tools/testing/selftests/kvm/arm64/mmio_sign_ext.c b/tools/testing/selftests/kvm/arm64/mmio_sign_ext.c
new file mode 100644
index 0000000000000..d1efbccd1603a
--- /dev/null
+++ b/tools/testing/selftests/kvm/arm64/mmio_sign_ext.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * mmio_sign_ext - Test sign-extending MMIO load emulation (LDRSB/LDRSH/LDRSW)
+ *
+ * Copyright (c) 2026 Google LLC
+ */
+
+#include "processor.h"
+#include "test_util.h"
+
+#define MMIO_ADDR	0x8000000ULL
+
+struct mmio_test {
+	const char *name;
+	uint64_t data;
+	uint8_t len;
+	uint64_t expected;
+};
+
+/* Paired 1:1, in order, with the loads in guest_code() below. */
+static const struct mmio_test tests[] = {
+	/* LDRSB Xt: byte sign-extended to 64 bits */
+	{ "LDRSB Xt 0xFF",	0xFF,		1, 0xFFFFFFFFFFFFFFFFULL },
+	{ "LDRSB Xt 0x7F",	0x7F,		1, 0x7FULL },
+
+	/* LDRSB Wt: byte sign-extended to 32 bits, upper 32 bits zeroed */
+	{ "LDRSB Wt 0xFF",	0xFF,		1, 0xFFFFFFFFULL },
+	{ "LDRSB Wt 0x7F",	0x7F,		1, 0x7FULL },
+
+	/* LDRSH Xt: halfword sign-extended to 64 bits */
+	{ "LDRSH Xt 0x8001",	0x8001,		2, 0xFFFFFFFFFFFF8001ULL },
+	{ "LDRSH Xt 0x7FFF",	0x7FFF,		2, 0x7FFFULL },
+
+	/* LDRSH Wt: halfword sign-extended to 32 bits, upper 32 bits zeroed */
+	{ "LDRSH Wt 0x8001",	0x8001,		2, 0xFFFF8001ULL },
+	{ "LDRSH Wt 0x7FFF",	0x7FFF,		2, 0x7FFFULL },
+
+	/* LDRSW Xt: word sign-extended to 64 bits (no Wt form) */
+	{ "LDRSW Xt 0x80000001", 0x80000001,	4, 0xFFFFFFFF80000001ULL },
+	{ "LDRSW Xt 0x7FFFFFFF", 0x7FFFFFFF,	4, 0x7FFFFFFFULL },
+};
+
+static void guest_code(void)
+{
+	uint64_t val;
+
+	asm volatile("ldrsb %0, [%1]" : "=r"(val) : "r"(MMIO_ADDR) : "memory");
+	GUEST_SYNC(val);
+
+	asm volatile("ldrsb %0, [%1]" : "=r"(val) : "r"(MMIO_ADDR) : "memory");
+	GUEST_SYNC(val);
+
+	asm volatile("ldrsb %w0, [%1]" : "=r"(val) : "r"(MMIO_ADDR) : "memory");
+	GUEST_SYNC(val);
+
+	asm volatile("ldrsb %w0, [%1]" : "=r"(val) : "r"(MMIO_ADDR) : "memory");
+	GUEST_SYNC(val);
+
+	asm volatile("ldrsh %0, [%1]" : "=r"(val) : "r"(MMIO_ADDR) : "memory");
+	GUEST_SYNC(val);
+
+	asm volatile("ldrsh %0, [%1]" : "=r"(val) : "r"(MMIO_ADDR) : "memory");
+	GUEST_SYNC(val);
+
+	asm volatile("ldrsh %w0, [%1]" : "=r"(val) : "r"(MMIO_ADDR) : "memory");
+	GUEST_SYNC(val);
+
+	asm volatile("ldrsh %w0, [%1]" : "=r"(val) : "r"(MMIO_ADDR) : "memory");
+	GUEST_SYNC(val);
+
+	asm volatile("ldrsw %0, [%1]" : "=r"(val) : "r"(MMIO_ADDR) : "memory");
+	GUEST_SYNC(val);
+
+	asm volatile("ldrsw %0, [%1]" : "=r"(val) : "r"(MMIO_ADDR) : "memory");
+	GUEST_SYNC(val);
+
+	GUEST_DONE();
+}
+
+static void handle_mmio(struct kvm_run *run, const struct mmio_test *t)
+{
+	TEST_ASSERT_EQ(run->mmio.phys_addr, MMIO_ADDR);
+	TEST_ASSERT(!run->mmio.is_write, "Expected MMIO read for %s", t->name);
+	TEST_ASSERT_EQ(run->mmio.len, t->len);
+
+	memset(run->mmio.data, 0, sizeof(run->mmio.data));
+	/* Works because arm64 KVM hosts are always little-endian. */
+	memcpy(run->mmio.data, &t->data, t->len);
+}
+
+int main(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	struct ucall uc;
+	unsigned int i;
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
+	virt_map(vm, MMIO_ADDR, MMIO_ADDR, 1);
+
+	ksft_print_header();
+	ksft_set_plan(ARRAY_SIZE(tests));
+
+	for (i = 0; i < ARRAY_SIZE(tests); i++) {
+		const struct mmio_test *t = &tests[i];
+
+		vcpu_run(vcpu);
+		TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_MMIO);
+
+		handle_mmio(vcpu->run, t);
+		vcpu_run(vcpu);
+
+		switch (get_ucall(vcpu, &uc)) {
+		case UCALL_SYNC:
+			TEST_ASSERT_EQ(uc.args[1], t->expected);
+			break;
+		case UCALL_ABORT:
+			REPORT_GUEST_ASSERT(uc);
+			break;
+		default:
+			TEST_FAIL("Unexpected ucall for %s", t->name);
+		}
+
+		ksft_test_result_pass("%s\n", t->name);
+	}
+
+	vcpu_run(vcpu);
+	TEST_ASSERT(get_ucall(vcpu, &uc) == UCALL_DONE, "Expected UCALL_DONE");
+
+	kvm_vm_free(vm);
+
+	ksft_finished();
+}
-- 
2.39.5

Re: [PATCH 0/2] KVM: arm64: Fix and test MMIO sign-extending loads

[Oliver Upton]

On Mon, Jun 22, 2026 at 08:06:59PM +0100, Fuad Tabba wrote:
> Hi folks,
> 
> A sign-extending load (LDRSB/LDRSH/LDRSW) from emulated MMIO returns a
> zero-extended value rather than the sign-extended one the architecture
> requires; vcpu_data_host_to_guest() strips the sign bits when it masks
> the data to the access width.
> 
> If my git archeology is right, the masking dates to 2014 (b30070862edbd,
> big-endian support) and has been wrong ever since, but sign-extending
> loads from device memory are rare enough that nobody hit it. Patch 1
> fixes it; patch 2 adds a selftest so it doesn't regress.
> 
> Cheers,
> /fuad
> 
> Fuad Tabba (2):
>   KVM: arm64: Fix sign-extension of MMIO loads
>   KVM: arm64: selftests: Add MMIO sign-extending load test

For the oddball guest :)

Reviewed-by: Oliver Upton <oupton@kernel.org>

Thanks,
Oliver