Re: {PATCH] accel/tcg: Fix CPU specific unaligned behaviour

["Alex Bennée" <alex.bennee@linaro.org>]

Helge Deller <deller@kernel.org> writes:

> When the emulated CPU reads or writes to a memory location
> a) for which no read/write permissions exists, *and*
> b) the access happens unaligned (non-natural alignment),
> then the CPU should either
> - trigger a permission fault, or
> - trigger an unalign access fault.
>
> In the current code the alignment check happens before the memory
> permission checks, so only unalignment faults will be triggered.
>
> This behaviour breaks the emulation of the PARISC architecture, where the CPU
> does a memory verification first. The behaviour can be tested with the testcase
> from the bugzilla report.
>
> Add the necessary code to allow PARISC and possibly other architectures to
> trigger a memory fault instead.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=219339
>
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 117b516739..dd1da358fb 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1684,6 +1684,26 @@ static void mmu_watch_or_dirty(CPUState *cpu, MMULookupPageData *data,
>      data->flags = flags;
>  }
>  
> +/* when accessing unreadable memory unaligned, will the CPU issue
> + * a alignment trap or a memory access trap ? */
> +#ifdef TARGET_HPPA
> +# define CPU_ALIGNMENT_CHECK_AFTER_MEMCHECK  1
> +#else
> +# define CPU_ALIGNMENT_CHECK_AFTER_MEMCHECK  0
> +#endif

I'm pretty certain we don't want to be introducing per-guest hacks into
the core cputlb.c code when we are aiming to make it a compile once
object.

I guess the real question is where could we put this flag? My gut says
we should expand the MO_ALIGN bits in MemOp to express the precedence or
not of the alignment check in relation to permissions.

> +
> +static void mmu_check_alignment(CPUState *cpu, vaddr addr,
> +                       uintptr_t ra, MMUAccessType type, MMULookupLocals *l)
> +{
> +    unsigned a_bits;
> +
> +    /* Handle CPU specific unaligned behaviour */
> +    a_bits = get_alignment_bits(l->memop);
> +    if (addr & ((1 << a_bits) - 1)) {
> +        cpu_unaligned_access(cpu, addr, type, l->mmu_idx, ra);
> +    }
> +}
> +
>  /**
>   * mmu_lookup: translate page(s)
>   * @cpu: generic cpu state
> @@ -1699,7 +1719,6 @@ static void mmu_watch_or_dirty(CPUState *cpu, MMULookupPageData *data,
>  static bool mmu_lookup(CPUState *cpu, vaddr addr, MemOpIdx oi,
>                         uintptr_t ra, MMUAccessType type, MMULookupLocals *l)
>  {
> -    unsigned a_bits;
>      bool crosspage;
>      int flags;
>  
> @@ -1708,10 +1727,8 @@ static bool mmu_lookup(CPUState *cpu, vaddr addr, MemOpIdx oi,
>  
>      tcg_debug_assert(l->mmu_idx < NB_MMU_MODES);
>  
> -    /* Handle CPU specific unaligned behaviour */
> -    a_bits = get_alignment_bits(l->memop);
> -    if (addr & ((1 << a_bits) - 1)) {
> -        cpu_unaligned_access(cpu, addr, type, l->mmu_idx, ra);
> +    if (!CPU_ALIGNMENT_CHECK_AFTER_MEMCHECK) {

Then this would be something like:

  if (!(memop & MO_ALIGN_PP)) or something

> +        mmu_check_alignment(cpu, addr, ra, type, l);
>      }
>  
>      l->page[0].addr = addr;
> @@ -1760,6 +1777,10 @@ static bool mmu_lookup(CPUState *cpu, vaddr addr, MemOpIdx oi,
>          tcg_debug_assert((flags & TLB_BSWAP) == 0);
>      }
>  
> +    if (CPU_ALIGNMENT_CHECK_AFTER_MEMCHECK) {
> +        mmu_check_alignment(cpu, addr, ra, type, l);
> +    }
> +
>      /*
>       * This alignment check differs from the one above, in that this is
>       * based on the atomicity of the operation. The intended use case is

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro