Re: [PATCH kvm-unit-tests v3 4/8] arm/arm64: mmu: Stop mapping an assumed IO region

[Andrew Jones <drjones@redhat.com>]

On Thu, May 13, 2021 at 04:48:16PM +0100, Alexandru Elisei wrote:
> Hi Drew,
> 
> On 5/10/21 4:45 PM, Alexandru Elisei wrote:
> > Hi Drew,
> >
> > On 4/29/21 5:41 PM, Andrew Jones wrote:
> >> By providing a proper ioremap function, we can just rely on devices
> >> calling it for each region they need (as they already do) instead of
> >> mapping a big assumed I/O range. We don't require the MMU to be
> >> enabled at the time of the ioremap. In that case, we add the mapping
> >> to the identity map anyway. This allows us to call setup_vm after
> >> io_init. Why don't we just call setup_vm before io_init, I hear you
> >> ask? Well, that's because tests like sieve want to start with the MMU
> >> off, later call setup_vm, and all the while have working I/O. Some
> >> unit tests are just really demanding...
> >>
> >> While at it, ensure we map the I/O regions with XN (execute never),
> >> as suggested by Alexandru Elisei.
> > I got to thinking why this wasn't an issue before. Under KVM, device memory is not
> > usually mapped at stage 2, so any speculated reads wouldn't have reached memory at
> > all. The only way I imagine that happening if the user was running kvm-unit-tests
> > with a passthrough PCI device, which I don't think happens too often.
> >
> > But we cannot rely on devices not being mapped at stage 2 when running under EFI
> > (we're mapping them ourselves with ioremap), so I believe this is a good fix.
> >
> > Had another look at the patch, looks good to me:
> 
> While testing the series I discovered that this patch introduces a bug when
> running under kvmtool.
> 
> Here's the splat:
> 
> $ ./configure --vmm=kvmtool --earlycon=uart,mmio,0x1000000 --page-size=4K && make
> clean && make -j6 && ./vm run -c2 -m128 -f arm/micro-bench.flat
> [..]
>   # lkvm run --firmware arm/micro-bench.flat -m 128 -c 2 --name guest-6986
>   Info: Placing fdt at 0x80200000 - 0x80210000
> chr_testdev_init: chr-testdev: can't find a virtio-console
> Timer Frequency 24000000 Hz (Output in microseconds)
> 
> name                                    total ns                         avg
> ns            
> --------------------------------------------------------------------------------------------
> hvc                                 168674516.0                        
> 2573.0             
> Load address: 80000000
> PC: 80000128 PC offset: 128
> Unhandled exception ec=0x25 (DABT_EL1)
> Vector: 4 (el1h_sync)
> ESR_EL1:         96000006, ec=0x25 (DABT_EL1)
> FAR_EL1: 000000000a000008 (valid)
> Exception frame registers:
> pc : [<0000000080000128>] lr : [<000000008000cac8>] pstate: 800003c5
> sp : 000000008003ff90
> x29: 0000000000000000 x28: 0000000000000000
> x27: 00000011ada4d0c2 x26: 0000000000000000
> x25: 0000000080015978 x24: 0000000080015a90
> x23: 0000048c27394fff x22: 20c49ba5e353f7cf
> x21: 28f5c28f5c28f5c3 x20: 0000000080016af0
> x19: 000000e8d4a51000 x18: 0000000080040000
> x17: 0000000000000000 x16: 0000000080008210
> x15: 000000008003fe5c x14: 0000000000000260
> x13: 00000000000002a4 x12: 0000000080040000
> x11: 0000000000000001 x10: 0000000000000060
> x9 : 0000000000000000 x8 : 0000000000000039
> x7 : 0000000000000040 x6 : 0000000080013983
> x5 : 000000008003f74e x4 : 000000008003f69c
> x3 : 0000000000000000 x2 : 0000000000000000
> x1 : 0000000000000000 x0 : 000000000a000008
> 
> The issue is caused by the mmio_read_user_exec() function from arm/micro-bench.c.
> kvmtool doesn't have a chr-testdev device, and because probing fails, the address
> 0x0a000008 isn't ioremap'ed. The 0-1G memory region is not automatically mapped
> anymore, and the access triggers a data abort at stage 1.
> 
> I fixed the splat with this change:
> 
> diff --git a/arm/micro-bench.c b/arm/micro-bench.c
> index 95c418c10eb4..ad9e44d71d8d 100644
> --- a/arm/micro-bench.c
> +++ b/arm/micro-bench.c
> @@ -281,7 +281,7 @@ static void mmio_read_user_exec(void)
>          * updated in the future if any relevant changes in QEMU
>          * test-dev are made.
>          */
> -       void *userspace_emulated_addr = (void*)0x0a000008;
> +       void *userspace_emulated_addr = (void*)ioremap(0x0a000008, 8);
>  
>         readl(userspace_emulated_addr);
>  }
> 
> kvmtool ignores the MMIO exit reason if no device owns the IPA, that's why it also
> works on kvmtool.
> 
> The micro-bench test with the diff passes under qemu and kvmtool, tested with 4K,
> 16K and 64K pages on an odroid-c4.
>

Thanks Alex,

I think a better fix is this untested one below, though. If you can test
it out and confirm it also resolves the issue, then I'll add this patch
to the series.

Thanks,
drew


diff --git a/arm/micro-bench.c b/arm/micro-bench.c
index 95c418c10eb4..deafd5695c33 100644
--- a/arm/micro-bench.c
+++ b/arm/micro-bench.c
@@ -273,16 +273,22 @@ static void hvc_exec(void)
        asm volatile("mov w0, #0x4b000000; hvc #0" ::: "w0");
 }
 
-static void mmio_read_user_exec(void)
+/*
+ * FIXME: Read device-id in virtio mmio here in order to
+ * force an exit to userspace. This address needs to be
+ * updated in the future if any relevant changes in QEMU
+ * test-dev are made.
+ */
+static void *userspace_emulated_addr;
+
+static bool mmio_read_user_prep(void)
 {
-       /*
-        * FIXME: Read device-id in virtio mmio here in order to
-        * force an exit to userspace. This address needs to be
-        * updated in the future if any relevant changes in QEMU
-        * test-dev are made.
-        */
-       void *userspace_emulated_addr = (void*)0x0a000008;
+       userspace_emulated_addr = (void*)ioremap(0x0a000008, 8);
+       return true;
+}
 
+static void mmio_read_user_exec(void)
+{
        readl(userspace_emulated_addr);
 }
 
@@ -309,14 +315,14 @@ struct exit_test {
 };
 
 static struct exit_test tests[] = {
-       {"hvc",                 NULL,           hvc_exec,               NULL,           65536,          true},
-       {"mmio_read_user",      NULL,           mmio_read_user_exec,    NULL,           65536,          true},
-       {"mmio_read_vgic",      NULL,           mmio_read_vgic_exec,    NULL,           65536,          true},
-       {"eoi",                 NULL,           eoi_exec,               NULL,           65536,          true},
-       {"ipi",                 ipi_prep,       ipi_exec,               NULL,           65536,          true},
-       {"ipi_hw",              ipi_hw_prep,    ipi_exec,               NULL,           65536,          true},
-       {"lpi",                 lpi_prep,       lpi_exec,               NULL,           65536,          true},
-       {"timer_10ms",          timer_prep,     timer_exec,             timer_post,     256,            true},
+       {"hvc",                 NULL,                   hvc_exec,               NULL,           65536,          true},
+       {"mmio_read_user",      mmio_read_user_prep,    mmio_read_user_exec,    NULL,           65536,          true},
+       {"mmio_read_vgic",      NULL,                   mmio_read_vgic_exec,    NULL,           65536,          true},
+       {"eoi",                 NULL,                   eoi_exec,               NULL,           65536,          true},
+       {"ipi",                 ipi_prep,               ipi_exec,               NULL,           65536,          true},
+       {"ipi_hw",              ipi_hw_prep,            ipi_exec,               NULL,           65536,          true},
+       {"lpi",                 lpi_prep,               lpi_exec,               NULL,           65536,          true},
+       {"timer_10ms",          timer_prep,             timer_exec,             timer_post,     256,            true},
 };
 
 struct ns_time {