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

[Daniel P . Berrangé]

On Tue, Nov 15, 2022 at 12:37:45PM +0100, Philippe Mathieu-Daudé wrote:
> 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.

This illustrates the problem with putting '&&' at start of line.
It harms the vertical alignment of the expressions, leading to
the desire to un-indent the block by 3 spaces to get 'arm_feature'
to line up. Understandable, but no editor/code indentors will
preserve this kind of indentation/alignment, so it shouldn't
be done.

Both ways you can choose to line up the indent for operator at
start of line are unpleasant - it is a no-win scenario IMHO.

> > > +        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.

I'm not convinced that operator at start of line makes any
difference at all to reviewability, and as above it leads
to undesirable indentation choices.

> 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?

The QEMU defacto standard is operators at end of line, given what
appears as the usage ratio of 6:1

$ git grep -E '^\s*(&&|&|\||\|\|)\s' '*.c' | wc -l
1692

$ git grep -E '\s(&&|&|\||\|\|)\s*$' '*.c' | wc -l
10289

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|