Re: [PATCH v4 0/4] KVM: arm64: BUG FIX: Correctly handle zero register transfers
On Tue, Dec 08, 2015 at 09:57:21AM +0300, Pavel Fedin wrote: > Hello! > > > I messed up the "load into xzr" test royally in the last attached patch. > > It was quite wrong. > > Yes, because "mov %0, xzr" is not trapped. > > > I have now tested > > > > asm volatile( > > "str %3, [%1]\n\t" > > "ldr wzr, [%1]\n\t" > > "str wzr, [%2]\n\t" > > "ldr %0, [%2]\n\t" > > :"=r"(val):"r"(addr), "r"(addr2), "r"(0x):"memory"); > > report("mmio: 'ldr wzr' check: read 0x%08lx", val != 0x, val); > > > > which passes > > I guess i forgot to mention that both addr and addr2 have to be MMIO > registers. If they are plain memory, then of course everything > will work because they are not trapped. Yes, my round two (which still didn't fail) used mmio for both addr and addr2. > > > Anyway, I > > probably won't clean this test up and post it. I don't think we really > > need to add it as a regression test, unless others disagree and would > > like to see it added. > > Considering how difficult it was to find this problem, and how tricky and > unobvious it is, i would ask to add this test. Especially > considering you've already written it. At least it will serve as a reminder > about the problem. OK. I need to wrap up some other work right now, but then I'll clean this patch up and send it properly. Thanks, drew > > Kind regards, > Pavel Fedin > Expert Engineer > Samsung Electronics Research center Russia > > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 0/4] KVM: arm64: BUG FIX: Correctly handle zero register transfers
Hello! > I messed up the "load into xzr" test royally in the last attached patch. > It was quite wrong. Yes, because "mov %0, xzr" is not trapped. > I have now tested > > asm volatile( > "str %3, [%1]\n\t" > "ldr wzr, [%1]\n\t" > "str wzr, [%2]\n\t" > "ldr %0, [%2]\n\t" > :"=r"(val):"r"(addr), "r"(addr2), "r"(0x):"memory"); > report("mmio: 'ldr wzr' check: read 0x%08lx", val != 0x, val); > > which passes I guess i forgot to mention that both addr and addr2 have to be MMIO registers. If they are plain memory, then of course everything will work because they are not trapped. > Anyway, I > probably won't clean this test up and post it. I don't think we really > need to add it as a regression test, unless others disagree and would > like to see it added. Considering how difficult it was to find this problem, and how tricky and unobvious it is, i would ask to add this test. Especially considering you've already written it. At least it will serve as a reminder about the problem. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/4] KVM: arm64: BUG FIX: Correctly handle zero register transfers
On Mon, Dec 07, 2015 at 04:36:31PM -0600, Andrew Jones wrote: > On Mon, Dec 07, 2015 at 11:36:28AM +0300, Pavel Fedin wrote: > > Hello! > > > > > FYI, I tried writing test cases for this issue with kvm-unit-tests. The > > > issue didn't reproduce for me. It's quite possible my test cases are > > > flawed, so I'm not making any claims about the validity of the series > > > > This is indeed very interesting, so i'll take a look at it. > > For now i've just only took a quick glance at the code, and i have at > > least one suggestion. Could you happen to have sp == 0 in > > check_xzr_sysreg()? In this case it will magically work. > > Also, you could try to write a test which tries to overwrite xzr. > > Something like: > > > > volatile int *addr1; > > volatile int *addr2; > > > > asm volatile("str %3, [%1]\n\t" > > "ldr wzr, [%1]\n\t" > > "str wzr, [%2]\n\t", > > "ldr %0, [%2]\n\t" > > :"=r"(res):"r"(addr1), "r"(addr2), > > "r"(some_nonzero_val):"memory"); > > > > Then check for res == some_nonzero_val. If they are equal, you've got the > > bug :) > > > > Besides the fixes mentioned in other mails, I did add this load to xzr > tests too. For mmio we get the expected failure. mrs seems to work > though, but maybe that's expected. > > qemu-system-aarch64 -machine virt,accel=kvm -cpu host \ > -device virtio-serial-device -device virtconsole,chardev=ctd \ > -chardev testdev,id=ctd -display none -serial stdio \ > -kernel arm/xzr-test.flat -smp 2 > > PASS: mmio: sanity check: read 0x > FAIL: mmio: 'str wzr' check: read 0x0badc0de > FAIL: mmio: 'ldr wzr' check: read 0x0badc0de > PASS: sysreg: sp = 0x401affe0 > FAIL: sysreg: from xzr check: read 0xc0de0badc0de > PASS: sysreg: to xzr check: read 0x > I messed up the "load into xzr" test royally in the last attached patch. It was quite wrong. I have now tested asm volatile( "str %3, [%1]\n\t" "ldr wzr, [%1]\n\t" "str wzr, [%2]\n\t" "ldr %0, [%2]\n\t" :"=r"(val):"r"(addr), "r"(addr2), "r"(0x):"memory"); report("mmio: 'ldr wzr' check: read 0x%08lx", val != 0x, val); which passes and val = readl(addr); printf("addr = 0x%08lx\n", val); val = readl(addr2); printf("addr2 = 0x%08lx\n", val); gives addr = 0x addr2 = 0x So it looks like we don't "change" xzr somehow with loads. Anyway, I probably won't clean this test up and post it. I don't think we really need to add it as a regression test, unless others disagree and would like to see it added. Thanks, drew -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/4] KVM: arm64: BUG FIX: Correctly handle zero register transfers
On Mon, Dec 07, 2015 at 11:36:28AM +0300, Pavel Fedin wrote: > Hello! > > > FYI, I tried writing test cases for this issue with kvm-unit-tests. The > > issue didn't reproduce for me. It's quite possible my test cases are > > flawed, so I'm not making any claims about the validity of the series > > This is indeed very interesting, so i'll take a look at it. > For now i've just only took a quick glance at the code, and i have at least > one suggestion. Could you happen to have sp == 0 in > check_xzr_sysreg()? In this case it will magically work. > Also, you could try to write a test which tries to overwrite xzr. Something > like: > > volatile int *addr1; > volatile int *addr2; > > asm volatile("str %3, [%1]\n\t" > "ldr wzr, [%1]\n\t" > "str wzr, [%2]\n\t", > "ldr %0, [%2]\n\t" > :"=r"(res):"r"(addr1), "r"(addr2), > "r"(some_nonzero_val):"memory"); > > Then check for res == some_nonzero_val. If they are equal, you've got the > bug :) > Besides the fixes mentioned in other mails, I did add this load to xzr tests too. For mmio we get the expected failure. mrs seems to work though, but maybe that's expected. qemu-system-aarch64 -machine virt,accel=kvm -cpu host \ -device virtio-serial-device -device virtconsole,chardev=ctd \ -chardev testdev,id=ctd -display none -serial stdio \ -kernel arm/xzr-test.flat -smp 2 PASS: mmio: sanity check: read 0x FAIL: mmio: 'str wzr' check: read 0x0badc0de FAIL: mmio: 'ldr wzr' check: read 0x0badc0de PASS: sysreg: sp = 0x401affe0 FAIL: sysreg: from xzr check: read 0xc0de0badc0de PASS: sysreg: to xzr check: read 0x SUMMARY: 6 tests, 3 unexpected failures Return value from qemu: 3 Updated test attached. drew >From ef5af811a72c14977e7958ee94b0c7b0fb99e6e8 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Fri, 4 Dec 2015 23:55:53 +0100 Subject: [kvm-unit-tests PATCH] arm64: add xzr emulator test --- v2: - added Pavel's fixes - changed target sysreg arm/xzr-test.c | 89 + config/config-arm64.mak | 4 ++- 2 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 arm/xzr-test.c diff --git a/arm/xzr-test.c b/arm/xzr-test.c new file mode 100644 index 0..cf92dcc2d4e00 --- /dev/null +++ b/arm/xzr-test.c @@ -0,0 +1,89 @@ +#include +#include +#include +#include +#include +#include +#include + + +static void check_xzr_sysreg(void) +{ + uint64_t val; + +#if 0 + flush_tlb_all(); + mmu_disable(); /* Tell KVM to set HCR_TVM for this VCPU */ +#endif + + val = current_stack_pointer; + report("sysreg: sp = 0x%016lx", val != 0, val); + + asm volatile("msr sp_el0, %0" : : "r" (0xdeadc0de0badc0de)); + isb(); + +#if 0 + asm volatile("msr ttbr0_el1, %0" : : "r" (0x & PAGE_MASK)); + isb(); + asm volatile("mrs %0, ttbr0_el1" : "=r" (val)); + isb(); + report("sysreg: sanity check: read 0x%016lx", val == (0x & PAGE_MASK), val); + + asm volatile("msr ttbr0_el1, xzr"); + isb(); + asm volatile("mrs %0, ttbr0_el1" : "=r" (val)); + isb(); + report("sysreg: xzr check: read 0x%016lx", val == 0, val); +#endif + asm volatile("msr dbgbvr0_el1, xzr"); + isb(); + asm volatile("mrs %0, dbgbvr0_el1" : "=r" (val)); + isb(); + report("sysreg: from xzr check: read 0x%016lx", val == 0, val); + asm volatile("mrs xzr, dbgbvr0_el1"); + isb(); + asm volatile("mov %0, xzr" : "=r" (val)); + report("sysreg: to xzr check: read 0x%016lx", val == 0, val); + + halt(); +} + +static uint32_t *steal_mmio_addr(void) +{ + /* +* Steal an MMIO addr from chr-testdev. Before calling exit() +* chr-testdev must be reinit. +*/ + return (uint32_t *)(0x0a003e00UL /* base */ + 0x40 /* queue pfn */); +} + +int main(void) +{ + volatile uint32_t *addr = steal_mmio_addr(); + uint32_t val; + long i; + + asm volatile("msr sp_el0, %0" : : "r" (0xdeadc0de0badc0de)); + isb(); + + writel(0x, addr); + val = readl(addr); + report("mmio: sanity check: read 0x%08lx", val == 0x, val); + + mb(); + asm volatile("str wzr, [%0]" : : "r" (addr)); + val = readl(addr); + report("mmio: 'str wzr' check: read 0x%08lx", val == 0, val); + mb(); + asm volatile("ldr wzr, [%0]" : : "r" (addr)); + report("mmio: 'ldr wzr' check: read 0x%08lx", val == 0, val); + + writel(0, addr); + chr_testdev_init(); + + smp_boot_secondary(1, check_xzr_sysreg); + for (i = 0; i < 10; ++i) + cpu_relax(); + + return report_summary(); +} diff --git a/config/config-arm64.mak b/config/config-arm64.mak index d61b703c8140e..65b355175f8a0 100644 --- a/config/config-arm64.mak +++ b/config/config-arm64.mak @@ -12,9 +12,11
Re: [PATCH v4 0/4] KVM: arm64: BUG FIX: Correctly handle zero register transfers
On Mon, Dec 07, 2015 at 03:58:11PM -0600, Andrew Jones wrote: > On Mon, Dec 07, 2015 at 12:48:12PM +0300, Pavel Fedin wrote: > > Hello! > > > > > FYI, I tried writing test cases for this issue with kvm-unit-tests. The > > > issue didn't reproduce for me. It's quite possible my test cases are > > > flawed > > > > Indeed they are, a very little thing fell through again... :) > > It's not just SP, it's SP_EL0. And you never initialize it to anything > > because your code always runs in kernel mode, so it's just > > zero, so you get your zero. > > But if you add a little thing in the beginning of your main(): > > > > asm volatile("msr sp_el0, %0" : : "r" (0xDEADC0DE0BADC0DE)); > > Ah! Thanks for this. The mmio test does now fail for me too. The sysreg > test still doesn't fail for me (even though I'm doing the above on the > vcpu I use for that too). Maybe there's something weird with which reg > I'm using, and whether or not my attempt to get trapping enabled on it > is working the way I expected. I'll play with it some more. > Must be the trapping thing. I switched to dbgbvr0_el1, which has trapping enabled on it until it's touched, and was able the reproduce the xzr issue it. drew -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/4] KVM: arm64: BUG FIX: Correctly handle zero register transfers
On Mon, Dec 07, 2015 at 12:48:12PM +0300, Pavel Fedin wrote: > Hello! > > > FYI, I tried writing test cases for this issue with kvm-unit-tests. The > > issue didn't reproduce for me. It's quite possible my test cases are > > flawed > > Indeed they are, a very little thing fell through again... :) > It's not just SP, it's SP_EL0. And you never initialize it to anything > because your code always runs in kernel mode, so it's just > zero, so you get your zero. > But if you add a little thing in the beginning of your main(): > > asm volatile("msr sp_el0, %0" : : "r" (0xDEADC0DE0BADC0DE)); Ah! Thanks for this. The mmio test does now fail for me too. The sysreg test still doesn't fail for me (even though I'm doing the above on the vcpu I use for that too). Maybe there's something weird with which reg I'm using, and whether or not my attempt to get trapping enabled on it is working the way I expected. I'll play with it some more. > > then you have it: > --- cut --- > [root@thunderx-2 kvm-unit-tests]# ./arm-run arm/xzr-test.flat -smp 2 > qemu-system-aarch64 -machine virt,accel=kvm:tcg,gic-version=host -cpu host > -device virtio-serial-device -device > virtconsole,chardev=ctd -chardev testdev,id=ctd -display none -serial stdio > -kernel arm/xzr-test.flat -smp 2 > PASS: mmio: sanity check: read 0x > FAIL: mmio: 'str wzr' check: read 0x0badc0de > vm_setup_vq: virtqueue 0 already setup! base=0xa003e00 > chr_testdev_init: chr-testdev: can't init virtqueues > --- cut --- > > Here i run only MMIO test, because i could not compile sysreg one, so i > simply commented it out. > > P.S. Could you also apply something like the following to arm/run: > --- cut --- > arm/run | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/arm/run b/arm/run > index 662a856..3890c8c 100755 > --- a/arm/run > +++ b/arm/run > @@ -33,7 +33,11 @@ if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \ > exit 2 > fi > > -M='-machine virt,accel=kvm:tcg' > +if $qemu $M,? 2>&1 | grep gic-version > /dev/null; then > + GIC='gic-version=host,' > +fi > + > +M="-machine virt,${GIC}accel=kvm:tcg" > chr_testdev='-device virtio-serial-device' > chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd' > --- cut --- Yes, I'll send a patch for this soon. I actually have something similar to this in my local tree already, I just hadn't bothered sending it as I didn't think anybody else needed it yet. > > Without it qemu does not work on GICv3-only hardware, like my board, because > it defaults to gic-version=2. I don't post the patch > on the mailing lists, because in order to be able to post this 5-liner i'll > need to go through the formal approval procedure at my > company, and i just don't want to bother for a single small fix. :) Will do > as a "Reported-by:". It'd be nice if you could go through the procedure. You've been sending patches to KVM, and ideally we'll start trying to send kvm-unit-tests patches along with feature patches. Thanks, drew -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/4] KVM: arm64: BUG FIX: Correctly handle zero register transfers
On Mon, Dec 07, 2015 at 11:47:44AM +0300, Pavel Fedin wrote: > Hello! > > > But, if Pavel doesn't > > mind trying them out on his system, then it'd be good to know if they > > reproduce there. I'd like to find out if it's a test case problem or > > something else strange going on with environments. > > Does not build, applied to master: > --- cut --- > aarch64-unknown-linux-gnu-gcc -std=gnu99 -ffreestanding -Wextra -O2 -I lib > -I lib/libfdt -g -MMD -MF arm/.xzr-test.d -Wall > -fomit-frame-pointer -fno-stack-protector -c -o arm/xzr-test.o > arm/xzr-test.c > arm/xzr-test.c: In function 'check_xzr_sysreg': > arm/xzr-test.c:13:2: warning: implicit declaration of function 'mmu_disable' > [-Wimplicit-function-declaration] > mmu_disable(); /* Tell KVM to set HCR_TVM for this VCPU */ > ^ > aarch64-unknown-linux-gnu-gcc -std=gnu99 -ffreestanding -Wextra -O2 -I lib > -I lib/libfdt -g -MMD -MF arm/.xzr-test.d -Wall > -fomit-frame-pointer -fno-stack-protector -nostdlib -o arm/xzr-test.elf \ > -Wl,-T,arm/flat.lds,--build-id=none,-Ttext=4008 \ > arm/xzr-test.o arm/cstart64.o lib/libcflat.a lib/libfdt/libfdt.a > /usr/lib/gcc/aarch64-unknown-linux-gnu/4.9.0/libgcc.a > lib/arm/libeabi.a > arm/xzr-test.o: In function `check_xzr_sysreg': > /cygdrive/d/Projects/kvm-unit-tests/arm/xzr-test.c:13: undefined reference to > `mmu_disable' > --- cut --- Have you done a git pull of your kvm-unit-tests repo lately? The patch that introduces mmu_disable was commit a few months ago or so. Other than your repo just not having mmu_disable(), then I can't think of why it compiles for me and not you. If you have done a recent git pull, then maybe do a 'make distclean; ./configure; make' Thanks, drew > > Kind regards, > Pavel Fedin > Expert Engineer > Samsung Electronics Research center Russia > > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 0/4] KVM: arm64: BUG FIX: Correctly handle zero register transfers
Hello! > FYI, I tried writing test cases for this issue with kvm-unit-tests. The > issue didn't reproduce for me. It's quite possible my test cases are > flawed Indeed they are, a very little thing fell through again... :) It's not just SP, it's SP_EL0. And you never initialize it to anything because your code always runs in kernel mode, so it's just zero, so you get your zero. But if you add a little thing in the beginning of your main(): asm volatile("msr sp_el0, %0" : : "r" (0xDEADC0DE0BADC0DE)); then you have it: --- cut --- [root@thunderx-2 kvm-unit-tests]# ./arm-run arm/xzr-test.flat -smp 2 qemu-system-aarch64 -machine virt,accel=kvm:tcg,gic-version=host -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -display none -serial stdio -kernel arm/xzr-test.flat -smp 2 PASS: mmio: sanity check: read 0x FAIL: mmio: 'str wzr' check: read 0x0badc0de vm_setup_vq: virtqueue 0 already setup! base=0xa003e00 chr_testdev_init: chr-testdev: can't init virtqueues --- cut --- Here i run only MMIO test, because i could not compile sysreg one, so i simply commented it out. P.S. Could you also apply something like the following to arm/run: --- cut --- arm/run | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/arm/run b/arm/run index 662a856..3890c8c 100755 --- a/arm/run +++ b/arm/run @@ -33,7 +33,11 @@ if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \ exit 2 fi -M='-machine virt,accel=kvm:tcg' +if $qemu $M,? 2>&1 | grep gic-version > /dev/null; then + GIC='gic-version=host,' +fi + +M="-machine virt,${GIC}accel=kvm:tcg" chr_testdev='-device virtio-serial-device' chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd' --- cut --- Without it qemu does not work on GICv3-only hardware, like my board, because it defaults to gic-version=2. I don't post the patch on the mailing lists, because in order to be able to post this 5-liner i'll need to go through the formal approval procedure at my company, and i just don't want to bother for a single small fix. :) Will do as a "Reported-by:". Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 0/4] KVM: arm64: BUG FIX: Correctly handle zero register transfers
Hello! > But, if Pavel doesn't > mind trying them out on his system, then it'd be good to know if they > reproduce there. I'd like to find out if it's a test case problem or > something else strange going on with environments. Does not build, applied to master: --- cut --- aarch64-unknown-linux-gnu-gcc -std=gnu99 -ffreestanding -Wextra -O2 -I lib -I lib/libfdt -g -MMD -MF arm/.xzr-test.d -Wall -fomit-frame-pointer -fno-stack-protector -c -o arm/xzr-test.o arm/xzr-test.c arm/xzr-test.c: In function 'check_xzr_sysreg': arm/xzr-test.c:13:2: warning: implicit declaration of function 'mmu_disable' [-Wimplicit-function-declaration] mmu_disable(); /* Tell KVM to set HCR_TVM for this VCPU */ ^ aarch64-unknown-linux-gnu-gcc -std=gnu99 -ffreestanding -Wextra -O2 -I lib -I lib/libfdt -g -MMD -MF arm/.xzr-test.d -Wall -fomit-frame-pointer -fno-stack-protector -nostdlib -o arm/xzr-test.elf \ -Wl,-T,arm/flat.lds,--build-id=none,-Ttext=4008 \ arm/xzr-test.o arm/cstart64.o lib/libcflat.a lib/libfdt/libfdt.a /usr/lib/gcc/aarch64-unknown-linux-gnu/4.9.0/libgcc.a lib/arm/libeabi.a arm/xzr-test.o: In function `check_xzr_sysreg': /cygdrive/d/Projects/kvm-unit-tests/arm/xzr-test.c:13: undefined reference to `mmu_disable' --- cut --- Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 0/4] KVM: arm64: BUG FIX: Correctly handle zero register transfers
Hello! > FYI, I tried writing test cases for this issue with kvm-unit-tests. The > issue didn't reproduce for me. It's quite possible my test cases are > flawed, so I'm not making any claims about the validity of the series This is indeed very interesting, so i'll take a look at it. For now i've just only took a quick glance at the code, and i have at least one suggestion. Could you happen to have sp == 0 in check_xzr_sysreg()? In this case it will magically work. Also, you could try to write a test which tries to overwrite xzr. Something like: volatile int *addr1; volatile int *addr2; asm volatile("str %3, [%1]\n\t" "ldr wzr, [%1]\n\t" "str wzr, [%2]\n\t", "ldr %0, [%2]\n\t" :"=r"(res):"r"(addr1), "r"(addr2), "r"(some_nonzero_val):"memory"); Then check for res == some_nonzero_val. If they are equal, you've got the bug :) Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/4] KVM: arm64: BUG FIX: Correctly handle zero register transfers
On Fri, Dec 04, 2015 at 03:03:10PM +0300, Pavel Fedin wrote: > ARM64 CPU has zero register which is read-only, with a value of 0. > However, KVM currently incorrectly recognizes it being SP (because > Rt == 31, and in struct user_pt_regs 'regs' array is followed by SP), > resulting in invalid value being read, or even SP corruption on write. > > The problem has been discovered by performing an operation > > *((volatile int *)reg) = 0; > > which compiles as "str xzr, [xx]", and resulted in strange values being > written. > > v3 => v4: > - Unwrapped assignment in patch 0003 > > v2 => v3: > - Brought back some const modifiers in unaffected functions > > v1 => v2: > - Changed type of transfer value to u64 and store it directly in > struct sys_reg_params instead of a pointer > - Use lower_32_bits()/upper_32_bits() where appropriate > - Fixed wrong usage of 'Rt' instead of 'Rt2' in kvm_handle_cp_64(), > overlooked in v1 > - Do not write value back when reading > > Pavel Fedin (4): > KVM: arm64: Correctly handle zero register during MMIO > KVM: arm64: Remove const from struct sys_reg_params > KVM: arm64: Correctly handle zero register in system register accesses > KVM: arm64: Get rid of old vcpu_reg() > FYI, I tried writing test cases for this issue with kvm-unit-tests. The issue didn't reproduce for me. It's quite possible my test cases are flawed, so I'm not making any claims about the validity of the series (I also see that it has already been acked and pulled). But, if Pavel doesn't mind trying them out on his system, then it'd be good to know if they reproduce there. I'd like to find out if it's a test case problem or something else strange going on with environments. kvm-unit-tests patch attached Thanks, drew >From 6576833b5e45801f0226316afae7daf0936a0aee Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Fri, 4 Dec 2015 23:55:53 +0100 Subject: [kvm-unit-tests PATCH] arm64: add xzr emulator test --- arm/xzr-test.c | 61 + config/config-arm64.mak | 4 +++- 2 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 arm/xzr-test.c diff --git a/arm/xzr-test.c b/arm/xzr-test.c new file mode 100644 index 0..77a11461c955c --- /dev/null +++ b/arm/xzr-test.c @@ -0,0 +1,61 @@ +#include +#include +#include +#include +#include +#include + +static void check_xzr_sysreg(void) +{ + uint64_t val; + + flush_tlb_all(); + mmu_disable(); /* Tell KVM to set HCR_TVM for this VCPU */ + + asm volatile("msr ttbr0_el1, %0" : : "r" (0x & PAGE_MASK)); + isb(); + asm volatile("mrs %0, ttbr0_el1" : "=r" (val)); + isb(); + report("sysreg: sanity check: read 0x%016lx", val == (0x & PAGE_MASK), val); + + asm volatile("msr ttbr0_el1, xzr"); + isb(); + asm volatile("mrs %0, ttbr0_el1" : "=r" (val)); + isb(); + report("sysreg: xzr check: read 0x%016lx", val == 0, val); + + halt(); +} + +static uint32_t *steal_mmio_addr(void) +{ + /* +* Steal an MMIO addr from chr-testdev. Before calling exit() +* chr-testdev must be reinit. +*/ + return (uint32_t *)(0x0a003e00UL /* base */ + 0x40 /* queue pfn */); +} + +int main(void) +{ + volatile uint32_t *addr = steal_mmio_addr(); + uint32_t val; + long i; + + writel(0x, addr); + val = readl(addr); + report("mmio: sanity check: read 0x%08lx", val == 0x, val); + + mb(); + asm volatile("str wzr, [%0]" : : "r" (addr)); + val = readl(addr); + report("mmio: 'str wzr' check: read 0x%08lx", val == 0, val); + + chr_testdev_init(); + + smp_boot_secondary(1, check_xzr_sysreg); + for (i = 0; i < 10; ++i) + cpu_relax(); + + return report_summary(); +} diff --git a/config/config-arm64.mak b/config/config-arm64.mak index d61b703c8140e..65b355175f8a0 100644 --- a/config/config-arm64.mak +++ b/config/config-arm64.mak @@ -12,9 +12,11 @@ cflatobjs += lib/arm64/processor.o cflatobjs += lib/arm64/spinlock.o # arm64 specific tests -tests = +tests = $(TEST_DIR)/xzr-test.flat include config/config-arm-common.mak arch_clean: arm_clean $(RM) lib/arm64/.*.d + +$(TEST_DIR)/xzr-test.elf: $(cstart.o) $(TEST_DIR)/xzr-test.o -- 1.8.3.1