[PATCH v3 0/6] target/arm: Do memory alignment check for device memory

[PATCH v3 0/6] target/arm: Do memory alignment check for device memory

[Richard Henderson]

v1: https://lore.kernel.org/qemu-devel/20230223204342.1093632-1-richard.henderson@linaro.org/
v2: https://lore.kernel.org/qemu-devel/20230621121902.1392277-1-richard.henderson@linaro.org/

Some of the v2 patch set was merged last June, but the rest languished.
No substantive changes from v2; only fixed rebase conflicts.


r~


Richard Henderson (6):
  target/arm: Support 32-byte alignment in pow2_align
  exec/memattrs: Remove target_tlb_bit*
  accel/tcg: Add tlb_fill_flags to CPUTLBEntryFull
  accel/tcg: Add TLB_CHECK_ALIGNED
  target/arm: Do memory type alignment check when translation disabled
  target/arm: Do memory type alignment check when translation enabled

 include/exec/cpu-all.h     |  4 +++-
 include/exec/memattrs.h    | 12 ------------
 include/hw/core/cpu.h      |  3 +++
 accel/tcg/cputlb.c         | 35 ++++++++++++++++++++++++++++-------
 target/arm/ptw.c           | 32 ++++++++++++++++++++++++++++++++
 target/arm/tcg/hflags.c    | 34 ++++++++++++++++++++++++++++++++--
 target/arm/tcg/translate.c |  8 +-------
 target/sparc/mmu_helper.c  |  2 +-
 8 files changed, 100 insertions(+), 30 deletions(-)

-- 
2.34.1

[PATCH v3 1/6] target/arm: Support 32-byte alignment in pow2_align

[Richard Henderson]

Now that we have removed TARGET_PAGE_BITS_MIN-6 from
TLB_FLAGS_MASK, we can test for 32-byte alignment.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/tcg/translate.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c
index f947c62c6b..c8a2470675 100644
--- a/target/arm/tcg/translate.c
+++ b/target/arm/tcg/translate.c
@@ -900,13 +900,7 @@ static inline void store_reg_from_load(DisasContext *s, int reg, TCGv_i32 var)
 MemOp pow2_align(unsigned i)
 {
     static const MemOp mop_align[] = {
-        0, MO_ALIGN_2, MO_ALIGN_4, MO_ALIGN_8, MO_ALIGN_16,
-        /*
-         * FIXME: TARGET_PAGE_BITS_MIN affects TLB_FLAGS_MASK such
-         * that 256-bit alignment (MO_ALIGN_32) cannot be supported:
-         * see get_alignment_bits(). Enforce only 128-bit alignment for now.
-         */
-        MO_ALIGN_16
+        0, MO_ALIGN_2, MO_ALIGN_4, MO_ALIGN_8, MO_ALIGN_16, MO_ALIGN_32
     };
     g_assert(i < ARRAY_SIZE(mop_align));
     return mop_align[i];
-- 
2.34.1

[PATCH v3 2/6] exec/memattrs: Remove target_tlb_bit*

[Richard Henderson]

These fields are no longer used since 937f224559.
Target specific extensions to the page tables should be done
with TARGET_PAGE_ENTRY_EXTRA.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/memattrs.h | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index d04170aa27..afa885f983 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -54,16 +54,6 @@ typedef struct MemTxAttrs {
     unsigned int requester_id:16;
     /* Invert endianness for this page */
     unsigned int byte_swap:1;
-    /*
-     * The following are target-specific page-table bits.  These are not
-     * related to actual memory transactions at all.  However, this structure
-     * is part of the tlb_fill interface, cached in the cputlb structure,
-     * and has unused bits.  These fields will be read by target-specific
-     * helpers using env->iotlb[mmu_idx][tlb_index()].attrs.target_tlb_bitN.
-     */
-    unsigned int target_tlb_bit0 : 1;
-    unsigned int target_tlb_bit1 : 1;
-    unsigned int target_tlb_bit2 : 1;
 } MemTxAttrs;
 
 /* Bus masters which don't specify any attributes will get this,
-- 
2.34.1

[PATCH v3 3/6] accel/tcg: Add tlb_fill_flags to CPUTLBEntryFull

[Richard Henderson]

Allow the target to set tlb flags to apply to all of the
comparators.  Remove MemTxAttrs.byte_swap, as the bit is
not relevant to memory transactions, only the page mapping.
Adjust target/sparc to set TLB_BSWAP directly.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/memattrs.h   | 2 --
 include/hw/core/cpu.h     | 3 +++
 accel/tcg/cputlb.c        | 5 +----
 target/sparc/mmu_helper.c | 2 +-
 4 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index afa885f983..14cdd8d582 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -52,8 +52,6 @@ typedef struct MemTxAttrs {
     unsigned int memory:1;
     /* Requester ID (for MSI for example) */
     unsigned int requester_id:16;
-    /* Invert endianness for this page */
-    unsigned int byte_swap:1;
 } MemTxAttrs;
 
 /* Bus masters which don't specify any attributes will get this,
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index af1a29526d..4c2fddc85b 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -230,6 +230,9 @@ typedef struct CPUTLBEntryFull {
     /* @lg_page_size contains the log2 of the page size. */
     uint8_t lg_page_size;
 
+    /* Additional tlb flags requested by tlb_fill. */
+    uint8_t tlb_fill_flags;
+
     /*
      * Additional tlb flags for use by the slow path. If non-zero,
      * the corresponding CPUTLBEntry comparator must have TLB_FORCE_SLOW.
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 047cd2cc0a..72d6a09616 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1145,14 +1145,11 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx,
               " prot=%x idx=%d\n",
               addr, full->phys_addr, prot, mmu_idx);
 
-    read_flags = 0;
+    read_flags = full->tlb_fill_flags;
     if (full->lg_page_size < TARGET_PAGE_BITS) {
         /* Repeat the MMU check and TLB fill on every access.  */
         read_flags |= TLB_INVALID_MASK;
     }
-    if (full->attrs.byte_swap) {
-        read_flags |= TLB_BSWAP;
-    }
 
     is_ram = memory_region_is_ram(section->mr);
     is_romd = memory_region_is_romd(section->mr);
diff --git a/target/sparc/mmu_helper.c b/target/sparc/mmu_helper.c
index 5170a668bb..e7b1997d54 100644
--- a/target/sparc/mmu_helper.c
+++ b/target/sparc/mmu_helper.c
@@ -580,7 +580,7 @@ static int get_physical_address_data(CPUSPARCState *env, CPUTLBEntryFull *full,
             int do_fault = 0;
 
             if (TTE_IS_IE(env->dtlb[i].tte)) {
-                full->attrs.byte_swap = true;
+                full->tlb_fill_flags |= TLB_BSWAP;
             }
 
             /* access ok? */
-- 
2.34.1

[PATCH v3 4/6] accel/tcg: Add TLB_CHECK_ALIGNED

[Richard Henderson]

This creates a per-page method for checking of alignment.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/cpu-all.h |  4 +++-
 accel/tcg/cputlb.c     | 30 +++++++++++++++++++++++++++---
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index bc05dce7ab..1a6510fd3b 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -357,8 +357,10 @@ static inline int cpu_mmu_index(CPUState *cs, bool ifetch)
 #define TLB_BSWAP            (1 << 0)
 /* Set if TLB entry contains a watchpoint.  */
 #define TLB_WATCHPOINT       (1 << 1)
+/* Set if TLB entry requires aligned accesses.  */
+#define TLB_CHECK_ALIGNED    (1 << 2)
 
-#define TLB_SLOW_FLAGS_MASK  (TLB_BSWAP | TLB_WATCHPOINT)
+#define TLB_SLOW_FLAGS_MASK  (TLB_BSWAP | TLB_WATCHPOINT | TLB_CHECK_ALIGNED)
 
 /* The two sets of flags must not overlap. */
 QEMU_BUILD_BUG_ON(TLB_FLAGS_MASK & TLB_SLOW_FLAGS_MASK);
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 72d6a09616..f1e4c23535 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1453,9 +1453,8 @@ static int probe_access_internal(CPUState *cpu, vaddr addr,
     flags |= full->slow_flags[access_type];
 
     /* Fold all "mmio-like" bits into TLB_MMIO.  This is not RAM.  */
-    if (unlikely(flags & ~(TLB_WATCHPOINT | TLB_NOTDIRTY))
-        ||
-        (access_type != MMU_INST_FETCH && force_mmio)) {
+    if (unlikely(flags & ~(TLB_WATCHPOINT | TLB_NOTDIRTY | TLB_CHECK_ALIGNED))
+        || (access_type != MMU_INST_FETCH && force_mmio)) {
         *phost = NULL;
         return TLB_MMIO;
     }
@@ -1836,6 +1835,31 @@ static bool mmu_lookup(CPUState *cpu, vaddr addr, MemOpIdx oi,
         tcg_debug_assert((flags & TLB_BSWAP) == 0);
     }
 
+    /*
+     * This alignment check differs from the one above, in that this is
+     * based on the atomicity of the operation. The intended use case is
+     * the ARM memory type field of each PTE, where access to pages with
+     * Device memory type require alignment.
+     */
+    if (unlikely(flags & TLB_CHECK_ALIGNED)) {
+        MemOp size = l->memop & MO_SIZE;
+
+        switch (l->memop & MO_ATOM_MASK) {
+        case MO_ATOM_NONE:
+            size = MO_8;
+            break;
+        case MO_ATOM_IFALIGN_PAIR:
+        case MO_ATOM_WITHIN16_PAIR:
+            size = size ? size - 1 : 0;
+            break;
+        default:
+            break;
+        }
+        if (addr & ((1 << size) - 1)) {
+            cpu_unaligned_access(cpu, addr, type, l->mmu_idx, ra);
+        }
+    }
+
     return crosspage;
 }
 
-- 
2.34.1

[PATCH v3 5/6] target/arm: Do memory type alignment check when translation disabled

[Richard Henderson]

If translation is disabled, the default memory type is Device, which
requires alignment checking.  This is more optimally done early via
the MemOp given to the TCG memory operation.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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/tcg/hflags.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/target/arm/tcg/hflags.c b/target/arm/tcg/hflags.c
index 8e5d35d922..5da1b0fc1d 100644
--- a/target/arm/tcg/hflags.c
+++ b/target/arm/tcg/hflags.c
@@ -26,6 +26,35 @@ static inline bool fgt_svc(CPUARMState *env, int el)
         FIELD_EX64(env->cp15.fgt_exec[FGTREG_HFGITR], HFGITR_EL2, SVC_EL1);
 }
 
+/* Return true if memory alignment should be enforced. */
+static bool aprofile_require_alignment(CPUARMState *env, int el, uint64_t sctlr)
+{
+#ifdef CONFIG_USER_ONLY
+    return false;
+#else
+    /* Check the alignment enable bit. */
+    if (sctlr & SCTLR_A) {
+        return true;
+    }
+
+    /*
+     * If translation is disabled, then the default memory type is
+     * Device(-nGnRnE) instead of Normal, which requires that alignment
+     * be enforced.  Since this affects all ram, it is most efficient
+     * to handle this during translation.
+     */
+    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;
+#endif
+}
+
 static CPUARMTBFlags rebuild_hflags_common(CPUARMState *env, int fp_el,
                                            ARMMMUIdx mmu_idx,
                                            CPUARMTBFlags flags)
@@ -121,8 +150,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);
     }
 
@@ -223,7 +253,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 v3 5/6] target/arm: Do memory type alignment check when translation disabled

[Jonathan Cameron via]

On Fri,  1 Mar 2024 10:41:09 -1000
Richard Henderson <richard.henderson@linaro.org> wrote:

> If translation is disabled, the default memory type is Device, which
> requires alignment checking.  This is more optimally done early via
> the MemOp given to the TCG memory operation.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 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>

Hi Richard.

I noticed some tests I was running stopped booting with master.
(it's a fun and complex stack of QEMU + kvm on QEMU for vCPU Hotplug kernel work,
but this is the host booting)

EDK2 build from upstream as of somepoint last week.

Bisects to this patch.

 qemu-system-aarch64 -M virt,gic-version=3,virtualization=true -m 4g,maxmem=8G,slots=8 -cpu cortex-a76 -smp cpus=4,threads=2,clusters=2,sockets=1 \
 -kernel Image \
 -drive if=none,file=full.qcow2,format=qcow2,id=hd \
 -device ioh3420,id=root_port1 -device virtio-blk-pci,drive=hd \
 -netdev user,id=mynet,hostfwd=tcp::5555-:22 -device virtio-net-pci,netdev=mynet,id=bob \
 -nographic -no-reboot -append 'earlycon root=/dev/vda2 fsck.mode=skip tp_printk' \
 -monitor telnet:127.0.0.1:1235,server,nowait -bios QEMU_EFI.fd \
 -object memory-backend-ram,size=4G,id=mem0 \
 -numa node,nodeid=0,cpus=0-3,memdev=mem0

Symptoms: Nothing on console from edk2 which is built in debug mode so is normally very noisy.
          No sign of anything much happening at all :(

Jonathan



> ---
>  target/arm/tcg/hflags.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/tcg/hflags.c b/target/arm/tcg/hflags.c
> index 8e5d35d922..5da1b0fc1d 100644
> --- a/target/arm/tcg/hflags.c
> +++ b/target/arm/tcg/hflags.c
> @@ -26,6 +26,35 @@ static inline bool fgt_svc(CPUARMState *env, int el)
>          FIELD_EX64(env->cp15.fgt_exec[FGTREG_HFGITR], HFGITR_EL2, SVC_EL1);
>  }
>  
> +/* Return true if memory alignment should be enforced. */
> +static bool aprofile_require_alignment(CPUARMState *env, int el, uint64_t sctlr)
> +{
> +#ifdef CONFIG_USER_ONLY
> +    return false;
> +#else
> +    /* Check the alignment enable bit. */
> +    if (sctlr & SCTLR_A) {
> +        return true;
> +    }
> +
> +    /*
> +     * If translation is disabled, then the default memory type is
> +     * Device(-nGnRnE) instead of Normal, which requires that alignment
> +     * be enforced.  Since this affects all ram, it is most efficient
> +     * to handle this during translation.
> +     */
> +    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;
> +#endif
> +}
> +
>  static CPUARMTBFlags rebuild_hflags_common(CPUARMState *env, int fp_el,
>                                             ARMMMUIdx mmu_idx,
>                                             CPUARMTBFlags flags)
> @@ -121,8 +150,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);
>      }
>  
> @@ -223,7 +253,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);
>      }
>  

Re: [PATCH v3 5/6] target/arm: Do memory type alignment check when translation disabled

[Jonathan Cameron via]

On Fri,  1 Mar 2024 10:41:09 -1000
Richard Henderson <richard.henderson@linaro.org> wrote:

> If translation is disabled, the default memory type is Device, which
> requires alignment checking.  This is more optimally done early via
> the MemOp given to the TCG memory operation.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 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>

Hi Richard.

I noticed some tests I was running stopped booting with master.
(it's a fun and complex stack of QEMU + kvm on QEMU for vCPU Hotplug kernel work,
but this is the host booting)

EDK2 build from upstream as of somepoint last week.

Bisects to this patch.

 qemu-system-aarch64 -M virt,gic-version=3,virtualization=true -m 4g,maxmem=8G,slots=8 -cpu cortex-a76 -smp cpus=4,threads=2,clusters=2,sockets=1 \
 -kernel Image \
 -drive if=none,file=full.qcow2,format=qcow2,id=hd \
 -device ioh3420,id=root_port1 -device virtio-blk-pci,drive=hd \
 -netdev user,id=mynet,hostfwd=tcp::5555-:22 -device virtio-net-pci,netdev=mynet,id=bob \
 -nographic -no-reboot -append 'earlycon root=/dev/vda2 fsck.mode=skip tp_printk' \
 -monitor telnet:127.0.0.1:1235,server,nowait -bios QEMU_EFI.fd \
 -object memory-backend-ram,size=4G,id=mem0 \
 -numa node,nodeid=0,cpus=0-3,memdev=mem0

Symptoms: Nothing on console from edk2 which is built in debug mode so is normally very noisy.
          No sign of anything much happening at all :(

Jonathan



> ---
>  target/arm/tcg/hflags.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/tcg/hflags.c b/target/arm/tcg/hflags.c
> index 8e5d35d922..5da1b0fc1d 100644
> --- a/target/arm/tcg/hflags.c
> +++ b/target/arm/tcg/hflags.c
> @@ -26,6 +26,35 @@ static inline bool fgt_svc(CPUARMState *env, int el)
>          FIELD_EX64(env->cp15.fgt_exec[FGTREG_HFGITR], HFGITR_EL2, SVC_EL1);
>  }
>  
> +/* Return true if memory alignment should be enforced. */
> +static bool aprofile_require_alignment(CPUARMState *env, int el, uint64_t sctlr)
> +{
> +#ifdef CONFIG_USER_ONLY
> +    return false;
> +#else
> +    /* Check the alignment enable bit. */
> +    if (sctlr & SCTLR_A) {
> +        return true;
> +    }
> +
> +    /*
> +     * If translation is disabled, then the default memory type is
> +     * Device(-nGnRnE) instead of Normal, which requires that alignment
> +     * be enforced.  Since this affects all ram, it is most efficient
> +     * to handle this during translation.
> +     */
> +    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;
> +#endif
> +}
> +
>  static CPUARMTBFlags rebuild_hflags_common(CPUARMState *env, int fp_el,
>                                             ARMMMUIdx mmu_idx,
>                                             CPUARMTBFlags flags)
> @@ -121,8 +150,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);
>      }
>  
> @@ -223,7 +253,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);
>      }
>  

Re: [PATCH v3 5/6] target/arm: Do memory type alignment check when translation disabled

[Richard Henderson]

On 4/16/24 08:11, Jonathan Cameron wrote:
> On Fri,  1 Mar 2024 10:41:09 -1000
> Richard Henderson <richard.henderson@linaro.org> wrote:
> 
>> If translation is disabled, the default memory type is Device, which
>> requires alignment checking.  This is more optimally done early via
>> the MemOp given to the TCG memory operation.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> 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>
> 
> Hi Richard.
> 
> I noticed some tests I was running stopped booting with master.
> (it's a fun and complex stack of QEMU + kvm on QEMU for vCPU Hotplug kernel work,
> but this is the host booting)
> 
> EDK2 build from upstream as of somepoint last week.
> 
> Bisects to this patch.
> 
>   qemu-system-aarch64 -M virt,gic-version=3,virtualization=true -m 4g,maxmem=8G,slots=8 -cpu cortex-a76 -smp cpus=4,threads=2,clusters=2,sockets=1 \
>   -kernel Image \
>   -drive if=none,file=full.qcow2,format=qcow2,id=hd \
>   -device ioh3420,id=root_port1 -device virtio-blk-pci,drive=hd \
>   -netdev user,id=mynet,hostfwd=tcp::5555-:22 -device virtio-net-pci,netdev=mynet,id=bob \
>   -nographic -no-reboot -append 'earlycon root=/dev/vda2 fsck.mode=skip tp_printk' \
>   -monitor telnet:127.0.0.1:1235,server,nowait -bios QEMU_EFI.fd \
>   -object memory-backend-ram,size=4G,id=mem0 \
>   -numa node,nodeid=0,cpus=0-3,memdev=mem0
> 
> Symptoms: Nothing on console from edk2 which is built in debug mode so is normally very noisy.
>            No sign of anything much happening at all :(

This isn't a fantastic bug report.

(1) If it doesn't boot efi, then none of the -kernel parameters are necessary.
(2) I'd be surprised if the full.qcow2 drive parameters are necessary either.
     But if they are, what contents?  Is a new empty drive sufficient, just
     enough to send the bios through the correct device initialization?
(3) edk2 build from ...
     Well, this is partly edk2's fault, as the build documentation is awful.
     I spent an entire afternoon trying to figure it out and gave up.

I will say that the edk2 shipped with qemu does work, so... are you absolutely
certain that it isn't a bug in edk2 since then?  Firmware bugs are exactly what
that patch is supposed to expose, as requested by issue #1204.

I'd say you should boot with "-d int" and see what kind of interrupts you're getting very 
early on.  I suspect that you'll see data aborts with ESR xx/yy where the last 6 bits of 
yy are 0x21 (alignment fault).


r~

Re: [PATCH v3 5/6] target/arm: Do memory type alignment check when translation disabled

[Jonathan Cameron via]

On Wed, 17 Apr 2024 13:07:35 -0700
Richard Henderson <richard.henderson@linaro.org> wrote:

> On 4/16/24 08:11, Jonathan Cameron wrote:
> > On Fri,  1 Mar 2024 10:41:09 -1000
> > Richard Henderson <richard.henderson@linaro.org> wrote:
> >   
> >> If translation is disabled, the default memory type is Device, which
> >> requires alignment checking.  This is more optimally done early via
> >> the MemOp given to the TCG memory operation.
> >>
> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> 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>  
> > 
> > Hi Richard.
> > 
> > I noticed some tests I was running stopped booting with master.
> > (it's a fun and complex stack of QEMU + kvm on QEMU for vCPU Hotplug kernel work,
> > but this is the host booting)
> > 
> > EDK2 build from upstream as of somepoint last week.
> > 
> > Bisects to this patch.
> > 
> >   qemu-system-aarch64 -M virt,gic-version=3,virtualization=true -m 4g,maxmem=8G,slots=8 -cpu cortex-a76 -smp cpus=4,threads=2,clusters=2,sockets=1 \
> >   -kernel Image \
> >   -drive if=none,file=full.qcow2,format=qcow2,id=hd \
> >   -device ioh3420,id=root_port1 -device virtio-blk-pci,drive=hd \
> >   -netdev user,id=mynet,hostfwd=tcp::5555-:22 -device virtio-net-pci,netdev=mynet,id=bob \
> >   -nographic -no-reboot -append 'earlycon root=/dev/vda2 fsck.mode=skip tp_printk' \
> >   -monitor telnet:127.0.0.1:1235,server,nowait -bios QEMU_EFI.fd \
> >   -object memory-backend-ram,size=4G,id=mem0 \
> >   -numa node,nodeid=0,cpus=0-3,memdev=mem0
> > 
> > Symptoms: Nothing on console from edk2 which is built in debug mode so is normally very noisy.
> >            No sign of anything much happening at all :(  
> 
> This isn't a fantastic bug report.
> 
> (1) If it doesn't boot efi, then none of the -kernel parameters are necessary.
> (2) I'd be surprised if the full.qcow2 drive parameters are necessary either.
>      But if they are, what contents?  Is a new empty drive sufficient, just
>      enough to send the bios through the correct device initialization?
> (3) edk2 build from ...
>      Well, this is partly edk2's fault, as the build documentation is awful.
>      I spent an entire afternoon trying to figure it out and gave up.
> 
> I will say that the edk2 shipped with qemu does work, so... are you absolutely
> certain that it isn't a bug in edk2 since then?  Firmware bugs are exactly what
> that patch is supposed to expose, as requested by issue #1204.
> 
> I'd say you should boot with "-d int" and see what kind of interrupts you're getting very 
> early on.  I suspect that you'll see data aborts with ESR xx/yy where the last 6 bits of 
> yy are 0x21 (alignment fault).

Hi Richard,

Sorry for lack of details, I was aware it wasn't great and should have stated I planned
to come back with more details when I had time to debug.  Snowed under so for now I've
just dropped back to 8.2 and will get back to this perhaps next week.

Jonathan

> 
> 
> r~

Re: [PATCH v3 5/6] target/arm: Do memory type alignment check when translation disabled

[Jonathan Cameron via]

On Wed, 17 Apr 2024 13:07:35 -0700
Richard Henderson <richard.henderson@linaro.org> wrote:

> On 4/16/24 08:11, Jonathan Cameron wrote:
> > On Fri,  1 Mar 2024 10:41:09 -1000
> > Richard Henderson <richard.henderson@linaro.org> wrote:
> >   
> >> If translation is disabled, the default memory type is Device, which
> >> requires alignment checking.  This is more optimally done early via
> >> the MemOp given to the TCG memory operation.
> >>
> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> 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>  
> > 
> > Hi Richard.
> > 
> > I noticed some tests I was running stopped booting with master.
> > (it's a fun and complex stack of QEMU + kvm on QEMU for vCPU Hotplug kernel work,
> > but this is the host booting)
> > 
> > EDK2 build from upstream as of somepoint last week.
> > 
> > Bisects to this patch.
> > 
> >   qemu-system-aarch64 -M virt,gic-version=3,virtualization=true -m 4g,maxmem=8G,slots=8 -cpu cortex-a76 -smp cpus=4,threads=2,clusters=2,sockets=1 \
> >   -kernel Image \
> >   -drive if=none,file=full.qcow2,format=qcow2,id=hd \
> >   -device ioh3420,id=root_port1 -device virtio-blk-pci,drive=hd \
> >   -netdev user,id=mynet,hostfwd=tcp::5555-:22 -device virtio-net-pci,netdev=mynet,id=bob \
> >   -nographic -no-reboot -append 'earlycon root=/dev/vda2 fsck.mode=skip tp_printk' \
> >   -monitor telnet:127.0.0.1:1235,server,nowait -bios QEMU_EFI.fd \
> >   -object memory-backend-ram,size=4G,id=mem0 \
> >   -numa node,nodeid=0,cpus=0-3,memdev=mem0
> > 
> > Symptoms: Nothing on console from edk2 which is built in debug mode so is normally very noisy.
> >            No sign of anything much happening at all :(  
> 
> This isn't a fantastic bug report.
> 
> (1) If it doesn't boot efi, then none of the -kernel parameters are necessary.
> (2) I'd be surprised if the full.qcow2 drive parameters are necessary either.
>      But if they are, what contents?  Is a new empty drive sufficient, just
>      enough to send the bios through the correct device initialization?
> (3) edk2 build from ...
>      Well, this is partly edk2's fault, as the build documentation is awful.
>      I spent an entire afternoon trying to figure it out and gave up.
> 
> I will say that the edk2 shipped with qemu does work, so... are you absolutely
> certain that it isn't a bug in edk2 since then?  Firmware bugs are exactly what
> that patch is supposed to expose, as requested by issue #1204.
> 
> I'd say you should boot with "-d int" and see what kind of interrupts you're getting very 
> early on.  I suspect that you'll see data aborts with ESR xx/yy where the last 6 bits of 
> yy are 0x21 (alignment fault).

Hi Richard,

Sorry for lack of details, I was aware it wasn't great and should have stated I planned
to come back with more details when I had time to debug.  Snowed under so for now I've
just dropped back to 8.2 and will get back to this perhaps next week.

Jonathan

> 
> 
> r~

Re: [PATCH v3 5/6] target/arm: Do memory type alignment check when translation disabled

[Jonathan Cameron via]

On Thu, 18 Apr 2024 09:15:55 +0100
Jonathan Cameron via <qemu-devel@nongnu.org> wrote:

> On Wed, 17 Apr 2024 13:07:35 -0700
> Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> > On 4/16/24 08:11, Jonathan Cameron wrote:  
> > > On Fri,  1 Mar 2024 10:41:09 -1000
> > > Richard Henderson <richard.henderson@linaro.org> wrote:
> > >     
> > >> If translation is disabled, the default memory type is Device, which
> > >> requires alignment checking.  This is more optimally done early via
> > >> the MemOp given to the TCG memory operation.
> > >>
> > >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > >> 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>    
> > > 
> > > Hi Richard.
> > > 
> > > I noticed some tests I was running stopped booting with master.
> > > (it's a fun and complex stack of QEMU + kvm on QEMU for vCPU Hotplug kernel work,
> > > but this is the host booting)
> > > 
> > > EDK2 build from upstream as of somepoint last week.
> > > 
> > > Bisects to this patch.
> > > 
> > >   qemu-system-aarch64 -M virt,gic-version=3,virtualization=true -m 4g,maxmem=8G,slots=8 -cpu cortex-a76 -smp cpus=4,threads=2,clusters=2,sockets=1 \
> > >   -kernel Image \
> > >   -drive if=none,file=full.qcow2,format=qcow2,id=hd \
> > >   -device ioh3420,id=root_port1 -device virtio-blk-pci,drive=hd \
> > >   -netdev user,id=mynet,hostfwd=tcp::5555-:22 -device virtio-net-pci,netdev=mynet,id=bob \
> > >   -nographic -no-reboot -append 'earlycon root=/dev/vda2 fsck.mode=skip tp_printk' \
> > >   -monitor telnet:127.0.0.1:1235,server,nowait -bios QEMU_EFI.fd \
> > >   -object memory-backend-ram,size=4G,id=mem0 \
> > >   -numa node,nodeid=0,cpus=0-3,memdev=mem0
> > > 
> > > Symptoms: Nothing on console from edk2 which is built in debug mode so is normally very noisy.
> > >            No sign of anything much happening at all :(    
> > 
> > This isn't a fantastic bug report.
> > 
> > (1) If it doesn't boot efi, then none of the -kernel parameters are necessary.
> > (2) I'd be surprised if the full.qcow2 drive parameters are necessary either.
> >      But if they are, what contents?  Is a new empty drive sufficient, just
> >      enough to send the bios through the correct device initialization?
> > (3) edk2 build from ...
> >      Well, this is partly edk2's fault, as the build documentation is awful.
> >      I spent an entire afternoon trying to figure it out and gave up.
> > 
> > I will say that the edk2 shipped with qemu does work, so... are you absolutely
> > certain that it isn't a bug in edk2 since then?  Firmware bugs are exactly what
> > that patch is supposed to expose, as requested by issue #1204.
> > 
> > I'd say you should boot with "-d int" and see what kind of interrupts you're getting very 
> > early on.  I suspect that you'll see data aborts with ESR xx/yy where the last 6 bits of 
> > yy are 0x21 (alignment fault).  
> 
> Hi Richard,
> 
> Sorry for lack of details, I was aware it wasn't great and should have stated I planned
> to come back with more details when I had time to debug.  Snowed under so for now I've
> just dropped back to 8.2 and will get back to this perhaps next week.

+CC EDK2 list and Gerd.

Still not a thorough report but some breadcrumbs.

May be something about my local build setup as the shipped EDK2 succeeds,
but the one I'm building via
uefi-tools/edk2-build.sh armvirtqemu64
(some aged instructions here that are more or less working still)
https://people.kernel.org/jic23/

Indeed starts out with some alignment faults.

Gerd, any ideas?  Maybe I needs something subtly different in my
edk2 build?  I've not looked at this bit of the qemu infrastructure
before - is there a document on how that image is built?
As Richard observed, EDK2 isn't the simplest thing to build - I've
been using uefitools for this for a long time, so maybe I missed some
new requirement?

Build machine is x86_64 ubuntu, gcc 12.2.0.

I need to build it because of some necessary tweaks to debug a
PCI enumeration issue in Linux. (these tests were without those
tweaks)

As Richard observed, most of the command line isn't needed.

qemu-system-aarch64 -M virt,virtualization=true, -m 4g -cpu cortex-a76 \
-bios QEMU_EFI.fd -d int

Jonathan

 


> 
> Jonathan
> 
> > 
> > 
> > r~  
> 
> 

Re: [PATCH v3 5/6] target/arm: Do memory type alignment check when translation disabled

[Jonathan Cameron via]

On Thu, 18 Apr 2024 09:15:55 +0100
Jonathan Cameron via <qemu-devel@nongnu.org> wrote:

> On Wed, 17 Apr 2024 13:07:35 -0700
> Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> > On 4/16/24 08:11, Jonathan Cameron wrote:  
> > > On Fri,  1 Mar 2024 10:41:09 -1000
> > > Richard Henderson <richard.henderson@linaro.org> wrote:
> > >     
> > >> If translation is disabled, the default memory type is Device, which
> > >> requires alignment checking.  This is more optimally done early via
> > >> the MemOp given to the TCG memory operation.
> > >>
> > >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > >> 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>    
> > > 
> > > Hi Richard.
> > > 
> > > I noticed some tests I was running stopped booting with master.
> > > (it's a fun and complex stack of QEMU + kvm on QEMU for vCPU Hotplug kernel work,
> > > but this is the host booting)
> > > 
> > > EDK2 build from upstream as of somepoint last week.
> > > 
> > > Bisects to this patch.
> > > 
> > >   qemu-system-aarch64 -M virt,gic-version=3,virtualization=true -m 4g,maxmem=8G,slots=8 -cpu cortex-a76 -smp cpus=4,threads=2,clusters=2,sockets=1 \
> > >   -kernel Image \
> > >   -drive if=none,file=full.qcow2,format=qcow2,id=hd \
> > >   -device ioh3420,id=root_port1 -device virtio-blk-pci,drive=hd \
> > >   -netdev user,id=mynet,hostfwd=tcp::5555-:22 -device virtio-net-pci,netdev=mynet,id=bob \
> > >   -nographic -no-reboot -append 'earlycon root=/dev/vda2 fsck.mode=skip tp_printk' \
> > >   -monitor telnet:127.0.0.1:1235,server,nowait -bios QEMU_EFI.fd \
> > >   -object memory-backend-ram,size=4G,id=mem0 \
> > >   -numa node,nodeid=0,cpus=0-3,memdev=mem0
> > > 
> > > Symptoms: Nothing on console from edk2 which is built in debug mode so is normally very noisy.
> > >            No sign of anything much happening at all :(    
> > 
> > This isn't a fantastic bug report.
> > 
> > (1) If it doesn't boot efi, then none of the -kernel parameters are necessary.
> > (2) I'd be surprised if the full.qcow2 drive parameters are necessary either.
> >      But if they are, what contents?  Is a new empty drive sufficient, just
> >      enough to send the bios through the correct device initialization?
> > (3) edk2 build from ...
> >      Well, this is partly edk2's fault, as the build documentation is awful.
> >      I spent an entire afternoon trying to figure it out and gave up.
> > 
> > I will say that the edk2 shipped with qemu does work, so... are you absolutely
> > certain that it isn't a bug in edk2 since then?  Firmware bugs are exactly what
> > that patch is supposed to expose, as requested by issue #1204.
> > 
> > I'd say you should boot with "-d int" and see what kind of interrupts you're getting very 
> > early on.  I suspect that you'll see data aborts with ESR xx/yy where the last 6 bits of 
> > yy are 0x21 (alignment fault).  
> 
> Hi Richard,
> 
> Sorry for lack of details, I was aware it wasn't great and should have stated I planned
> to come back with more details when I had time to debug.  Snowed under so for now I've
> just dropped back to 8.2 and will get back to this perhaps next week.

+CC EDK2 list and Gerd.

Still not a thorough report but some breadcrumbs.

May be something about my local build setup as the shipped EDK2 succeeds,
but the one I'm building via
uefi-tools/edk2-build.sh armvirtqemu64
(some aged instructions here that are more or less working still)
https://people.kernel.org/jic23/

Indeed starts out with some alignment faults.

Gerd, any ideas?  Maybe I needs something subtly different in my
edk2 build?  I've not looked at this bit of the qemu infrastructure
before - is there a document on how that image is built?
As Richard observed, EDK2 isn't the simplest thing to build - I've
been using uefitools for this for a long time, so maybe I missed some
new requirement?

Build machine is x86_64 ubuntu, gcc 12.2.0.

I need to build it because of some necessary tweaks to debug a
PCI enumeration issue in Linux. (these tests were without those
tweaks)

As Richard observed, most of the command line isn't needed.

qemu-system-aarch64 -M virt,virtualization=true, -m 4g -cpu cortex-a76 \
-bios QEMU_EFI.fd -d int

Jonathan

 


> 
> Jonathan
> 
> > 
> > 
> > r~  
> 
> 

Re: [edk2-devel] [PATCH v3 5/6] target/arm: Do memory type alignment check when translation disabled

[Gerd Hoffmann]

  Hi,

> Gerd, any ideas?  Maybe I needs something subtly different in my
> edk2 build?  I've not looked at this bit of the qemu infrastructure
> before - is there a document on how that image is built?

There is roms/Makefile for that.

make -C roms help
make -C roms efi

So easiest would be to just update the edk2 submodule to what you
need, then rebuild.

The build is handled by the roms/edk2-build.py script,
with the build configuration being in roms/edk2-build.config.
That is usable outside the qemu source tree too, i.e. like this:

  python3 /path/to/qemu.git/roms/edk2-build.py \
    --config /path/to/qemu.git/roms/edk2-build.config \
    --core /path/to/edk2.git \
    --match armvirt \
    --silent --no-logs

That'll try to place the images build in "../pc-bios", so maybe better
work with a copy of the config file where you adjust this.

HTH,
  Gerd

Re: [edk2-devel] [PATCH v3 5/6] target/arm: Do memory type alignment check when translation disabled

[Jonathan Cameron via]

On Fri, 19 Apr 2024 13:52:07 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > Gerd, any ideas?  Maybe I needs something subtly different in my
> > edk2 build?  I've not looked at this bit of the qemu infrastructure
> > before - is there a document on how that image is built?  
> 
> There is roms/Makefile for that.
> 
> make -C roms help
> make -C roms efi
> 
> So easiest would be to just update the edk2 submodule to what you
> need, then rebuild.
> 
> The build is handled by the roms/edk2-build.py script,
> with the build configuration being in roms/edk2-build.config.
> That is usable outside the qemu source tree too, i.e. like this:
> 
>   python3 /path/to/qemu.git/roms/edk2-build.py \
>     --config /path/to/qemu.git/roms/edk2-build.config \
>     --core /path/to/edk2.git \
>     --match armvirt \
>     --silent --no-logs
> 
> That'll try to place the images build in "../pc-bios", so maybe better
> work with a copy of the config file where you adjust this.
> 
> HTH,
>   Gerd
> 

Thanks Gerd!

So the builds are very similar via the two method...
However - the QEMU build sets -D CAVIUM_ERRATUM_27456=TRUE

And that's the difference - with that set for my other builds the alignment
problems go away...

Any idea why we have that set in roms/edk2-build.config?
Superficially it seems rather unlikely anyone cares about thunderx1
(if they do we need to get them some new hardware with fresh bugs)
bugs now and this config file was only added last year.


However, the last comment in Ard's commit message below seems
highly likely to be relevant!

Chasing through Ard's patch it has the side effect of dropping
an override of a requirement for strict alignment. 
So with out the errata 
DEFINE GCC_AARCH64_CC_XIPFLAGS     = -mstrict-align -mgeneral-regs-only
is replaced with
 [BuildOptions]
+!if $(CAVIUM_ERRATUM_27456) == TRUE^M
+  GCC:*_*_AARCH64_PP_FLAGS = -DCAVIUM_ERRATUM_27456^M
+!else^M
   GCC:*_*_AARCH64_CC_XIPFLAGS ==
+!endif^M

The edk2 commit that added this was the following +CC Ard.

Given I wasn't sure of the syntax of that file I set it
manually to the original value and indeed it works.


commit ec54ce1f1ab41b92782b37ae59e752fff0ef9c41
Author: Ard Biesheuvel <ardb@kernel.org>
Date:   Wed Jan 4 16:51:35 2023 +0100

    ArmVirtPkg/ArmVirtQemu: Avoid early ID map on ThunderX

    The early ID map used by ArmVirtQemu uses ASID scoped non-global
    mappings, as this allows us to switch to the permanent ID map seamlessly
    without the need for explicit TLB maintenance.

    However, this triggers a known erratum on ThunderX, which does not
    tolerate non-global mappings that are executable at EL1, as this appears
    to result in I-cache corruption. (Linux disables the KPTI based Meltdown
    mitigation on ThunderX for the same reason)

    So work around this, by detecting the CPU implementor and part number,
    and proceeding without the early ID map if a ThunderX CPU is detected.

    Note that this requires the C code to be built with strict alignment
    again, as we may end up executing it with the MMU and caches off.

    Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
    Acked-by: Laszlo Ersek <lersek@redhat.com>
    Tested-by: dann frazier <dann.frazier@canonical.com>

Test case is
qemu-system-aarch64 -M virt,virtualization=true, -m 4g -cpu cortex-a76 \
-bios QEMU_EFI.fd -d int

Which gets alignment faults since:
https://lore.kernel.org/all/20240301204110.656742-6-richard.henderson@linaro.org/

So my feeling here is EDK2 should either have yet another config for QEMU as a host
or should always set the alignment without needing to pick the CAVIUM 27456 errata
which I suspect will get dropped soonish anyway if anyone ever cleans up
old errata.

Jonathan

Re: [edk2-devel] [PATCH v3 5/6] target/arm: Do memory type alignment check when translation disabled

[Jonathan Cameron via]

On Fri, 19 Apr 2024 13:52:07 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > Gerd, any ideas?  Maybe I needs something subtly different in my
> > edk2 build?  I've not looked at this bit of the qemu infrastructure
> > before - is there a document on how that image is built?  
> 
> There is roms/Makefile for that.
> 
> make -C roms help
> make -C roms efi
> 
> So easiest would be to just update the edk2 submodule to what you
> need, then rebuild.
> 
> The build is handled by the roms/edk2-build.py script,
> with the build configuration being in roms/edk2-build.config.
> That is usable outside the qemu source tree too, i.e. like this:
> 
>   python3 /path/to/qemu.git/roms/edk2-build.py \
>     --config /path/to/qemu.git/roms/edk2-build.config \
>     --core /path/to/edk2.git \
>     --match armvirt \
>     --silent --no-logs
> 
> That'll try to place the images build in "../pc-bios", so maybe better
> work with a copy of the config file where you adjust this.
> 
> HTH,
>   Gerd
> 

Thanks Gerd!

So the builds are very similar via the two method...
However - the QEMU build sets -D CAVIUM_ERRATUM_27456=TRUE

And that's the difference - with that set for my other builds the alignment
problems go away...

Any idea why we have that set in roms/edk2-build.config?
Superficially it seems rather unlikely anyone cares about thunderx1
(if they do we need to get them some new hardware with fresh bugs)
bugs now and this config file was only added last year.


However, the last comment in Ard's commit message below seems
highly likely to be relevant!

Chasing through Ard's patch it has the side effect of dropping
an override of a requirement for strict alignment. 
So with out the errata 
DEFINE GCC_AARCH64_CC_XIPFLAGS     = -mstrict-align -mgeneral-regs-only
is replaced with
 [BuildOptions]
+!if $(CAVIUM_ERRATUM_27456) == TRUE^M
+  GCC:*_*_AARCH64_PP_FLAGS = -DCAVIUM_ERRATUM_27456^M
+!else^M
   GCC:*_*_AARCH64_CC_XIPFLAGS ==
+!endif^M

The edk2 commit that added this was the following +CC Ard.

Given I wasn't sure of the syntax of that file I set it
manually to the original value and indeed it works.


commit ec54ce1f1ab41b92782b37ae59e752fff0ef9c41
Author: Ard Biesheuvel <ardb@kernel.org>
Date:   Wed Jan 4 16:51:35 2023 +0100

    ArmVirtPkg/ArmVirtQemu: Avoid early ID map on ThunderX

    The early ID map used by ArmVirtQemu uses ASID scoped non-global
    mappings, as this allows us to switch to the permanent ID map seamlessly
    without the need for explicit TLB maintenance.

    However, this triggers a known erratum on ThunderX, which does not
    tolerate non-global mappings that are executable at EL1, as this appears
    to result in I-cache corruption. (Linux disables the KPTI based Meltdown
    mitigation on ThunderX for the same reason)

    So work around this, by detecting the CPU implementor and part number,
    and proceeding without the early ID map if a ThunderX CPU is detected.

    Note that this requires the C code to be built with strict alignment
    again, as we may end up executing it with the MMU and caches off.

    Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
    Acked-by: Laszlo Ersek <lersek@redhat.com>
    Tested-by: dann frazier <dann.frazier@canonical.com>

Test case is
qemu-system-aarch64 -M virt,virtualization=true, -m 4g -cpu cortex-a76 \
-bios QEMU_EFI.fd -d int

Which gets alignment faults since:
https://lore.kernel.org/all/20240301204110.656742-6-richard.henderson@linaro.org/

So my feeling here is EDK2 should either have yet another config for QEMU as a host
or should always set the alignment without needing to pick the CAVIUM 27456 errata
which I suspect will get dropped soonish anyway if anyone ever cleans up
old errata.

Jonathan

Re: [edk2-devel] [PATCH v3 5/6] target/arm: Do memory type alignment check when translation disabled

[Ard Biesheuvel]

On Fri, 19 Apr 2024 at 18:09, Jonathan Cameron via groups.io
<jonathan.cameron=huawei.com@groups.io> wrote:
>
> On Fri, 19 Apr 2024 13:52:07 +0200
> Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> >   Hi,
> >
> > > Gerd, any ideas?  Maybe I needs something subtly different in my
> > > edk2 build?  I've not looked at this bit of the qemu infrastructure
> > > before - is there a document on how that image is built?
> >
> > There is roms/Makefile for that.
> >
> > make -C roms help
> > make -C roms efi
> >
> > So easiest would be to just update the edk2 submodule to what you
> > need, then rebuild.
> >
> > The build is handled by the roms/edk2-build.py script,
> > with the build configuration being in roms/edk2-build.config.
> > That is usable outside the qemu source tree too, i.e. like this:
> >
> >   python3 /path/to/qemu.git/roms/edk2-build.py \
> >     --config /path/to/qemu.git/roms/edk2-build.config \
> >     --core /path/to/edk2.git \
> >     --match armvirt \
> >     --silent --no-logs
> >
> > That'll try to place the images build in "../pc-bios", so maybe better
> > work with a copy of the config file where you adjust this.
> >
> > HTH,
> >   Gerd
> >
>
> Thanks Gerd!
>
> So the builds are very similar via the two method...
> However - the QEMU build sets -D CAVIUM_ERRATUM_27456=TRUE
>
> And that's the difference - with that set for my other builds the alignment
> problems go away...
>
> Any idea why we have that set in roms/edk2-build.config?
> Superficially it seems rather unlikely anyone cares about thunderx1
> (if they do we need to get them some new hardware with fresh bugs)
> bugs now and this config file was only added last year.
>
>
> However, the last comment in Ard's commit message below seems
> highly likely to be relevant!
>
> Chasing through Ard's patch it has the side effect of dropping
> an override of a requirement for strict alignment.
> So with out the errata
> DEFINE GCC_AARCH64_CC_XIPFLAGS     = -mstrict-align -mgeneral-regs-only
> is replaced with
>  [BuildOptions]
> +!if $(CAVIUM_ERRATUM_27456) == TRUE^M
> +  GCC:*_*_AARCH64_PP_FLAGS = -DCAVIUM_ERRATUM_27456^M
> +!else^M
>    GCC:*_*_AARCH64_CC_XIPFLAGS ==
> +!endif^M
>
> The edk2 commit that added this was the following +CC Ard.
>
> Given I wasn't sure of the syntax of that file I set it
> manually to the original value and indeed it works.
>
>
> commit ec54ce1f1ab41b92782b37ae59e752fff0ef9c41
> Author: Ard Biesheuvel <ardb@kernel.org>
> Date:   Wed Jan 4 16:51:35 2023 +0100
>
>     ArmVirtPkg/ArmVirtQemu: Avoid early ID map on ThunderX
>
>     The early ID map used by ArmVirtQemu uses ASID scoped non-global
>     mappings, as this allows us to switch to the permanent ID map seamlessly
>     without the need for explicit TLB maintenance.
>
>     However, this triggers a known erratum on ThunderX, which does not
>     tolerate non-global mappings that are executable at EL1, as this appears
>     to result in I-cache corruption. (Linux disables the KPTI based Meltdown
>     mitigation on ThunderX for the same reason)
>
>     So work around this, by detecting the CPU implementor and part number,
>     and proceeding without the early ID map if a ThunderX CPU is detected.
>
>     Note that this requires the C code to be built with strict alignment
>     again, as we may end up executing it with the MMU and caches off.
>
>     Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>     Acked-by: Laszlo Ersek <lersek@redhat.com>
>     Tested-by: dann frazier <dann.frazier@canonical.com>
>
> Test case is
> qemu-system-aarch64 -M virt,virtualization=true, -m 4g -cpu cortex-a76 \
> -bios QEMU_EFI.fd -d int
>
> Which gets alignment faults since:
> https://lore.kernel.org/all/20240301204110.656742-6-richard.henderson@linaro.org/
>
> So my feeling here is EDK2 should either have yet another config for QEMU as a host
> or should always set the alignment without needing to pick the CAVIUM 27456 errata
> which I suspect will get dropped soonish anyway if anyone ever cleans up
> old errata.
>

This code was never really intended for execution at EL2, but it
happened to work, partially because TCG's lack of strict alignment
checking when the MMU is off.

Those assumptions no longer hold, so yes, let's get this fixed properly.

Given VHE and nested virt (which will likely imply VHE in practice), I
would like to extend this functionality (i.e., the use of preliminary
page tables in NOR flash) to EL2 as well, but with VHE enabled. This
means we can still elide TLB maintenance (and BBM checks) by using
different ASIDs, and otherwise, fall back to entering with the MMU off
if VHE is not available. In that case, we should enforce strict
alignment too, so that needs to be fixed regardless.

I'll try to code something up and send it round. In the mean time,
feel free to propose a minimal patch that reinstates the strict
alignment if you are pressed for time, and I'll merge it right away.

Re: [edk2-devel] [PATCH v3 5/6] target/arm: Do memory type alignment check when translation disabled

[Ard Biesheuvel]

On Fri, 19 Apr 2024 at 18:36, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Fri, 19 Apr 2024 at 18:09, Jonathan Cameron via groups.io
> <jonathan.cameron=huawei.com@groups.io> wrote:
> >
> > On Fri, 19 Apr 2024 13:52:07 +0200
> > Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > >   Hi,
> > >
> > > > Gerd, any ideas?  Maybe I needs something subtly different in my
> > > > edk2 build?  I've not looked at this bit of the qemu infrastructure
> > > > before - is there a document on how that image is built?
> > >
> > > There is roms/Makefile for that.
> > >
> > > make -C roms help
> > > make -C roms efi
> > >
> > > So easiest would be to just update the edk2 submodule to what you
> > > need, then rebuild.
> > >
> > > The build is handled by the roms/edk2-build.py script,
> > > with the build configuration being in roms/edk2-build.config.
> > > That is usable outside the qemu source tree too, i.e. like this:
> > >
> > >   python3 /path/to/qemu.git/roms/edk2-build.py \
> > >     --config /path/to/qemu.git/roms/edk2-build.config \
> > >     --core /path/to/edk2.git \
> > >     --match armvirt \
> > >     --silent --no-logs
> > >
> > > That'll try to place the images build in "../pc-bios", so maybe better
> > > work with a copy of the config file where you adjust this.
> > >
> > > HTH,
> > >   Gerd
> > >
> >
> > Thanks Gerd!
> >
> > So the builds are very similar via the two method...
> > However - the QEMU build sets -D CAVIUM_ERRATUM_27456=TRUE
> >
> > And that's the difference - with that set for my other builds the alignment
> > problems go away...
> >
> > Any idea why we have that set in roms/edk2-build.config?
> > Superficially it seems rather unlikely anyone cares about thunderx1
> > (if they do we need to get them some new hardware with fresh bugs)
> > bugs now and this config file was only added last year.
> >
> >
> > However, the last comment in Ard's commit message below seems
> > highly likely to be relevant!
> >
> > Chasing through Ard's patch it has the side effect of dropping
> > an override of a requirement for strict alignment.
> > So with out the errata
> > DEFINE GCC_AARCH64_CC_XIPFLAGS     = -mstrict-align -mgeneral-regs-only
> > is replaced with
> >  [BuildOptions]
> > +!if $(CAVIUM_ERRATUM_27456) == TRUE^M
> > +  GCC:*_*_AARCH64_PP_FLAGS = -DCAVIUM_ERRATUM_27456^M
> > +!else^M
> >    GCC:*_*_AARCH64_CC_XIPFLAGS ==
> > +!endif^M
> >
> > The edk2 commit that added this was the following +CC Ard.
> >
> > Given I wasn't sure of the syntax of that file I set it
> > manually to the original value and indeed it works.
> >
> >
> > commit ec54ce1f1ab41b92782b37ae59e752fff0ef9c41
> > Author: Ard Biesheuvel <ardb@kernel.org>
> > Date:   Wed Jan 4 16:51:35 2023 +0100
> >
> >     ArmVirtPkg/ArmVirtQemu: Avoid early ID map on ThunderX
> >
> >     The early ID map used by ArmVirtQemu uses ASID scoped non-global
> >     mappings, as this allows us to switch to the permanent ID map seamlessly
> >     without the need for explicit TLB maintenance.
> >
> >     However, this triggers a known erratum on ThunderX, which does not
> >     tolerate non-global mappings that are executable at EL1, as this appears
> >     to result in I-cache corruption. (Linux disables the KPTI based Meltdown
> >     mitigation on ThunderX for the same reason)
> >
> >     So work around this, by detecting the CPU implementor and part number,
> >     and proceeding without the early ID map if a ThunderX CPU is detected.
> >
> >     Note that this requires the C code to be built with strict alignment
> >     again, as we may end up executing it with the MMU and caches off.
> >
> >     Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >     Acked-by: Laszlo Ersek <lersek@redhat.com>
> >     Tested-by: dann frazier <dann.frazier@canonical.com>
> >
> > Test case is
> > qemu-system-aarch64 -M virt,virtualization=true, -m 4g -cpu cortex-a76 \
> > -bios QEMU_EFI.fd -d int
> >
> > Which gets alignment faults since:
> > https://lore.kernel.org/all/20240301204110.656742-6-richard.henderson@linaro.org/
> >
> > So my feeling here is EDK2 should either have yet another config for QEMU as a host
> > or should always set the alignment without needing to pick the CAVIUM 27456 errata
> > which I suspect will get dropped soonish anyway if anyone ever cleans up
> > old errata.
> >
>
> This code was never really intended for execution at EL2, but it
> happened to work, partially because TCG's lack of strict alignment
> checking when the MMU is off.
>
> Those assumptions no longer hold, so yes, let's get this fixed properly.
>
> Given VHE and nested virt (which will likely imply VHE in practice), I
> would like to extend this functionality (i.e., the use of preliminary
> page tables in NOR flash) to EL2 as well, but with VHE enabled. This
> means we can still elide TLB maintenance (and BBM checks) by using
> different ASIDs, and otherwise, fall back to entering with the MMU off
> if VHE is not available. In that case, we should enforce strict
> alignment too, so that needs to be fixed regardless.
>
> I'll try to code something up and send it round. In the mean time,
> feel free to propose a minimal patch that reinstates the strict
> alignment if you are pressed for time, and I'll merge it right away.

Actually, let's just so that first - I'll send out a patch.

Re: [PATCH v3 5/6] target/arm: Do memory type alignment check when translation disabled

[Clément Chigot]

Hi Richard,

While testing the future V9, I've some regressions on a custom board
using cortex-R5 CPUs.
Unaligned data accesses are no longer allowed because of that patch.

I've dug into the various documentation and it seems that R-profile
CPUs don't have the same default memory type as A-profile. It depends
on a default memory map provided in the R-Profile RM in C1.3 [1], even
when PMU is disabled.

> Each PMSAv8-32 MPU has an associated default memory map which is used when the MPU is not enabled.
> ...
> Table C1-4 and Table C1-5 describe the default memory map defined for the EL1 MPU.

For our case, Table C1-5 can be simplified as:
 |  0x00000000 – 0x7FFFFFFF Normal
 |  0x80000000 – 0xBFFFFFFF Device-nGnRE
 |  0xC0000000 – 0xFFFFFFFF Device-nGnRnE

Therefore, we can't blindly enable strict alignment checking solely on
SCTLR bits. We should make it depend on the address targeted. But is
it possible to know that address in `aprofile_require_alignment` ?
with `mmu_idx` ?

By the way, are R-Profile CPUs the same as those having the `PMSA`
feature ? That could mean we can use the `ARM_FEATURE_PMSA` to deal
with that, instead of create a new `ARM_FEATURE_R`

Note that the RM I've linked is for ARMv8. But this other link [2]
seems to show a similar behavior for arm-v7.

cc Jonathan and Ard, though not sure this is the same bug you've
reported earlier.

Thanks,
Clément
[1] https://developer.arm.com/documentation/ddi0568/a-c/?lang=en
[2] https://developer.arm.com/documentation/ddi0406/cb/System-Level-Architecture/Protected-Memory-System-Architecture--PMSA-/About-the-PMSA/Enabling-and-disabling-the-MPU?lang=en#BEIJEFCJ

On Fri, Mar 1, 2024 at 9:43 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> If translation is disabled, the default memory type is Device, which
> requires alignment checking.  This is more optimally done early via
> the MemOp given to the TCG memory operation.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 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/tcg/hflags.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/tcg/hflags.c b/target/arm/tcg/hflags.c
> index 8e5d35d922..5da1b0fc1d 100644
> --- a/target/arm/tcg/hflags.c
> +++ b/target/arm/tcg/hflags.c
> @@ -26,6 +26,35 @@ static inline bool fgt_svc(CPUARMState *env, int el)
>          FIELD_EX64(env->cp15.fgt_exec[FGTREG_HFGITR], HFGITR_EL2, SVC_EL1);
>  }
>
> +/* Return true if memory alignment should be enforced. */
> +static bool aprofile_require_alignment(CPUARMState *env, int el, uint64_t sctlr)
> +{
> +#ifdef CONFIG_USER_ONLY
> +    return false;
> +#else
> +    /* Check the alignment enable bit. */
> +    if (sctlr & SCTLR_A) {
> +        return true;
> +    }
> +
> +    /*
> +     * If translation is disabled, then the default memory type is
> +     * Device(-nGnRnE) instead of Normal, which requires that alignment
> +     * be enforced.  Since this affects all ram, it is most efficient
> +     * to handle this during translation.
> +     */
> +    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;
> +#endif
> +}
> +
>  static CPUARMTBFlags rebuild_hflags_common(CPUARMState *env, int fp_el,
>                                             ARMMMUIdx mmu_idx,
>                                             CPUARMTBFlags flags)
> @@ -121,8 +150,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);
>      }
>
> @@ -223,7 +253,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 v3 5/6] target/arm: Do memory type alignment check when translation disabled

[Richard Henderson]

On 4/22/24 08:26, Clément Chigot wrote:
> Hi Richard,
> 
> While testing the future V9, I've some regressions on a custom board
> using cortex-R5 CPUs.
> Unaligned data accesses are no longer allowed because of that patch.
> 
> I've dug into the various documentation and it seems that R-profile
> CPUs don't have the same default memory type as A-profile. It depends
> on a default memory map provided in the R-Profile RM in C1.3 [1], even
> when PMU is disabled.
> 
>> Each PMSAv8-32 MPU has an associated default memory map which is used when the MPU is not enabled.
>> ...
>> Table C1-4 and Table C1-5 describe the default memory map defined for the EL1 MPU.
> 
> For our case, Table C1-5 can be simplified as:
>   |  0x00000000 – 0x7FFFFFFF Normal
>   |  0x80000000 – 0xBFFFFFFF Device-nGnRE
>   |  0xC0000000 – 0xFFFFFFFF Device-nGnRnE
> 
> Therefore, we can't blindly enable strict alignment checking solely on
> SCTLR bits. We should make it depend on the address targeted. But is
> it possible to know that address in `aprofile_require_alignment` ?
> with `mmu_idx` ?

No, this would need to be handled in get_phys_addr_disabled.

> By the way, are R-Profile CPUs the same as those having the `PMSA`
> feature ? That could mean we can use the `ARM_FEATURE_PMSA` to deal
> with that, instead of create a new `ARM_FEATURE_R`

No, some armv5 have PMSA.

> 
> Note that the RM I've linked is for ARMv8. But this other link [2]
> seems to show a similar behavior for arm-v7.
> 
> cc Jonathan and Ard, though not sure this is the same bug you've
> reported earlier.
> 
> Thanks,
> Clément
> [1] https://developer.arm.com/documentation/ddi0568/a-c/?lang=en
> [2] https://developer.arm.com/documentation/ddi0406/cb/System-Level-Architecture/Protected-Memory-System-Architecture--PMSA-/About-the-PMSA/Enabling-and-disabling-the-MPU?lang=en#BEIJEFCJ

Ouch, thanks for the armv7 link.  At the moment it looks like my blanket mmu-disabled 
change should be restricted to armv8.


r~

Re: [PATCH v3 5/6] target/arm: Do memory type alignment check when translation disabled

[Peter Maydell]

On Mon, 22 Apr 2024 at 16:48, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 4/22/24 08:26, Clément Chigot wrote:
> > Hi Richard,
> >
> > While testing the future V9, I've some regressions on a custom board
> > using cortex-R5 CPUs.
> > Unaligned data accesses are no longer allowed because of that patch.
> >
> > I've dug into the various documentation and it seems that R-profile
> > CPUs don't have the same default memory type as A-profile. It depends
> > on a default memory map provided in the R-Profile RM in C1.3 [1], even
> > when PMU is disabled.
> >
> >> Each PMSAv8-32 MPU has an associated default memory map which is used when the MPU is not enabled.
> >> ...
> >> Table C1-4 and Table C1-5 describe the default memory map defined for the EL1 MPU.
> >
> > For our case, Table C1-5 can be simplified as:
> >   |  0x00000000 – 0x7FFFFFFF Normal
> >   |  0x80000000 – 0xBFFFFFFF Device-nGnRE
> >   |  0xC0000000 – 0xFFFFFFFF Device-nGnRnE
> >
> > Therefore, we can't blindly enable strict alignment checking solely on
> > SCTLR bits. We should make it depend on the address targeted. But is
> > it possible to know that address in `aprofile_require_alignment` ?
> > with `mmu_idx` ?
>
> No, this would need to be handled in get_phys_addr_disabled.
>
> > By the way, are R-Profile CPUs the same as those having the `PMSA`
> > feature ? That could mean we can use the `ARM_FEATURE_PMSA` to deal
> > with that, instead of create a new `ARM_FEATURE_R`
>
> No, some armv5 have PMSA.
>
> >
> > Note that the RM I've linked is for ARMv8. But this other link [2]
> > seems to show a similar behavior for arm-v7.
> >
> > cc Jonathan and Ard, though not sure this is the same bug you've
> > reported earlier.
> >
> > Thanks,
> > Clément
> > [1] https://developer.arm.com/documentation/ddi0568/a-c/?lang=en
> > [2] https://developer.arm.com/documentation/ddi0406/cb/System-Level-Architecture/Protected-Memory-System-Architecture--PMSA-/About-the-PMSA/Enabling-and-disabling-the-MPU?lang=en#BEIJEFCJ
>
> Ouch, thanks for the armv7 link.  At the moment it looks like my blanket mmu-disabled
> change should be restricted to armv8.

Restricted to A-profile, probably. I haven't cross-checked, but IIRC
for v7A this is IMPDEF and we're OK to fault it.

thanks
-- PMM

[PATCH v3 6/6] target/arm: Do memory type alignment check when translation enabled

[Richard Henderson]

If translation is enabled, and the PTE memory type is Device,
enable checking alignment via TLB_CHECK_ALIGNMENT.  While the
check is done later than it should be per the ARM, it's better
than not performing the check at all.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/ptw.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index ba1a27ca2b..fc2f226411 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -471,6 +471,16 @@ static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
     return false;
 }
 
+static bool S1_attrs_are_device(uint8_t attrs)
+{
+    /*
+     * This slightly under-decodes the MAIR_ELx field:
+     * 0b0000dd01 is Device with FEAT_XS, otherwise UNPREDICTABLE;
+     * 0b0000dd1x is UNPREDICTABLE.
+     */
+    return (attrs & 0xf0) == 0;
+}
+
 static bool S2_attrs_are_device(uint64_t hcr, uint8_t attrs)
 {
     /*
@@ -1684,6 +1694,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
     bool aarch64 = arm_el_is_aa64(env, el);
     uint64_t descriptor, new_descriptor;
     ARMSecuritySpace out_space;
+    bool device;
 
     /* TODO: This code does not support shareability levels. */
     if (aarch64) {
@@ -2106,6 +2117,12 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
     if (regime_is_stage2(mmu_idx)) {
         result->cacheattrs.is_s2_format = true;
         result->cacheattrs.attrs = extract32(attrs, 2, 4);
+        /*
+         * Security state does not really affect HCR_EL2.FWB;
+         * we only need to filter FWB for aa32 or other FEAT.
+         */
+        device = S2_attrs_are_device(arm_hcr_el2_eff(env),
+                                     result->cacheattrs.attrs);
     } else {
         /* Index into MAIR registers for cache attributes */
         uint8_t attrindx = extract32(attrs, 2, 3);
@@ -2118,6 +2135,21 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
         if (aarch64 && cpu_isar_feature(aa64_bti, cpu)) {
             result->f.extra.arm.guarded = extract64(attrs, 50, 1); /* GP */
         }
+        device = S1_attrs_are_device(result->cacheattrs.attrs);
+    }
+
+    /*
+     * Enable alignment checks on Device memory.
+     *
+     * Per R_XCHFJ, this check is mis-ordered, in that this alignment check
+     * should have priority 30, while the permission check should be next at
+     * priority 31 and stage2 translation faults come after that.
+     * Due to the way the TCG softmmu TLB operates, we will have implicitly
+     * done the permission check and the stage2 lookup in finding the TLB
+     * entry, so the alignment check cannot be done sooner.
+     */
+    if (device) {
+        result->f.tlb_fill_flags |= TLB_CHECK_ALIGNED;
     }
 
     /*
-- 
2.34.1

Re: [PATCH v3 6/6] target/arm: Do memory type alignment check when translation enabled

[Peter Maydell]

On Fri, 1 Mar 2024 at 20:42, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> If translation is enabled, and the PTE memory type is Device,
> enable checking alignment via TLB_CHECK_ALIGNMENT.  While the
> check is done later than it should be per the ARM, it's better
> than not performing the check at all.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>


> +    /*
> +     * Enable alignment checks on Device memory.
> +     *
> +     * Per R_XCHFJ, this check is mis-ordered, in that this alignment check
> +     * should have priority 30, while the permission check should be next at
> +     * priority 31 and stage2 translation faults come after that.
> +     * Due to the way the TCG softmmu TLB operates, we will have implicitly
> +     * done the permission check and the stage2 lookup in finding the TLB
> +     * entry, so the alignment check cannot be done sooner.
> +     */

Looks like in rev J.a the priority list has had some extra entries
added, so these are now items 35, 36 and 37 in the list.
Maybe we should drop the numbering and say

 * Per R_XCHFJ, this check is mis-ordered. The correct ordering
 * for alignment, permission, and stage 2 faults should be:
 *    - Alignment fault caused by the memory type
 *    - Permission fault
 *    - A stage 2 fault on the memory access
 * but due to ...

?

> +    if (device) {
> +        result->f.tlb_fill_flags |= TLB_CHECK_ALIGNED;
>     }

In v7, the alignment faults on Device memory accesses are only
architecturally required if the CPU implements the Virtualization
Extensions; otherwise they are UNPREDICTABLE. But in practice
QEMU doesn't implement any CPU types with ARM_FEATURE_LPAE
but not ARM_FEATURE_V7VE, and "take an alignment fault" is
something that the UNPREDICTABLE case allows us to do, so
it doesn't seem necessary to put in a check for ARM_FEATURE_LPAE
here. We could mention it in the comment, though.

I propose to fold in this comment diff and take the patchset into
target-arm.next:

--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -2141,12 +2141,19 @@ static bool get_phys_addr_lpae(CPUARMState
*env, S1Translate *ptw,
     /*
      * Enable alignment checks on Device memory.
      *
-     * Per R_XCHFJ, this check is mis-ordered, in that this alignment check
-     * should have priority 30, while the permission check should be next at
-     * priority 31 and stage2 translation faults come after that.
-     * Due to the way the TCG softmmu TLB operates, we will have implicitly
-     * done the permission check and the stage2 lookup in finding the TLB
-     * entry, so the alignment check cannot be done sooner.
+     * Per R_XCHFJ, this check is mis-ordered. The correct ordering
+     * for alignment, permission, and stage 2 faults should be:
+     *    - Alignment fault caused by the memory type
+     *    - Permission fault
+     *    - A stage 2 fault on the memory access
+     * but due to the way the TCG softmmu TLB operates, we will have
+     * implicitly done the permission check and the stage2 lookup in
+     * finding the TLB entry, so the alignment check cannot be done sooner.
+     *
+     * In v7, for a CPU without the Virtualization Extensions this
+     * access is UNPREDICTABLE; we choose to make it take the alignment
+     * fault as is required for a v7VE CPU. (QEMU doesn't emulate any
+     * CPUs with ARM_FEATURE_LPAE but not ARM_FEATURE_V7VE anyway.)
      */
     if (device) {
         result->f.tlb_fill_flags |= TLB_CHECK_ALIGNED;

thanks
-- PMM

Re: [PATCH v3 6/6] target/arm: Do memory type alignment check when translation enabled

[Richard Henderson]

On 3/4/24 07:10, Peter Maydell wrote:
> On Fri, 1 Mar 2024 at 20:42, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> If translation is enabled, and the PTE memory type is Device,
>> enable checking alignment via TLB_CHECK_ALIGNMENT.  While the
>> check is done later than it should be per the ARM, it's better
>> than not performing the check at all.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> 
> 
>> +    /*
>> +     * Enable alignment checks on Device memory.
>> +     *
>> +     * Per R_XCHFJ, this check is mis-ordered, in that this alignment check
>> +     * should have priority 30, while the permission check should be next at
>> +     * priority 31 and stage2 translation faults come after that.
>> +     * Due to the way the TCG softmmu TLB operates, we will have implicitly
>> +     * done the permission check and the stage2 lookup in finding the TLB
>> +     * entry, so the alignment check cannot be done sooner.
>> +     */
> 
> Looks like in rev J.a the priority list has had some extra entries
> added, so these are now items 35, 36 and 37 in the list.
> Maybe we should drop the numbering and say
> 
>   * Per R_XCHFJ, this check is mis-ordered. The correct ordering
>   * for alignment, permission, and stage 2 faults should be:
>   *    - Alignment fault caused by the memory type
>   *    - Permission fault
>   *    - A stage 2 fault on the memory access
>   * but due to ...
> 
> ?
> 
>> +    if (device) {
>> +        result->f.tlb_fill_flags |= TLB_CHECK_ALIGNED;
>>      }
> 
> In v7, the alignment faults on Device memory accesses are only
> architecturally required if the CPU implements the Virtualization
> Extensions; otherwise they are UNPREDICTABLE. But in practice
> QEMU doesn't implement any CPU types with ARM_FEATURE_LPAE
> but not ARM_FEATURE_V7VE, and "take an alignment fault" is
> something that the UNPREDICTABLE case allows us to do, so
> it doesn't seem necessary to put in a check for ARM_FEATURE_LPAE
> here. We could mention it in the comment, though.
> 
> I propose to fold in this comment diff and take the patchset into
> target-arm.next:
> 
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -2141,12 +2141,19 @@ static bool get_phys_addr_lpae(CPUARMState
> *env, S1Translate *ptw,
>       /*
>        * Enable alignment checks on Device memory.
>        *
> -     * Per R_XCHFJ, this check is mis-ordered, in that this alignment check
> -     * should have priority 30, while the permission check should be next at
> -     * priority 31 and stage2 translation faults come after that.
> -     * Due to the way the TCG softmmu TLB operates, we will have implicitly
> -     * done the permission check and the stage2 lookup in finding the TLB
> -     * entry, so the alignment check cannot be done sooner.
> +     * Per R_XCHFJ, this check is mis-ordered. The correct ordering
> +     * for alignment, permission, and stage 2 faults should be:
> +     *    - Alignment fault caused by the memory type
> +     *    - Permission fault
> +     *    - A stage 2 fault on the memory access
> +     * but due to the way the TCG softmmu TLB operates, we will have
> +     * implicitly done the permission check and the stage2 lookup in
> +     * finding the TLB entry, so the alignment check cannot be done sooner.
> +     *
> +     * In v7, for a CPU without the Virtualization Extensions this
> +     * access is UNPREDICTABLE; we choose to make it take the alignment
> +     * fault as is required for a v7VE CPU. (QEMU doesn't emulate any
> +     * CPUs with ARM_FEATURE_LPAE but not ARM_FEATURE_V7VE anyway.)

Looks good, thanks for the update.


r~

Re: [PATCH v3 0/6] target/arm: Do memory alignment check for device memory

[Peter Maydell]

On Fri, 1 Mar 2024 at 20:42, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> v1: https://lore.kernel.org/qemu-devel/20230223204342.1093632-1-richard.henderson@linaro.org/
> v2: https://lore.kernel.org/qemu-devel/20230621121902.1392277-1-richard.henderson@linaro.org/
>
> Some of the v2 patch set was merged last June, but the rest languished.
> No substantive changes from v2; only fixed rebase conflicts.

Applied to target-arm.next (with the comment tweak to the last
patch as noted there), thanks.

-- PMM