Re: [PATCH v4 4/7] target/arm: Enable TTBCR_EAE for ARMv8-R AArch32

[Philippe Mathieu-Daudé]

On 14/11/22 18:19, Peter Maydell wrote:
> On Sun, 23 Oct 2022 at 16:37, <tobias.roehmel@rwth-aachen.de> wrote:
> > From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
> >
> > ARMv8-R AArch32 CPUs behave as if TTBCR.EAE is always 1 even
> > tough they don't have the TTBCR register.
> > See ARM Architecture Reference Manual Supplement - ARMv8, for the ARMv8-R
> > AArch32 architecture profile Version:A.c section C1.2.
> >
> > Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
> > ---
> >   target/arm/debug_helper.c | 3 +++
> >   target/arm/internals.h    | 4 ++++
> >   target/arm/tlb_helper.c   | 3 +++
> >   3 files changed, 10 insertions(+)
> >
> > diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
> > index c21739242c..73665f988b 100644
> > --- a/target/arm/debug_helper.c
> > +++ b/target/arm/debug_helper.c
> > @@ -437,6 +437,9 @@ static uint32_t arm_debug_exception_fsr(CPUARMState *env)
> >
> >       if (target_el == 2 || arm_el_is_aa64(env, target_el)) {
> >           using_lpae = true;
> > +    } else if (arm_feature(env, ARM_FEATURE_PMSA)
> > +            && arm_feature(env, ARM_FEATURE_V8)) {
>
> Indentation looks wrong here. Generally the second line of a
> multiline if (...) condition starts in the column after the '(',
> so it lines up with the first part of the condition.
>
> > +        using_lpae = true;
> >       } else {
> >           if (arm_feature(env, ARM_FEATURE_LPAE) &&
> >               (env->cp15.tcr_el[target_el] & TTBCR_EAE)) {
>
> For instance this is an example in the existing code.
>
> We are inconsistent about whether we put operators like '&&' at
> the end of the first line or beginning of the second line, so
> pick whichever you like best, I guess.
Personally I find the operator at the end aesthetically nicer, but
few years ago Eric Blake reasoned that moving it at the beginning
was more explicit (to reviewers) thus safer, and I now I tend to
place it at the beginning.
Maybe part of the justification was cases where copy/pasting new
conditions in pre-existing could introduce a bug when the operator
is at the end?

+Eric/Daniel who usually give such good style advises :)