[PATCH v2 5/5] arm: qemu: override flash accessors to use virtualizable instructions

[Ard Biesheuvel]

On Thu, 11 Jun 2020 at 10:17, Ard Biesheuvel <ardb at kernel.org> wrote:
>
> Some instructions in the ARM ISA have multiple output registers, such
> as ldrd/ldp (load pair), where two registers are loaded from memory,
> but also ldr with indexing, where the memory base register is incremented
> as well when the value is loaded to the destination register.
>
> MMIO emulation under KVM is based on using the architecturally defined
> syndrome information that is provided when an exception is taken to the
> hypervisor. This syndrome information describes whether the instruction
> that triggered the exception is a load or a store, what the faulting
> address was, and which register was the destination register.
>
> This syndrome information can only describe one destination register, and
> when the trapping instruction is one with multiple outputs, KVM throws an
> error like
>
>   kvm [615929]: Data abort outside memslots with no valid syndrome info
>
> on the host and kills the QEMU process with the following error:
>
>   U-Boot 2020.07-rc3-00208-g88bd5b179360-dirty (Jun 06 2020 - 11:59:22 +0200)
>
>   DRAM:  1 GiB
>   Flash: error: kvm run failed Function not implemented
>   R00=00000001 R01=00000040 R02=7ee0ce20 R03=00000000
>   R04=7ffd9eec R05=00000004 R06=7ffda3f8 R07=00000055
>   R08=7ffd9eec R09=7ef0ded0 R10=7ee0ce20 R11=00000000
>   R12=00000004 R13=7ee0cdf8 R14=00000000 R15=7ff72d08
>   PSR=200001d3 --C- A svc32
>   QEMU: Terminated
>
> This means that, in order to run U-Boot in QEMU under KVM, we need to
> avoid such instructions when accessing emulated devices. For the flash
> in particular, which is a hybrid between a ROM (backed by a read-only
> KVM memslot) when in array mode, and an emulated MMIO device (when in
> write mode), we need to take care to only use instructions that KVM can
> deal with when they trap.
>
> So override the flash read accessors that are used when running on QEMU
> under KVM. Note that the the 64-bit wide read accessors and all the write
> accessors have been omitted: they are either never used to begin with, or
> don't suffer from the MMIO emulation issue (as str instructions don't have
> multiple output registers)
>

This is inaccurate: str instructions do not have multiple output
registers, but they may have an output register (for pre/post
increment) as well as an input register (for the value being stored),
and this cannot be described by the syndrome information. So I will
need to respin this once more.


> Signed-off-by: Ard Biesheuvel <ardb at kernel.org>
> ---
>  board/emulation/qemu-arm/qemu-arm.c | 30 ++++++++++++++++++++
>  include/configs/qemu-arm.h          |  1 +
>  2 files changed, 31 insertions(+)
>
> diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c
> index 1b0d543b93c1..524e302001ec 100644
> --- a/board/emulation/qemu-arm/qemu-arm.c
> +++ b/board/emulation/qemu-arm/qemu-arm.c
> @@ -142,3 +142,33 @@ efi_status_t platform_get_rng_device(struct udevice **dev)
>         return EFI_SUCCESS;
>  }
>  #endif /* CONFIG_EFI_RNG_PROTOCOL */
> +
> +#ifdef CONFIG_ARM64
> +#define __W    "w"
> +#else
> +#define __W
> +#endif
> +
> +u8 flash_read8(void *addr)
> +{
> +       u8 ret;
> +
> +       asm("ldrb %" __W "0, %1" : "=r"(ret) : "m"(*(u8 *)addr));
> +       return ret;
> +}
> +
> +u16 flash_read16(void *addr)
> +{
> +       u16 ret;
> +
> +       asm("ldrh %" __W "0, %1" : "=r"(ret) : "m"(*(u16 *)addr));
> +       return ret;
> +}
> +
> +u32 flash_read32(void *addr)
> +{
> +       u32 ret;
> +
> +       asm("ldr %" __W "0, %1" : "=r"(ret) : "m"(*(u32 *)addr));
> +       return ret;
> +}
> diff --git a/include/configs/qemu-arm.h b/include/configs/qemu-arm.h
> index 1ef75a87836b..bc8b7c5c1238 100644
> --- a/include/configs/qemu-arm.h
> +++ b/include/configs/qemu-arm.h
> @@ -53,5 +53,6 @@
>  #define CONFIG_SYS_MAX_FLASH_BANKS     2
>  #endif
>  #define CONFIG_SYS_MAX_FLASH_SECT      256 /* Sector: 256K, Bank: 64M */
> +#define CONFIG_CFI_FLASH_USE_WEAK_ACCESSORS
>
>  #endif /* __CONFIG_H */
> --
> 2.26.2
>