| Message ID | 20200606171534.736365-6-ardb@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes for running U-boot under QEMU/KVM | expand |
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 <ardb at 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? Why is panicking in U-Boot better than crashing in QEMU? Why can't this be realized as two 32bit writes? 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? > +} > + > +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 */ >
On Sat, 6 Jun 2020 at 23:08, Heinrich Schuchardt <xypron.glpk at 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 <ardb at 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 */ > > >
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 */ +} + +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 */ +} 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 */
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 <ardb at kernel.org> --- board/emulation/qemu-arm/qemu-arm.c | 55 ++++++++++++++++++++ include/configs/qemu-arm.h | 1 + 2 files changed, 56 insertions(+)