| Message ID | 20190114074855.16891-8-jcmvbkbc@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/xtensa: group helpers by functionality | expand |
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)? > + cpu_restore_state(CPU(cpu), retaddr, true); > + HELPER(exception_cause_vaddr)(env, > + env->pc, LOAD_STORE_ALIGNMENT_CAUSE, > + addr); > + } > +}
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
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,
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)); + assert(!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); } --- ? Thanks for the quick response btw :) Phil.
On 5/17/21 3:10 PM, Philippe Mathieu-Daudé 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: I meant 'IIUC' (if I understand correctly)... > -- >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)); > + assert(!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); > } > --- > > ? > > Thanks for the quick response btw :) > > Phil. >
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 and would happen on CPUs that have XTENSA_OPTION_HW_ALIGNMENT enabled. They could have a different CPUClass::tcg_ops, but I'm not sure it's worth it.
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.
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.
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 (:
diff --git a/target/xtensa/helper.c b/target/xtensa/helper.c index 2f1dec5c63e9..323c47a7fb54 100644 --- a/target/xtensa/helper.c +++ b/target/xtensa/helper.c @@ -29,10 +29,8 @@ #include "cpu.h" #include "exec/exec-all.h" #include "exec/gdbstub.h" +#include "exec/helper-proto.h" #include "qemu/host-utils.h" -#if !defined(CONFIG_USER_ONLY) -#include "hw/loader.h" -#endif static struct XtensaConfigList *xtensa_cores; @@ -188,6 +186,63 @@ int xtensa_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw, #else +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)) { + cpu_restore_state(CPU(cpu), retaddr, true); + HELPER(exception_cause_vaddr)(env, + env->pc, LOAD_STORE_ALIGNMENT_CAUSE, + addr); + } +} + +void tlb_fill(CPUState *cs, target_ulong vaddr, int size, + MMUAccessType access_type, int mmu_idx, uintptr_t retaddr) +{ + XtensaCPU *cpu = XTENSA_CPU(cs); + CPUXtensaState *env = &cpu->env; + uint32_t paddr; + uint32_t page_size; + unsigned access; + int ret = xtensa_get_physical_addr(env, true, vaddr, access_type, mmu_idx, + &paddr, &page_size, &access); + + qemu_log_mask(CPU_LOG_MMU, "%s(%08x, %d, %d) -> %08x, ret = %d\n", + __func__, vaddr, access_type, mmu_idx, paddr, ret); + + if (ret == 0) { + tlb_set_page(cs, + vaddr & TARGET_PAGE_MASK, + paddr & TARGET_PAGE_MASK, + access, mmu_idx, page_size); + } else { + cpu_restore_state(cs, retaddr, true); + HELPER(exception_cause_vaddr)(env, env->pc, ret, vaddr); + } +} + +void xtensa_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr, + unsigned size, MMUAccessType access_type, + int mmu_idx, MemTxAttrs attrs, + MemTxResult response, uintptr_t retaddr) +{ + XtensaCPU *cpu = XTENSA_CPU(cs); + CPUXtensaState *env = &cpu->env; + + cpu_restore_state(cs, retaddr, true); + HELPER(exception_cause_vaddr)(env, env->pc, + access_type == MMU_INST_FETCH ? + INSTR_PIF_ADDR_ERROR_CAUSE : + LOAD_STORE_PIF_ADDR_ERROR_CAUSE, + addr); +} + void xtensa_runstall(CPUXtensaState *env, bool runstall) { CPUState *cpu = CPU(xtensa_env_get_cpu(env)); diff --git a/target/xtensa/op_helper.c b/target/xtensa/op_helper.c index b0ef828f9ae5..1865f46c4b5f 100644 --- a/target/xtensa/op_helper.c +++ b/target/xtensa/op_helper.c @@ -37,62 +37,6 @@ #ifndef CONFIG_USER_ONLY -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)) { - cpu_restore_state(CPU(cpu), retaddr, true); - HELPER(exception_cause_vaddr)(env, - env->pc, LOAD_STORE_ALIGNMENT_CAUSE, addr); - } -} - -void tlb_fill(CPUState *cs, target_ulong vaddr, int size, - MMUAccessType access_type, int mmu_idx, uintptr_t retaddr) -{ - XtensaCPU *cpu = XTENSA_CPU(cs); - CPUXtensaState *env = &cpu->env; - uint32_t paddr; - uint32_t page_size; - unsigned access; - int ret = xtensa_get_physical_addr(env, true, vaddr, access_type, mmu_idx, - &paddr, &page_size, &access); - - qemu_log_mask(CPU_LOG_MMU, "%s(%08x, %d, %d) -> %08x, ret = %d\n", - __func__, vaddr, access_type, mmu_idx, paddr, ret); - - if (ret == 0) { - tlb_set_page(cs, - vaddr & TARGET_PAGE_MASK, - paddr & TARGET_PAGE_MASK, - access, mmu_idx, page_size); - } else { - cpu_restore_state(cs, retaddr, true); - HELPER(exception_cause_vaddr)(env, env->pc, ret, vaddr); - } -} - -void xtensa_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr, - unsigned size, MMUAccessType access_type, - int mmu_idx, MemTxAttrs attrs, - MemTxResult response, uintptr_t retaddr) -{ - XtensaCPU *cpu = XTENSA_CPU(cs); - CPUXtensaState *env = &cpu->env; - - cpu_restore_state(cs, retaddr, true); - HELPER(exception_cause_vaddr)(env, env->pc, - access_type == MMU_INST_FETCH ? - INSTR_PIF_ADDR_ERROR_CAUSE : - LOAD_STORE_PIF_ADDR_ERROR_CAUSE, - addr); -} - void HELPER(update_ccount)(CPUXtensaState *env) { uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
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(-)