Re: [PATCH 6/6] target/arm: Implement FEAT_LPA2

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h       | 12 +++++++
>  target/arm/internals.h |  2 ++
>  target/arm/cpu64.c     |  2 ++
>  target/arm/helper.c    | 80 +++++++++++++++++++++++++++++++++++-------
>  4 files changed, 83 insertions(+), 13 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 3149000004..379585352b 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -4283,6 +4283,18 @@ static inline bool isar_feature_aa64_i8mm(const 
> ARMISARegisters *id)
>      return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, I8MM) != 0;
>  }
>  
> +static inline bool isar_feature_aa64_tgran4_lpa2(const ARMISARegisters *id)
> +{
> +    return sextract64(id->id_aa64mmfr0,
> +                      R_ID_AA64MMFR0_TGRAN4_SHIFT,
> +                      R_ID_AA64MMFR0_TGRAN4_LENGTH) >= 1;

Is this correct - it shows:

  0b1111 4KB granule not supported.

which I don't think implies FEAT_LPA2 because 1 indicates 4kb granule
supports 52 bit addressing. The other values are also reserved. Maybe it
would be safer just of == 1?

(a little more reading later)

  The ID_AA64MMFR0_EL1.TGran4_2, ID_AA64MMFR0_EL1.TGran16_2 and
  ID_AA64MMFR0_EL1.TGran64_2 fields that identify the memory translation stage 
2 granule size, do not follow
  the standard ID scheme. Software must treat these fields as follows:
  • The value 0x0 indicates that support is identified by another field.
  • If the field value is not 0x0, the value indicates the level of support 
provided.
  This means that software should use a test of the form:
  if (field !=0 and field > value) {
  // do something that relies on the value of the feature
  }

That's just confusing. It implies that you could see any of the TGran
fields set to zero but still support LPA2 if the other fields are set. I
think this at least warrants mentioning in an accompanying comments for
the function. 

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée