Re: [PATCH 16/27] accel/tcg: Unify cpu_{ld,st}*_{be,le}_mmu

[Richard Henderson <richard.henderson@linaro.org>]

On 5/21/23 23:39, Philippe Mathieu-Daudé wrote:
> On 21/5/23 17:00, Richard Henderson wrote:
>> On 5/21/23 04:15, Philippe Mathieu-Daudé wrote:
>>> Hi Richard,
>>>
>>> On 20/5/23 18:26, Richard Henderson wrote:
>>>> With the current structure of cputlb.c, there is no difference
>>>> between the little-endian and big-endian entry points, aside
>>>> from the assert.  Unify the pairs of functions.
>>>>
>>>> The only use of the functions with explicit endianness was in
>>>> target/sparc64, and that was only to satisfy the assert.
>>>
>>> I'm having hard time to follow all the handling of the various
>>> ASI definitions from target/sparc/asi.h. ...
>>>
>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>>> ---
>>>>   include/exec/cpu_ldst.h     |  58 ++-----
>>>>   accel/tcg/cputlb.c          | 122 +++-----------
>>>>   accel/tcg/user-exec.c       | 322 ++++++++++--------------------------
>>>>   target/arm/tcg/m_helper.c   |   4 +-
>>>>   target/sparc/ldst_helper.c  |  18 +-
>>>>   accel/tcg/ldst_common.c.inc |  24 +--
>>>>   6 files changed, 137 insertions(+), 411 deletions(-)
>>>
>>>
>>>> diff --git a/target/sparc/ldst_helper.c b/target/sparc/ldst_helper.c
>>>> index 7972d56a72..981a47d8bb 100644
>>>> --- a/target/sparc/ldst_helper.c
>>>> +++ b/target/sparc/ldst_helper.c
>>>> @@ -1334,25 +1334,13 @@ uint64_t helper_ld_asi(CPUSPARCState *env, target_ulong addr,
>>>
>>>
>>> Shouldn't we propagate the ASI endianness?
>>
>> Already done in translate, get_asi():
>>
>>          /* The little-endian asis all have bit 3 set.  */
>>          if (asi & 8) {
>>              memop ^= MO_BSWAP;
>>          }
> 
> Just in front of my eyes 🤦‍♂️ So:
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> Maybe amend the commit description "The ASI endianness is
> already taken care of in get_asi() ..."?

That's what I was trying to say with "only there to satisfy the assert". I have expanded 
on that a bit.


> While looking at get_asi(), ASI_FL16_* cases overwrite
> 'memop', possibly discarding MO_ALIGN bit. Maybe this can't
> happen.

Ah, that does look like a bug in one of my recent conversions.


r~