Re: [PATCH v4 0/4] KVM: arm64: BUG FIX: Correctly handle zero register transfers

2015-12-08 Thread Andrew Jones 
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

2015-12-07 Thread Pavel Fedin 
 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

2015-12-07 Thread Pavel Fedin 
 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

2015-12-07 Thread Pavel Fedin 
 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

2015-12-07 Thread Pavel Fedin 
 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

2015-12-07 Thread Andrew Jones 
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

2015-12-07 Thread Andrew Jones 
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

2015-12-07 Thread Andrew Jones 
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

2015-12-07 Thread Andrew Jones 
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

2015-12-07 Thread Andrew Jones 
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
+++

Re: [PATCH v4 0/4] KVM: arm64: BUG FIX: Correctly handle zero register transfers

2015-12-04 Thread Andrew Jones 
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