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

[Richard Henderson <richard.henderson@linaro.org>]

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~