* [PATCH 1/3] target/arm: Check VSTCR.SW when assigning the stage 2 output PA space
2022-03-27 9:34 [PATCH 0/3] Bug fixes related to secure 2 stage translation Idan Horowitz
@ 2022-03-27 9:34 ` Idan Horowitz
2022-03-27 14:24 ` Rémi Denis-Courmont
2022-03-29 14:59 ` Richard Henderson
2022-03-27 9:34 ` [PATCH 2/3] target/arm: Take VSTCR.SW, VTCR.NSW into account in final stage 2 walk Idan Horowitz
` (2 subsequent siblings)
3 siblings, 2 replies; 12+ messages in thread
From: Idan Horowitz @ 2022-03-27 9:34 UTC (permalink / raw)
To: qemu-arm; +Cc: Peter Maydell, Idan Horowitz, qemu-devel
As per the AArch64.SS2OutputPASpace() psuedo-code in the ARMv8 ARM when the
PA space of the IPA is non secure, the output PA space is secure if and only
if all of the bits VTCR.<NSW, NSA>, VSTCR.<SW, SA> are not set.
Signed-off-by: Idan Horowitz <idan.horowitz@gmail.com>
---
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 812ca591f4..d0265b760f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -12697,7 +12697,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
} else {
attrs->secure =
!((env->cp15.vtcr_el2.raw_tcr & (VTCR_NSA | VTCR_NSW))
- || (env->cp15.vstcr_el2.raw_tcr & VSTCR_SA));
+ || (env->cp15.vstcr_el2.raw_tcr & (VSTCR_SA | VSTCR_SW)));
}
}
return 0;
--
2.35.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] target/arm: Check VSTCR.SW when assigning the stage 2 output PA space
2022-03-27 9:34 ` [PATCH 1/3] target/arm: Check VSTCR.SW when assigning the stage 2 output PA space Idan Horowitz
@ 2022-03-27 14:24 ` Rémi Denis-Courmont
2022-03-29 14:59 ` Richard Henderson
2022-03-29 14:59 ` Richard Henderson
1 sibling, 1 reply; 12+ messages in thread
From: Rémi Denis-Courmont @ 2022-03-27 14:24 UTC (permalink / raw)
To: qemu-arm; +Cc: Peter Maydell, Idan Horowitz, qemu-devel
Le sunnuntaina 27. maaliskuuta 2022, 12.34.26 EEST Idan Horowitz a écrit :
> As per the AArch64.SS2OutputPASpace() psuedo-code in the ARMv8 ARM when the
> PA space of the IPA is non secure, the output PA space is secure if and only
> if all of the bits VTCR.<NSW, NSA>, VSTCR.<SW, SA> are not set.
>
> Signed-off-by: Idan Horowitz <idan.horowitz@gmail.com>
> ---
> 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 812ca591f4..d0265b760f 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -12697,7 +12697,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong
> address, } else {
> attrs->secure =
> !((env->cp15.vtcr_el2.raw_tcr & (VTCR_NSA |
> VTCR_NSW)) - || (env->cp15.vstcr_el2.raw_tcr &
> VSTCR_SA)); + || (env->cp15.vstcr_el2.raw_tcr &
> (VSTCR_SA | VSTCR_SW))); }
The VTCR_EL2 specification says that the NSA bit "behaves as 1 for all purposes
other than reading back the value of the bit when one of the following is true
(...)
* The value of VTCR_EL2.NSW is 1.
* The value of VSTCR_EL2.SA is 1."
Sorry but I don't see any reason to check the SW bit here.
--
Реми Дёни-Курмон
http://www.remlab.net/
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] target/arm: Check VSTCR.SW when assigning the stage 2 output PA space
2022-03-27 14:24 ` Rémi Denis-Courmont
@ 2022-03-29 14:59 ` Richard Henderson
0 siblings, 0 replies; 12+ messages in thread
From: Richard Henderson @ 2022-03-29 14:59 UTC (permalink / raw)
To: Rémi Denis-Courmont, qemu-arm
Cc: Peter Maydell, Idan Horowitz, qemu-devel
On 3/27/22 08:24, Rémi Denis-Courmont wrote:
> Le sunnuntaina 27. maaliskuuta 2022, 12.34.26 EEST Idan Horowitz a écrit :
>> As per the AArch64.SS2OutputPASpace() psuedo-code in the ARMv8 ARM when the
>> PA space of the IPA is non secure, the output PA space is secure if and only
>> if all of the bits VTCR.<NSW, NSA>, VSTCR.<SW, SA> are not set.
>>
>> Signed-off-by: Idan Horowitz <idan.horowitz@gmail.com>
>> ---
>> 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 812ca591f4..d0265b760f 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -12697,7 +12697,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong
>> address, } else {
>> attrs->secure =
>> !((env->cp15.vtcr_el2.raw_tcr & (VTCR_NSA |
>> VTCR_NSW)) - || (env->cp15.vstcr_el2.raw_tcr &
>> VSTCR_SA)); + || (env->cp15.vstcr_el2.raw_tcr &
>> (VSTCR_SA | VSTCR_SW))); }
>
> The VTCR_EL2 specification says that the NSA bit "behaves as 1 for all purposes
> other than reading back the value of the bit when one of the following is true
> (...)
> * The value of VTCR_EL2.NSW is 1.
> * The value of VSTCR_EL2.SA is 1."
>
> Sorry but I don't see any reason to check the SW bit here.
Because the description of SA says that it behaves as 1 if SW is 1.
r~
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] target/arm: Check VSTCR.SW when assigning the stage 2 output PA space
2022-03-27 9:34 ` [PATCH 1/3] target/arm: Check VSTCR.SW when assigning the stage 2 output PA space Idan Horowitz
2022-03-27 14:24 ` Rémi Denis-Courmont
@ 2022-03-29 14:59 ` Richard Henderson
1 sibling, 0 replies; 12+ messages in thread
From: Richard Henderson @ 2022-03-29 14:59 UTC (permalink / raw)
To: Idan Horowitz, qemu-arm; +Cc: Peter Maydell, qemu-devel
On 3/27/22 03:34, Idan Horowitz wrote:
> As per the AArch64.SS2OutputPASpace() psuedo-code in the ARMv8 ARM when the
> PA space of the IPA is non secure, the output PA space is secure if and only
> if all of the bits VTCR.<NSW, NSA>, VSTCR.<SW, SA> are not set.
>
> Signed-off-by: Idan Horowitz<idan.horowitz@gmail.com>
> ---
> target/arm/helper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] target/arm: Take VSTCR.SW, VTCR.NSW into account in final stage 2 walk
2022-03-27 9:34 [PATCH 0/3] Bug fixes related to secure 2 stage translation Idan Horowitz
2022-03-27 9:34 ` [PATCH 1/3] target/arm: Check VSTCR.SW when assigning the stage 2 output PA space Idan Horowitz
@ 2022-03-27 9:34 ` Idan Horowitz
2022-03-29 16:36 ` Richard Henderson
2022-03-27 9:34 ` [PATCH 3/3] target/arm: Determine final stage 2 output PA space based on original IPA Idan Horowitz
2022-03-31 8:46 ` [PATCH 0/3] Bug fixes related to secure 2 stage translation Peter Maydell
3 siblings, 1 reply; 12+ messages in thread
From: Idan Horowitz @ 2022-03-27 9:34 UTC (permalink / raw)
To: qemu-arm; +Cc: Peter Maydell, Idan Horowitz, qemu-devel
As per the AArch64.SS2InitialTTWState() psuedo-code in the ARMv8 ARM the
initial PA space used for stage 2 table walks is assigned based on the SW
and NSW bits of the VSTCR and VTCR registers.
This was already implemented for the recursive stage 2 page table walks
in S1_ptw_translate(), but was missing for the final stage 2 walk.
Signed-off-by: Idan Horowitz <idan.horowitz@gmail.com>
---
target/arm/helper.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index d0265b760f..e2695e846a 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -12657,6 +12657,16 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
return ret;
}
+ if (arm_is_secure_below_el3(env)) {
+ if (attrs->secure) {
+ attrs->secure = !(env->cp15.vstcr_el2.raw_tcr & VSTCR_SW);
+ } else {
+ attrs->secure = !(env->cp15.vtcr_el2.raw_tcr & VTCR_NSW);
+ }
+ } else {
+ assert(!attrs->secure);
+ }
+
s2_mmu_idx = attrs->secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
is_el0 = mmu_idx == ARMMMUIdx_E10_0 || mmu_idx == ARMMMUIdx_SE10_0;
--
2.35.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] target/arm: Take VSTCR.SW, VTCR.NSW into account in final stage 2 walk
2022-03-27 9:34 ` [PATCH 2/3] target/arm: Take VSTCR.SW, VTCR.NSW into account in final stage 2 walk Idan Horowitz
@ 2022-03-29 16:36 ` Richard Henderson
0 siblings, 0 replies; 12+ messages in thread
From: Richard Henderson @ 2022-03-29 16:36 UTC (permalink / raw)
To: Idan Horowitz, qemu-arm; +Cc: Peter Maydell, qemu-devel
On 3/27/22 03:34, Idan Horowitz wrote:
> As per the AArch64.SS2InitialTTWState() psuedo-code in the ARMv8 ARM the
> initial PA space used for stage 2 table walks is assigned based on the SW
> and NSW bits of the VSTCR and VTCR registers.
> This was already implemented for the recursive stage 2 page table walks
> in S1_ptw_translate(), but was missing for the final stage 2 walk.
>
> Signed-off-by: Idan Horowitz<idan.horowitz@gmail.com>
> ---
> target/arm/helper.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] target/arm: Determine final stage 2 output PA space based on original IPA
2022-03-27 9:34 [PATCH 0/3] Bug fixes related to secure 2 stage translation Idan Horowitz
2022-03-27 9:34 ` [PATCH 1/3] target/arm: Check VSTCR.SW when assigning the stage 2 output PA space Idan Horowitz
2022-03-27 9:34 ` [PATCH 2/3] target/arm: Take VSTCR.SW, VTCR.NSW into account in final stage 2 walk Idan Horowitz
@ 2022-03-27 9:34 ` Idan Horowitz
2022-03-29 19:09 ` Richard Henderson
2022-03-31 8:46 ` [PATCH 0/3] Bug fixes related to secure 2 stage translation Peter Maydell
3 siblings, 1 reply; 12+ messages in thread
From: Idan Horowitz @ 2022-03-27 9:34 UTC (permalink / raw)
To: qemu-arm; +Cc: Peter Maydell, Idan Horowitz, qemu-devel
As per the AArch64.S2Walk() psuedo-code in the ARMv8 ARM, the final
decision as to the output address's PA space based on the SA/SW/NSA/NSA
bits needs to take the input IPA's PA space into account, and not the
PA space of the result of the stage 2 walk itself.
Signed-off-by: Idan Horowitz <idan.horowitz@gmail.com>
---
target/arm/helper.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index e2695e846a..16c2628f8f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -12644,6 +12644,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
hwaddr ipa;
int s2_prot;
int ret;
+ bool ipa_secure;
ARMCacheAttrs cacheattrs2 = {};
ARMMMUIdx s2_mmu_idx;
bool is_el0;
@@ -12657,14 +12658,15 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
return ret;
}
+ ipa_secure = attrs->secure;
if (arm_is_secure_below_el3(env)) {
- if (attrs->secure) {
+ if (ipa_secure) {
attrs->secure = !(env->cp15.vstcr_el2.raw_tcr & VSTCR_SW);
} else {
attrs->secure = !(env->cp15.vtcr_el2.raw_tcr & VTCR_NSW);
}
} else {
- assert(!attrs->secure);
+ assert(!ipa_secure);
}
s2_mmu_idx = attrs->secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
@@ -12701,7 +12703,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
/* Check if IPA translates to secure or non-secure PA space. */
if (arm_is_secure_below_el3(env)) {
- if (attrs->secure) {
+ if (ipa_secure) {
attrs->secure =
!(env->cp15.vstcr_el2.raw_tcr & (VSTCR_SA | VSTCR_SW));
} else {
--
2.35.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] target/arm: Determine final stage 2 output PA space based on original IPA
2022-03-27 9:34 ` [PATCH 3/3] target/arm: Determine final stage 2 output PA space based on original IPA Idan Horowitz
@ 2022-03-29 19:09 ` Richard Henderson
2022-03-29 20:18 ` Idan Horowitz
2022-03-31 8:46 ` Peter Maydell
0 siblings, 2 replies; 12+ messages in thread
From: Richard Henderson @ 2022-03-29 19:09 UTC (permalink / raw)
To: Idan Horowitz, qemu-arm; +Cc: Peter Maydell, qemu-devel
On 3/27/22 03:34, Idan Horowitz wrote:
> As per the AArch64.S2Walk() psuedo-code in the ARMv8 ARM, the final
> decision as to the output address's PA space based on the SA/SW/NSA/NSA
> bits needs to take the input IPA's PA space into account, and not the
> PA space of the result of the stage 2 walk itself.
>
> Signed-off-by: Idan Horowitz <idan.horowitz@gmail.com>
I believe I follow: because the walk uses walkstate.address.paddress.paspace, the ipa
input parameter is unchanged, and it is ipa that is passed to
AArch64.S2NextWalkStateLast() to form the output address.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] target/arm: Determine final stage 2 output PA space based on original IPA
2022-03-29 19:09 ` Richard Henderson
@ 2022-03-29 20:18 ` Idan Horowitz
2022-03-31 8:46 ` Peter Maydell
1 sibling, 0 replies; 12+ messages in thread
From: Idan Horowitz @ 2022-03-29 20:18 UTC (permalink / raw)
To: Richard Henderson, qemu-arm; +Cc: Peter Maydell, qemu-devel
On Tue, 29 Mar 2022 at 22:09, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> I believe I follow: because the walk uses walkstate.address.paddress.paspace, the ipa
> input parameter is unchanged, and it is ipa that is passed to
> AArch64.S2NextWalkStateLast() to form the output address.
>
Indeed, I initially found the issue when a test case passed on arm IP
but raised a data abort in QEMU.
Since fixing this issue solved the inconsistency, I believe this is
the intended behaviour and not a spec bug.
>
> r~
Idan Horowitz
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] target/arm: Determine final stage 2 output PA space based on original IPA
2022-03-29 19:09 ` Richard Henderson
2022-03-29 20:18 ` Idan Horowitz
@ 2022-03-31 8:46 ` Peter Maydell
1 sibling, 0 replies; 12+ messages in thread
From: Peter Maydell @ 2022-03-31 8:46 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-arm, Idan Horowitz, qemu-devel
On Tue, 29 Mar 2022 at 20:09, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/27/22 03:34, Idan Horowitz wrote:
> > As per the AArch64.S2Walk() psuedo-code in the ARMv8 ARM, the final
> > decision as to the output address's PA space based on the SA/SW/NSA/NSA
> > bits needs to take the input IPA's PA space into account, and not the
> > PA space of the result of the stage 2 walk itself.
> >
> > Signed-off-by: Idan Horowitz <idan.horowitz@gmail.com>
>
> I believe I follow: because the walk uses walkstate.address.paddress.paspace, the ipa
> input parameter is unchanged, and it is ipa that is passed to
> AArch64.S2NextWalkStateLast() to form the output address.
Textually, this is described on page D5-4802 of DDI 0487H.a;
the security of the output address of the memory access isn't
affected by the security of the output address of the translation
table walk.
-- PMM
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] Bug fixes related to secure 2 stage translation
2022-03-27 9:34 [PATCH 0/3] Bug fixes related to secure 2 stage translation Idan Horowitz
` (2 preceding siblings ...)
2022-03-27 9:34 ` [PATCH 3/3] target/arm: Determine final stage 2 output PA space based on original IPA Idan Horowitz
@ 2022-03-31 8:46 ` Peter Maydell
3 siblings, 0 replies; 12+ messages in thread
From: Peter Maydell @ 2022-03-31 8:46 UTC (permalink / raw)
To: Idan Horowitz; +Cc: qemu-arm, qemu-devel
On Sun, 27 Mar 2022 at 10:34, Idan Horowitz <idan.horowitz@gmail.com> wrote:
>
>
> Idan Horowitz (3):
> target/arm: Check VSTCR.SW when assigning the stage 2 output PA space
> target/arm: Take VSTCR.SW, VTCR.NSW into account in final stage 2 walk
> target/arm: Determine final stage 2 output PA space based on original
> IPA
>
Applied to target-arm.next, thanks. (I fixed a couple of typos in
the patch 3 commit message.)
-- PMM
^ permalink raw reply [flat|nested] 12+ messages in thread