[PATCH] target/arm: Do alignment check when translation disabled

[PATCH] target/arm: Do alignment check when translation disabled

[Richard Henderson]

If translation is disabled, the default memory type is Device,
which requires alignment checking.  Document, but defer, the
more general case of per-page alignment checking.

Reported-by: Idan Horowitz <idan.horowitz@gmail.com>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1204
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.c | 38 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index d7bc467a2a..79609443aa 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10713,6 +10713,39 @@ ARMMMUIdx arm_mmu_idx(CPUARMState *env)
     return arm_mmu_idx_el(env, arm_current_el(env));
 }
 
+/*
+ * Return true if memory alignment should be enforced.
+ */
+static bool aprofile_require_alignment(CPUARMState *env, int el, uint64_t sctlr)
+{
+    /* Check the alignment enable bit. */
+    if (sctlr & SCTLR_A) {
+        return true;
+    }
+
+    /*
+     * If translation is disabled, then the default memory type
+     * may be Device(-nGnRnE) instead of Normal, which requires that
+     * alignment be enforced.
+     *
+     * TODO: The more general case is translation enabled, with a per-page
+     * check of the memory type as assigned via MAIR_ELx and the PTE.
+     * We could arrange for a bit in MemTxAttrs to enforce alignment
+     * via forced use of the softmmu slow path.  Given that such pages
+     * are intended for MMIO, where the slow path is required anyhow,
+     * this should not result in extra overhead.
+     */
+    if (sctlr & SCTLR_M) {
+        /* Translation enabled: memory type in PTE via MAIR_ELx. */
+        return false;
+    }
+    if (el < 2 && (arm_hcr_el2_eff(env) & (HCR_DC | HCR_VM))) {
+        /* Stage 2 translation enabled: memory type in PTE. */
+        return false;
+    }
+    return true;
+}
+
 static CPUARMTBFlags rebuild_hflags_common(CPUARMState *env, int fp_el,
                                            ARMMMUIdx mmu_idx,
                                            CPUARMTBFlags flags)
@@ -10777,8 +10810,9 @@ static CPUARMTBFlags rebuild_hflags_a32(CPUARMState *env, int fp_el,
 {
     CPUARMTBFlags flags = {};
     int el = arm_current_el(env);
+    uint64_t sctlr = arm_sctlr(env, el);
 
-    if (arm_sctlr(env, el) & SCTLR_A) {
+    if (aprofile_require_alignment(env, el, sctlr)) {
         DP_TBFLAG_ANY(flags, ALIGN_MEM, 1);
     }
 
@@ -10871,7 +10905,7 @@ static CPUARMTBFlags rebuild_hflags_a64(CPUARMState *env, int el, int fp_el,
 
     sctlr = regime_sctlr(env, stage1);
 
-    if (sctlr & SCTLR_A) {
+    if (aprofile_require_alignment(env, el, sctlr)) {
         DP_TBFLAG_ANY(flags, ALIGN_MEM, 1);
     }
 
-- 
2.34.1

[PULL 00/20] target-arm.next patch queue

[Richard Henderson]

A collection of arm-related patches that I collected while
Peter was on holiday.  There are some still outstanding that
I didn't feel comfortable collecting, such as cortex-r52.


r~


The following changes since commit 79dfa177ae348bb5ab5f97c0915359b13d6186e2:

  Merge tag 'pull-qapi-2022-09-07' of git://repo.or.cz/qemu/armbru into staging (2022-09-07 13:13:30 -0400)

are available in the Git repository at:

  https://gitlab.com/rth7680/qemu.git tags/pull-arm-20220914

for you to fetch changes up to 761c532ab1ebe9d345c9afe4fb9c2c4b26c58582:

  target/arm: Make boards pass base address to armv7m_load_kernel() (2022-09-14 11:19:40 +0100)

----------------------------------------------------------------
Add cortex-a35.
Fix bcm2835 framebuffer for rpi firmware.
Add FEAT_ETS.
Add FEAT_PMUv3p5.
Cleanups to armv7m_load_kernel.

----------------------------------------------------------------
Enrik Berkhan (1):
      hw/arm/bcm2835_property: Add support for RPI_FIRMWARE_FRAMEBUFFER_GET_NUM_DISPLAYS

Hao Wu (1):
      target/arm: Add cortex-a35

Peter Maydell (18):
      target/arm: Make cpregs 0, c0, c{3-15}, {0-7} correctly RAZ in v8
      target/arm: Sort KVM reads of AArch32 ID registers into encoding order
      target/arm: Implement ID_MMFR5
      target/arm: Implement ID_DFR1
      target/arm: Advertise FEAT_ETS for '-cpu max'
      target/arm: Add missing space in comment
      target/arm: Don't corrupt high half of PMOVSR when cycle counter overflows
      target/arm: Correct value returned by pmu_counter_mask()
      target/arm: Don't mishandle count when enabling or disabling PMU counters
      target/arm: Ignore PMCR.D when PMCR.LC is set
      target/arm: Honour MDCR_EL2.HPMD in Secure EL2
      target/arm: Detect overflow when calculating next PMU interrupt
      target/arm: Rename pmu_8_n feature test functions
      target/arm: Implement FEAT_PMUv3p5 cycle counter disable bits
      target/arm: Support 64-bit event counters for FEAT_PMUv3p5
      target/arm: Report FEAT_PMUv3p5 for TCG '-cpu max'
      target/arm: Remove useless TARGET_BIG_ENDIAN check in armv7m_load_kernel()
      target/arm: Make boards pass base address to armv7m_load_kernel()

 docs/system/arm/emulation.rst |   2 +
 docs/system/arm/virt.rst      |   1 +
 include/hw/arm/boot.h         |   5 +-
 target/arm/cpu.h              |  39 ++++--
 target/arm/internals.h        |   5 +-
 hw/arm/armv7m.c               |  14 +--
 hw/arm/aspeed.c               |   1 +
 hw/arm/microbit.c             |   2 +-
 hw/arm/mps2-tz.c              |   2 +-
 hw/arm/mps2.c                 |   2 +-
 hw/arm/msf2-som.c             |   2 +-
 hw/arm/musca.c                |   3 +-
 hw/arm/netduino2.c            |   2 +-
 hw/arm/netduinoplus2.c        |   2 +-
 hw/arm/stellaris.c            |   2 +-
 hw/arm/stm32vldiscovery.c     |   2 +-
 hw/arm/virt.c                 |   1 +
 hw/misc/bcm2835_property.c    |   4 +
 target/arm/cpu64.c            |  83 ++++++++++++-
 target/arm/cpu_tcg.c          |   8 +-
 target/arm/helper.c           | 267 ++++++++++++++++++++++++++++++++++--------
 target/arm/kvm64.c            |   8 +-
 22 files changed, 374 insertions(+), 83 deletions(-)

[PATCH] target/arm: Do alignment check when translation disabled

[Richard Henderson]

If translation is disabled, the default memory type is Device,
which requires alignment checking.  Document, but defer, the
more general case of per-page alignment checking.

Reported-by: Idan Horowitz <idan.horowitz@gmail.com>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1204
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.c | 38 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index d7bc467a2a..79609443aa 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10713,6 +10713,39 @@ ARMMMUIdx arm_mmu_idx(CPUARMState *env)
     return arm_mmu_idx_el(env, arm_current_el(env));
 }
 
+/*
+ * Return true if memory alignment should be enforced.
+ */
+static bool aprofile_require_alignment(CPUARMState *env, int el, uint64_t sctlr)
+{
+    /* Check the alignment enable bit. */
+    if (sctlr & SCTLR_A) {
+        return true;
+    }
+
+    /*
+     * If translation is disabled, then the default memory type
+     * may be Device(-nGnRnE) instead of Normal, which requires that
+     * alignment be enforced.
+     *
+     * TODO: The more general case is translation enabled, with a per-page
+     * check of the memory type as assigned via MAIR_ELx and the PTE.
+     * We could arrange for a bit in MemTxAttrs to enforce alignment
+     * via forced use of the softmmu slow path.  Given that such pages
+     * are intended for MMIO, where the slow path is required anyhow,
+     * this should not result in extra overhead.
+     */
+    if (sctlr & SCTLR_M) {
+        /* Translation enabled: memory type in PTE via MAIR_ELx. */
+        return false;
+    }
+    if (el < 2 && (arm_hcr_el2_eff(env) & (HCR_DC | HCR_VM))) {
+        /* Stage 2 translation enabled: memory type in PTE. */
+        return false;
+    }
+    return true;
+}
+
 static CPUARMTBFlags rebuild_hflags_common(CPUARMState *env, int fp_el,
                                            ARMMMUIdx mmu_idx,
                                            CPUARMTBFlags flags)
@@ -10777,8 +10810,9 @@ static CPUARMTBFlags rebuild_hflags_a32(CPUARMState *env, int fp_el,
 {
     CPUARMTBFlags flags = {};
     int el = arm_current_el(env);
+    uint64_t sctlr = arm_sctlr(env, el);
 
-    if (arm_sctlr(env, el) & SCTLR_A) {
+    if (aprofile_require_alignment(env, el, sctlr)) {
         DP_TBFLAG_ANY(flags, ALIGN_MEM, 1);
     }
 
@@ -10871,7 +10905,7 @@ static CPUARMTBFlags rebuild_hflags_a64(CPUARMState *env, int el, int fp_el,
 
     sctlr = regime_sctlr(env, stage1);
 
-    if (sctlr & SCTLR_A) {
+    if (aprofile_require_alignment(env, el, sctlr)) {
         DP_TBFLAG_ANY(flags, ALIGN_MEM, 1);
     }
 
-- 
2.34.1

Re: [PATCH] target/arm: Do alignment check when translation disabled

[Peter Maydell]

On Wed, 14 Sept 2022 at 13:47, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> If translation is disabled, the default memory type is Device,
> which requires alignment checking.  Document, but defer, the
> more general case of per-page alignment checking.
>
> Reported-by: Idan Horowitz <idan.horowitz@gmail.com>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1204
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/helper.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index d7bc467a2a..79609443aa 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -10713,6 +10713,39 @@ ARMMMUIdx arm_mmu_idx(CPUARMState *env)
>      return arm_mmu_idx_el(env, arm_current_el(env));
>  }
>
> +/*
> + * Return true if memory alignment should be enforced.
> + */
> +static bool aprofile_require_alignment(CPUARMState *env, int el, uint64_t sctlr)
> +{
> +    /* Check the alignment enable bit. */
> +    if (sctlr & SCTLR_A) {
> +        return true;
> +    }
> +
> +    /*
> +     * If translation is disabled, then the default memory type
> +     * may be Device(-nGnRnE) instead of Normal, which requires that

"may be" ?

> +     * alignment be enforced.
> +     *
> +     * TODO: The more general case is translation enabled, with a per-page
> +     * check of the memory type as assigned via MAIR_ELx and the PTE.
> +     * We could arrange for a bit in MemTxAttrs to enforce alignment
> +     * via forced use of the softmmu slow path.  Given that such pages
> +     * are intended for MMIO, where the slow path is required anyhow,
> +     * this should not result in extra overhead.
> +     */
> +    if (sctlr & SCTLR_M) {
> +        /* Translation enabled: memory type in PTE via MAIR_ELx. */
> +        return false;
> +    }
> +    if (el < 2 && (arm_hcr_el2_eff(env) & (HCR_DC | HCR_VM))) {
> +        /* Stage 2 translation enabled: memory type in PTE. */
> +        return false;
> +    }
> +    return true;

The SCTLR_EL1 docs say that if HCR_EL2.{DC,TGE} != {0,0} then we need to
treat SCTLR_EL1.M as if it is 0. DC is covered above, but do we need/want
to do anything special for TGE ? Maybe we just never get into this case
because TGE means regime_sctlr() is never SCTLR_EL1 ? I forget how it
works...

We also need to not do this for anything with ARM_FEATURE_PMSA :
with PMSA, if the MPU is disabled because SCTLR.M is 0 then the
default memory type depends on the address (it's defined by the
"default memory map", DDI0406C.d table B5-1) and isn't always Device.

We should also mention in the comment why we're doing this particular
special case even though we don't care to do full alignment checking
for Device memory accesses: because initial MMU-off code is a common
use-case where the guest will be working with RAM that's set up as
Device memory, and it's nice to be able to detect misaligned-access
bugs in it.

> +}
> +
>  static CPUARMTBFlags rebuild_hflags_common(CPUARMState *env, int fp_el,
>                                             ARMMMUIdx mmu_idx,
>                                             CPUARMTBFlags flags)
> @@ -10777,8 +10810,9 @@ static CPUARMTBFlags rebuild_hflags_a32(CPUARMState *env, int fp_el,
>  {
>      CPUARMTBFlags flags = {};
>      int el = arm_current_el(env);
> +    uint64_t sctlr = arm_sctlr(env, el);
>
> -    if (arm_sctlr(env, el) & SCTLR_A) {
> +    if (aprofile_require_alignment(env, el, sctlr)) {
>          DP_TBFLAG_ANY(flags, ALIGN_MEM, 1);
>      }
>
> @@ -10871,7 +10905,7 @@ static CPUARMTBFlags rebuild_hflags_a64(CPUARMState *env, int el, int fp_el,
>
>      sctlr = regime_sctlr(env, stage1);
>
> -    if (sctlr & SCTLR_A) {
> +    if (aprofile_require_alignment(env, el, sctlr)) {
>          DP_TBFLAG_ANY(flags, ALIGN_MEM, 1);
>      }
>
> --
> 2.34.1

thanks
-- PMM

Re: [PATCH] target/arm: Do alignment check when translation disabled

[Richard Henderson]

On 9/22/22 08:31, Peter Maydell wrote:
> On Wed, 14 Sept 2022 at 13:47, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> If translation is disabled, the default memory type is Device,
>> which requires alignment checking.  Document, but defer, the
>> more general case of per-page alignment checking.
>>
>> Reported-by: Idan Horowitz <idan.horowitz@gmail.com>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1204
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/arm/helper.c | 38 ++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index d7bc467a2a..79609443aa 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -10713,6 +10713,39 @@ ARMMMUIdx arm_mmu_idx(CPUARMState *env)
>>       return arm_mmu_idx_el(env, arm_current_el(env));
>>   }
>>
>> +/*
>> + * Return true if memory alignment should be enforced.
>> + */
>> +static bool aprofile_require_alignment(CPUARMState *env, int el, uint64_t sctlr)
>> +{
>> +    /* Check the alignment enable bit. */
>> +    if (sctlr & SCTLR_A) {
>> +        return true;
>> +    }
>> +
>> +    /*
>> +     * If translation is disabled, then the default memory type
>> +     * may be Device(-nGnRnE) instead of Normal, which requires that
> 
> "may be" ?

Indeed, weak wording: "is".

> 
>> +     * alignment be enforced.
>> +     *
>> +     * TODO: The more general case is translation enabled, with a per-page
>> +     * check of the memory type as assigned via MAIR_ELx and the PTE.
>> +     * We could arrange for a bit in MemTxAttrs to enforce alignment
>> +     * via forced use of the softmmu slow path.  Given that such pages
>> +     * are intended for MMIO, where the slow path is required anyhow,
>> +     * this should not result in extra overhead.

I have addressed this todo for v2.  It turns out to be quite easy.

> The SCTLR_EL1 docs say that if HCR_EL2.{DC,TGE} != {0,0} then we need to
> treat SCTLR_EL1.M as if it is 0. DC is covered above, but do we need/want
> to do anything special for TGE ? Maybe we just never get into this case
> because TGE means regime_sctlr() is never SCTLR_EL1 ? I forget how it
> works...

It might be, I'll double-check.

> We also need to not do this for anything with ARM_FEATURE_PMSA :
> with PMSA, if the MPU is disabled because SCTLR.M is 0 then the
> default memory type depends on the address (it's defined by the
> "default memory map", DDI0406C.d table B5-1) and isn't always Device.

Ok, thanks for the pointer.

> We should also mention in the comment why we're doing this particular
> special case even though we don't care to do full alignment checking
> for Device memory accesses: because initial MMU-off code is a common
> use-case where the guest will be working with RAM that's set up as
> Device memory, and it's nice to be able to detect misaligned-access
> bugs in it.

Without the todo, I guess this goes away?  I will have a comment about the difference 
between whole-address space vs per-page alignment checking.


r~