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

[Peter Maydell <peter.maydell@linaro.org>]

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