Message ID | 20221023153659.121138-5-tobias.roehmel@rwth-aachen.de |
---|---|
State | New |
Headers | show |
Series | Add ARM Cortex-R52 CPU | expand |
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. > diff --git a/target/arm/internals.h b/target/arm/internals.h > index 307a596505..e3699421b0 100644 > --- a/target/arm/internals.h > +++ b/target/arm/internals.h > @@ -253,6 +253,10 @@ unsigned int arm_pamax(ARMCPU *cpu); > static inline bool extended_addresses_enabled(CPUARMState *env) > { > uint64_t tcr = env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1]; > + if (arm_feature(env, ARM_FEATURE_PMSA) > + && arm_feature(env, ARM_FEATURE_V8)) { Indentation is a bit odd here too. > + return true; > + } > return arm_el_is_aa64(env, 1) || > (arm_feature(env, ARM_FEATURE_LPAE) && (tcr & TTBCR_EAE)); > } > diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c > index ad225b1cb2..a2047b0bc6 100644 > --- a/target/arm/tlb_helper.c > +++ b/target/arm/tlb_helper.c > @@ -18,6 +18,9 @@ bool regime_using_lpae_format(CPUARMState *env, ARMMMUIdx mmu_idx) > int el = regime_el(env, mmu_idx); > if (el == 2 || arm_el_is_aa64(env, el)) { > return true; > + } else if (arm_feature(env, ARM_FEATURE_PMSA) > + && arm_feature(env, ARM_FEATURE_V8)) { > + return true; > } Use an "if ()..." not an "else if" here to match the style of the other check in this function. > if (arm_feature(env, ARM_FEATURE_LPAE) > && (regime_tcr(env, mmu_idx) & TTBCR_EAE)) { Otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
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 :)
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
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)) { + using_lpae = true; } else { if (arm_feature(env, ARM_FEATURE_LPAE) && (env->cp15.tcr_el[target_el] & TTBCR_EAE)) { diff --git a/target/arm/internals.h b/target/arm/internals.h index 307a596505..e3699421b0 100644 --- a/target/arm/internals.h +++ b/target/arm/internals.h @@ -253,6 +253,10 @@ unsigned int arm_pamax(ARMCPU *cpu); static inline bool extended_addresses_enabled(CPUARMState *env) { uint64_t tcr = env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1]; + if (arm_feature(env, ARM_FEATURE_PMSA) + && arm_feature(env, ARM_FEATURE_V8)) { + return true; + } return arm_el_is_aa64(env, 1) || (arm_feature(env, ARM_FEATURE_LPAE) && (tcr & TTBCR_EAE)); } diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c index ad225b1cb2..a2047b0bc6 100644 --- a/target/arm/tlb_helper.c +++ b/target/arm/tlb_helper.c @@ -18,6 +18,9 @@ bool regime_using_lpae_format(CPUARMState *env, ARMMMUIdx mmu_idx) int el = regime_el(env, mmu_idx); if (el == 2 || arm_el_is_aa64(env, el)) { return true; + } else if (arm_feature(env, ARM_FEATURE_PMSA) + && arm_feature(env, ARM_FEATURE_V8)) { + return true; } if (arm_feature(env, ARM_FEATURE_LPAE) && (regime_tcr(env, mmu_idx) & TTBCR_EAE)) {