1. Patchew
  2. QEMU
  3. [PATCH v4 0/7] Add ARM Cortex-R52 CPU
  4. View patch

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

tobias.roehmel@rwth-aachen.de posted 7 patches 3 years, 3 months ago
There is a newer version of this series
[PATCH v4 4/7] target/arm: Enable TTBCR_EAE for ARMv8-R AArch32
Posted by tobias.roehmel@rwth-aachen.de 3 years, 3 months ago 
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)) {
+        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)) {
-- 
2.34.1
Re: [PATCH v4 4/7] target/arm: Enable TTBCR_EAE for ARMv8-R AArch32
Posted by Peter Maydell 3 years, 2 months ago 
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
Re: [PATCH v4 4/7] target/arm: Enable TTBCR_EAE for ARMv8-R AArch32
Posted by Philippe Mathieu-Daudé 3 years, 2 months ago 
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 :)
Re: [PATCH v4 4/7] target/arm: Enable TTBCR_EAE for ARMv8-R AArch32
Posted by Daniel P. Berrangé 3 years, 2 months ago 
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 :|

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.