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 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