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

[Max Filippov]

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.

-- 
Thanks.
-- Max