Re: [Qemu-devel] [PATCH 7/7] target/xtensa: move non-HELPER functions to helper.c

[Philippe Mathieu-Daudé]

On 5/17/21 5:35 PM, Max Filippov wrote:
> On Mon, May 17, 2021 at 8:25 AM Max Filippov <jcmvbkbc@gmail.com> wrote:
>>
>> On Mon, May 17, 2021 at 6:10 AM Philippe Mathieu-Daudé <f4bug@amsat.org> 
>> wrote:
>>>
>>> On 5/17/21 2:11 PM, Max Filippov wrote:
>>>> On Mon, May 17, 2021 at 4:50 AM Max Filippov <jcmvbkbc@gmail.com> wrote:
>>>>>
>>>>> Hi Philippe,
>>>>>
>>>>> On Sun, May 16, 2021 at 10:05 PM Philippe Mathieu-Daudé
>>>>> <philippe@mathieu-daude.net> wrote:
>>>>>>
>>>>>> Hi Max,
>>>>>>
>>>>>> On Mon, Jan 14, 2019 at 8:52 AM Max Filippov <jcmvbkbc@gmail.com> wrote:
>>>>>>>
>>>>>>> Move remaining non-HELPER functions from op_helper.c to helper.c.
>>>>>>> No functional changes.
>>>>>>>
>>>>>>> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
>>>>>>> ---
>>>>>>>  target/xtensa/helper.c    | 61 
>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++---
>>>>>>>  target/xtensa/op_helper.c | 56 
>>>>>>> -------------------------------------------
>>>>>>>  2 files changed, 58 insertions(+), 59 deletions(-)
>>>>>>
>>>>>>> +void xtensa_cpu_do_unaligned_access(CPUState *cs,
>>>>>>> +                                    vaddr addr, MMUAccessType 
>>>>>>> access_type,
>>>>>>> +                                    int mmu_idx, uintptr_t retaddr)
>>>>>>> +{
>>>>>>> +    XtensaCPU *cpu = XTENSA_CPU(cs);
>>>>>>> +    CPUXtensaState *env = &cpu->env;
>>>>>>> +
>>>>>>> +    if (xtensa_option_enabled(env->config, 
>>>>>>> XTENSA_OPTION_UNALIGNED_EXCEPTION) &&
>>>>>>> +        !xtensa_option_enabled(env->config, 
>>>>>>> XTENSA_OPTION_HW_ALIGNMENT)) {
>>>>>>
>>>>>> I know this is a simple code movement, but I wonder, what should
>>>>>> happen when there is
>>>>>> an unaligned fault and the options are disabled? Is this an impossible
>>>>>> case (unreachable)?
>>>>>
>>>>> It should be unreachable when XTENSA_OPTION_UNALIGNED_EXCEPTION
>>>>> is disabled. In that case the translation code generates access on aligned
>>>>> addresses according to the xtensa ISA, see the function
>>>>> gen_load_store_alignment in target/xtensa/translate.c
>>>>
>>>> There's also a case when both options are enabled, i.e. the
>>>> xtensa core has support for transparent unaligned access.
>>>> In that case the helper does nothing and the generic TCG
>>>> code is supposed to deal with the unaligned access correctly,
>>>
>>> IIRC we can simplify as:
>>>
>>> -- >8 --
>>> diff --git a/target/xtensa/helper.c b/target/xtensa/helper.c
>>> index eeffee297d1..6e8a6cdc99e 100644
>>> --- a/target/xtensa/helper.c
>>> +++ b/target/xtensa/helper.c
>>> @@ -270,13 +270,14 @@ void xtensa_cpu_do_unaligned_access(CPUState *cs,
>>>      XtensaCPU *cpu = XTENSA_CPU(cs);
>>>      CPUXtensaState *env = &cpu->env;
>>>
>>> -    if (xtensa_option_enabled(env->config,
>>> XTENSA_OPTION_UNALIGNED_EXCEPTION) &&
>>> -        !xtensa_option_enabled(env->config, XTENSA_OPTION_HW_ALIGNMENT)) {
>>> -        cpu_restore_state(CPU(cpu), retaddr, true);
>>> -        HELPER(exception_cause_vaddr)(env,
>>> -                                      env->pc, LOAD_STORE_ALIGNMENT_CAUSE,
>>> -                                      addr);
>>> -    }
>>> +    assert(xtensa_option_enabled(env->config,
>>> +                                 XTENSA_OPTION_UNALIGNED_EXCEPTION));
>>
>> This part -- yes.
>>
>>> +    assert(!xtensa_option_enabled(env->config,
>>> XTENSA_OPTION_HW_ALIGNMENT));
>>
>> This part -- no, because the call to the TCGCPUOps::do_unaligned_access
>> is unconditional
> 
> Oh, I've checked get_alignment_bits and now I see that it's conditional.
> This change can be done then, but the translation part also needs to be 
> changed
> to put MO_UNALN on cores with XTENSA_OPTION_HW_ALIGNMENT.

If you don't mind writing the patch, I'd prefer you do it because
you have a better understanding and will likely get it right, otherwise
I'll add it to my TODO and come back to it when other of my in-flight
series get merged :)

Regards,

Phil.