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

[Max Filippov]

On Mon, May 17, 2021 at 9:54 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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 :)

Almost done, will send it after some testing.
Thanks for drawing my attention to it (:

-- 
Thanks.
-- Max