On Thu, May 13, 2021 at 07:18:44PM +0200, Andrew Jones wrote:
> 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 {
>
I still haven't tested it (beyond compiling), but I've tweaked this a bit.
You can see it here
https://gitlab.com/rhdrjones/kvm-unit-tests/-/commit/71938030d160e021db3388037d0d407df17e8e5e
The whole v4 of this series is here
https://gitlab.com/rhdrjones/kvm-unit-tests/-/commits/efiprep
Thanks,
drew
