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

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

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 2 Oct 2024 at 16:35, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> 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.
>
> There's also something curious going on here -- this patch
> says "we check alignment before permissions, and that's wrong
> on PARISC". But there's a comment in target/arm/ptw.c that
> says "we check permissions before alignment, and that's
> wrong on Arm":
>
>      * Enable alignment checks on Device memory.
>      *
>      * Per R_XCHFJ, this check is mis-ordered. The correct ordering
>      * for alignment, permission, and stage 2 faults should be:
>      *    - Alignment fault caused by the memory type
>      *    - Permission fault
>      *    - A stage 2 fault on the memory access
>      * but due to the way the TCG softmmu TLB operates, we will have
>      * implicitly done the permission check and the stage2 lookup in
>      * finding the TLB entry, so the alignment check cannot be done sooner.
>
> So do we check alignment first, or permissions first, or does
> the order vary depending on what we're doing?

If it varies by architecture and operation that is even more reason to
encode the wanted behaviour in the MemOp.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro