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

[Jianyong Wu <Jianyong.Wu@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Monday, May 18, 2020 11:06 PM
> To: Jianyong Wu <Jianyong.Wu@xxxxxxx>; minios-
> devel@xxxxxxxxxxxxxxxxxxxx; sharan.santhanam@xxxxxxxxx;
> simon.kuenzer@xxxxxxxxx
> Cc: Justin He <Justin.He@xxxxxxx>
> Subject: Re: [PATCH] arm64: rewrite io r/w helper to avoid kvm crash.
>
> 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/
>
I will fix all of these spelling and grammar fault.

> > 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).
>
Yeah, should not specify hyper mode here.

Thanks
Jianyong

> > + */
> > +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
IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.