Re: [PATCH] arm64: rewrite io r/w helper to avoid kvm crash.

[Julien Grall <julien@xxxxxxx>]

Hi,

On 15/05/2020 06:53, Jianyong Wu wrote:
> For now, unikraft will crash on arm64 in kvm mode if it compiled using
s/it compiled/it was compiled/

> optimize mode. Because compiler will choose post-index str intrs to
s/compiler/a compiler/ or the compiler.

s/intrs/instructions/

> write device memory. Unluckily, arm64 virtualization extension does only
> provides syndrome information for a limited subset of load/store
s/does only provides/only provides/

> instructions, that means we can't use load/store with writeback addressing
> mode to access mmio device.
> To avoid the compiler do those optimization, io read/write helpers should
s/do/to do/ I think.

> be rewitten by inline assembly with volatile constraint. After this,
s/by/using/

> unikraft can work fine on arm64.
>
> Signed-off-by: Jianyong Wu <jianyong.wu@xxxxxxx>
> ---
>   plat/common/include/arm/arm64/cpu.h | 84 ++++++++++++++++++++---------
>   1 file changed, 59 insertions(+), 25 deletions(-)
>
> diff --git a/plat/common/include/arm/arm64/cpu.h 
> b/plat/common/include/arm/arm64/cpu.h
> index 93ad13b..994d2f2 100644
> --- a/plat/common/include/arm/arm64/cpu.h
> +++ b/plat/common/include/arm/arm64/cpu.h
> @@ -41,31 +41,65 @@
>   #include <uk/alloc.h>
>   #include <uk/assert.h>
>   
> -/* Define macros to access IO registers */
> -#define __IOREG_READ(bits) \
> -static inline uint##bits##_t \
> -       ioreg_read##bits(const volatile uint##bits##_t *addr) \
> -               { return *addr; }
> -
> -#define __IOREG_WRITE(bits) \
> -static inline void \
> -       ioreg_write##bits(volatile uint##bits##_t *addr, \
> -                                               uint##bits##_t value) \
> -               { *addr = value; }
> -
> -
> -#define __IOREG_READ_ALL() __IOREG_READ(8)  \
> -                          __IOREG_READ(16) \
> -                          __IOREG_READ(32) \
> -                          __IOREG_READ(64) \
> -
> -#define __IOREG_WRITE_ALL()    __IOREG_WRITE(8)  \
> -                          __IOREG_WRITE(16) \
> -                          __IOREG_WRITE(32) \
> -                          __IOREG_WRITE(64) \
> -
> -__IOREG_READ_ALL()
> -__IOREG_WRITE_ALL()
> +/*
> + * we should use inline assembly with volatile constrain to access IO
s/constrain/constrainst/

> + * registers to avoid compiler use load/store intrns of writeback
> + * addressing mode which will cause crash when running in kvm mode.
This is not specific to KVM mode. This will happening on any hypervisor 
unless they decode the instruction (which is quite expensive).
> + */
> +static inline uint8_t ioreg_read8(const volatile uint8_t *address)
> +{
> +       uint8_t value;
> +
> +       asm volatile ("ldrb %w0, [%1]" : "=r"(value) : "r"(address));
> +       return value;
> +}
> +
> +static inline uint16_t ioreg_read16(const volatile uint16_t *address)
> +{
> +       uint16_t value;
> +
> +       asm volatile ("ldrh %w0, [%1]" : "=r"(value) : "r"(address));
> +       return value;
> +}
> +
> +static inline uint32_t ioreg_read32(const volatile uint32_t *address)
> +{
> +       uint32_t value;
> +
> +       asm volatile ("ldr %w0, [%1]" : "=r"(value) : "r"(address));
> +       return value;
> +}
> +
> +static inline uint64_t ioreg_read64(const volatile uint64_t *address)
> +{
> +       uint64_t value;
> +
> +       asm volatile ("ldr %0, [%1]" : "=r"(value) : "r"(address));
> +       return value;
> +}
> +
> +static inline void ioreg_write8(const volatile uint8_t *address, uint8_t value)
> +{
> +       asm volatile ("strb %w0, [%1]" : : "rZ"(value), "r"(address));
> +}
> +
> +static inline void ioreg_write16(const volatile uint16_t *address,
> +                                uint16_t value)
> +{
> +       asm volatile ("strh %w0, [%1]" : : "rZ"(value), "r"(address));
> +}
> +
> +static inline void ioreg_write32(const volatile uint32_t *address,
> +                                uint32_t value)
> +{
> +       asm volatile ("str %w0, [%1]" : : "rZ"(value), "r"(address));
> +}
> +
> +static inline void ioreg_write64(const volatile uint64_t *address,
> +                                uint64_t value)
> +{
> +       asm volatile ("str %0, [%1]" : : "rZ"(value), "r"(address));
> +}
NIT: Would it be possible to macro-ize this?

Regardless this question:

Reviewed-by: Julien Grall <julien@xxxxxxx>

Cheers,

--
Julien Grall