[PATCH] target/arm: fix s2mmu input size check

mkei@sfc.wide.ad.jp posted 1 patch 2 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220505004046.6711-1-mkei@sfc.wide.ad.jp
Maintainers: Peter Maydell <peter.maydell@linaro.org>
There is a newer version of this series
target/arm/helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] target/arm: fix s2mmu input size check
Posted by mkei@sfc.wide.ad.jp 2 years ago
From: Keisuke Iida <mkei@sfc.wide.ad.jp>

The maximum IPA size('inputsize') is constrained by the implemented PA size that is
specified by ID_AA64MMFR0_EL1.PARange. Please reference Arm Architecture Reference
Manual for A-profile architecture "Supported IPA size" on page D5-4788.

Signed-off-by: Keisuke Iida <mkei@sfc.wide.ad.jp>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 5a244c3ed9..868e7a2c0b 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -11116,7 +11116,7 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
         }
 
         /* Inputsize checks.  */
-        if (inputsize > outputsize &&
+        if (inputsize > arm_pamax(cpu) &&
             (arm_el_is_aa64(&cpu->env, 1) || inputsize > 40)) {
             /* This is CONSTRAINED UNPREDICTABLE and we choose to fault.  */
             return false;
-- 
2.34.1
Re: [PATCH] target/arm: fix s2mmu input size check
Posted by Peter Maydell 2 years ago
On Thu, 5 May 2022 at 01:40, <mkei@sfc.wide.ad.jp> wrote:
>
> From: Keisuke Iida <mkei@sfc.wide.ad.jp>
>
> The maximum IPA size('inputsize') is constrained by the implemented PA size that is
> specified by ID_AA64MMFR0_EL1.PARange. Please reference Arm Architecture Reference
> Manual for A-profile architecture "Supported IPA size" on page D5-4788.
>
> Signed-off-by: Keisuke Iida <mkei@sfc.wide.ad.jp>
> ---
>  target/arm/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 5a244c3ed9..868e7a2c0b 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -11116,7 +11116,7 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
>          }
>
>          /* Inputsize checks.  */
> -        if (inputsize > outputsize &&
> +        if (inputsize > arm_pamax(cpu) &&
>              (arm_el_is_aa64(&cpu->env, 1) || inputsize > 40)) {
>              /* This is CONSTRAINED UNPREDICTABLE and we choose to fault.  */
>              return false;

Can you give an example, eg a test case, where you see wrong
behaviour? The 'outputsize' variable in this function is
passed in from the caller get_phys_addr_lpae(), where (for
an AArch64 guest) it is indeed constrained to the value
of ID_AA64MMFR0.PARange:

        /*
         * Bound PS by PARANGE to find the effective output address size.
         * ID_AA64MMFR0 is a read-only register so values outside of the
         * supported mappings can be considered an implementation error.
         */
        ps = FIELD_EX64(cpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
        ps = MIN(ps, param.ps);
        assert(ps < ARRAY_SIZE(pamax_map));
        outputsize = pamax_map[ps];


thanks
-- PMM
Re: [PATCH] target/arm: fix s2mmu input size check
Posted by Keisuke Iida 2 years ago
Address Translation Fault is triggered when PA size set by VTCR_EL2.PS is less than IPA size set by VTCR_EL2.T0SZ on the guest. (e.g. vtcr_el2.PS = 1 && vtcr_el2.T0SZ = 25. PA size is 36bit, and IPA size is 39bit.)

         ps = FIELD_EX64(cpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
         ps = MIN(ps, param.ps);
         assert(ps < ARRAY_SIZE(pamax_map));
         outputsize = pamax_map[ps];

When 'param.ps' determined by VTCR_EL2.PS less than 'ps', 'outputsize' is set to PA address by VTCR_EL2.PS.

--

Keisuke Iida

On 2022/05/05 17:20, Peter Maydell wrote:

> On Thu, 5 May 2022 at 01:40,<mkei@sfc.wide.ad.jp>  wrote:
>> From: Keisuke Iida<mkei@sfc.wide.ad.jp>
>>
>> The maximum IPA size('inputsize') is constrained by the implemented PA size that is
>> specified by ID_AA64MMFR0_EL1.PARange. Please reference Arm Architecture Reference
>> Manual for A-profile architecture "Supported IPA size" on page D5-4788.
>>
>> Signed-off-by: Keisuke Iida<mkei@sfc.wide.ad.jp>
>> ---
>>   target/arm/helper.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index 5a244c3ed9..868e7a2c0b 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -11116,7 +11116,7 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
>>           }
>>
>>           /* Inputsize checks.  */
>> -        if (inputsize > outputsize &&
>> +        if (inputsize > arm_pamax(cpu) &&
>>               (arm_el_is_aa64(&cpu->env, 1) || inputsize > 40)) {
>>               /* This is CONSTRAINED UNPREDICTABLE and we choose to fault.  */
>>               return false;
> Can you give an example, eg a test case, where you see wrong
> behaviour? The 'outputsize' variable in this function is
> passed in from the caller get_phys_addr_lpae(), where (for
> an AArch64 guest) it is indeed constrained to the value
> of ID_AA64MMFR0.PARange:
>
>          /*
>           * Bound PS by PARANGE to find the effective output address size.
>           * ID_AA64MMFR0 is a read-only register so values outside of the
>           * supported mappings can be considered an implementation error.
>           */
>          ps = FIELD_EX64(cpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
>          ps = MIN(ps, param.ps);
>          assert(ps < ARRAY_SIZE(pamax_map));
>          outputsize = pamax_map[ps];
>
>
> thanks
> -- PMM