On Sat, 6 Jun 2020 at 23:08, Heinrich Schuchardt <xypron.g...@gmx.de> wrote:
>
> On 6/6/20 7:15 PM, Ard Biesheuvel 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 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 accessors that are used when running on QEMU.
> > Note that the write accessors are included for completeness, but the
> > read accessors are the ones that need this special care.
> >
> > Signed-off-by: Ard Biesheuvel <a...@kernel.org>
> > ---
> >  board/emulation/qemu-arm/qemu-arm.c | 55 ++++++++++++++++++++
> >  include/configs/qemu-arm.h          |  1 +
> >  2 files changed, 56 insertions(+)
> >
> > diff --git a/board/emulation/qemu-arm/qemu-arm.c 
> > b/board/emulation/qemu-arm/qemu-arm.c
> > index 1b0d543b93c1..32e18fd8b985 100644
> > --- a/board/emulation/qemu-arm/qemu-arm.c
> > +++ b/board/emulation/qemu-arm/qemu-arm.c
> > @@ -142,3 +142,58 @@ 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
> > +
> > +void flash_write8(u8 value, void *addr)
> > +{
> > +     asm("strb %" __W "1, %0" : "=m"(*(u8 *)addr) : "r"(value));
> > +}
> > +
> > +void flash_write16(u16 value, void *addr)
> > +{
> > +     asm("strh %" __W "1, %0" : "=m"(*(u16 *)addr) : "r"(value));
> > +}
> > +
> > +void flash_write32(u32 value, void *addr)
> > +{
> > +     asm("str %" __W "1, %0" : "=m"(*(u32 *)addr) : "r"(value));
> > +}
> > +
> > +void flash_write64(u64 value, void *addr)
> > +{
> > +     BUG(); /* FLASH_CFI_64BIT is not implemented by QEMU */
>
> Why should this BUG() on aarch64?

QEMU's CFI emulation does not implement 8 byte width, so there is no
point in implementing this accessor, as it will never be called
anyway.

The BUG() is there to ensure that *if* QEMU ever does get support for
8 byte wide CFI flash, we notice immediately, rather than having to
debug weird failures.

> Why is panicking in U-Boot better than crashing in QEMU?

Because U-boot crashes in the guest, while QEMU crashes in the host.

> Why can't this be realized as two 32bit writes?
>

It could be but there is no point: QEMU will never exercise this code
path anyway.

> This seems to be wrong:
> drivers/mtd/cfi_flash.c:179:
> /* No architectures currently implement __raw_readq() */
>
> I find definitions for all architectures.
>
> So shouldn't you correct cfi_flash.c and override __arch_getq() and
> __arch_putq() instead?
>

If anyone wants to fix 8 byte CFI, they are welcome to do so, but it
is a separate issue.

> > +}
> > +
> > +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;
> > +}
> > +
> > +u64 flash_read64(void *addr)
> > +{
> > +     BUG(); /* FLASH_CFI_64BIT is not implemented by QEMU */
>
> Same here.
>
> Best regards
>
> Heinrich
>
> > +}
> > 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 */
> >
>

Reply via email to