| Message ID | 1457645293-4625-1-git-send-email-shankerd@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Shanker, On 11/03/2016 04:28, Shanker Donthineni wrote: > The maximum and minimum values for T0SZ depend on level of > translation as per AArch64 Virtual Memory System Architecture. > The current code sets T0SZ to zero in TCR2_EL2 which is not s/TCR2_EL2/TCR_EL2/ > valid and also might see unexpected behavior on some CPUs. Can you provide more details? I looked at the specification, programming the field T0SZ to 0 is valid (see D4-1463 ARM DDI 0487A.b): "For a stage 1 translation The minimum TxSZ value is 16. If TxSZ is programmed to a value smaller than 16 then the implementation behaves as if the field were programmed to 16 for all purposes other than reading back the value of the field." > This patch sets T0SZ to (64-48)bits since XEN uses all 4 levels > to cover 48bit (256TB) virtual address space. > > Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org> > --- > xen/arch/arm/arm64/head.S | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 19fa2bb..28ee404 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -343,7 +343,7 @@ skip_bss: > * PT walks use Inner-Shareable accesses, > * PT walks are write-back, write-allocate in both cache levels, > * Full 64-bit address space goes through this table. */ This comment is no longer valid with your changes. Please update it. > - ldr x0, =(TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(0)) > + ldr x0, =(TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(64-48)) > /* ID_AA64MMFR0_EL1[3:0] (PARange) corresponds to TCR_EL2[18:16] (PS) */ > mrs x1, ID_AA64MMFR0_EL1 > bfi x0, x1, #16, #3 > Regards,
HI Jullen, On 03/12/2016 07:13 AM, Julien Grall wrote: > Hi Shanker, > > On 11/03/2016 04:28, Shanker Donthineni wrote: >> The maximum and minimum values for T0SZ depend on level of >> translation as per AArch64 Virtual Memory System Architecture. >> The current code sets T0SZ to zero in TCR2_EL2 which is not > > s/TCR2_EL2/TCR_EL2/ > Sorry for typo, I will fix in next patch >> valid and also might see unexpected behavior on some CPUs. > > Can you provide more details? > We are not able to boot XEN on Qualcomm platforms and CPU hung after after executing this line of ASM code. > I looked at the specification, programming the field T0SZ to 0 is valid > (see D4-1463 ARM DDI 0487A.b): > > "For a stage 1 translation > The minimum TxSZ value is 16. If TxSZ is programmed to a value smaller > than 16 then the implementation behaves as if the field were programmed > to 16 for all purposes other than reading back the value of the field." > The behavior of T0SZ=0 is described in ARM spec (DDI0487A_h, page 1752). Still I think setting the T0SZ to 48bit is the right fix similar to LINUX KVM64 EL2 code. For a stage 1 translation The minimum TxSZ value is 16. If TxSZ is programmed to a value smaller than 16 then it is IMPLEMENTATION DEFINED whether: • The implementation behaves as if the field were programmed to 16 for all purposes other than reading back the value of the field. • Any use of the TxSZ value generates a stage 1 Level 0 Translation fault. >> This patch sets T0SZ to (64-48)bits since XEN uses all 4 levels >> to cover 48bit (256TB) virtual address space. >> >> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org> >> --- >> xen/arch/arm/arm64/head.S | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >> index 19fa2bb..28ee404 100644 >> --- a/xen/arch/arm/arm64/head.S >> +++ b/xen/arch/arm/arm64/head.S >> @@ -343,7 +343,7 @@ skip_bss: >> * PT walks use Inner-Shareable accesses, >> * PT walks are write-back, write-allocate in both cache levels, >> * Full 64-bit address space goes through this table. */ > > This comment is no longer valid with your changes. Please update it. > Sure, I will change to 48bit virtual address space. >> - ldr x0, =(TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(0)) >> + ldr x0, =(TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(64-48)) >> /* ID_AA64MMFR0_EL1[3:0] (PARange) corresponds to TCR_EL2[18:16] (PS) */ >> mrs x1, ID_AA64MMFR0_EL1 >> bfi x0, x1, #16, #3 >> > > Regards, >
On 14/03/16 14:37, Shanker Donthineni wrote: > HI Jullen, Hi Shanker, > On 03/12/2016 07:13 AM, Julien Grall wrote: >> Hi Shanker, >> >> On 11/03/2016 04:28, Shanker Donthineni wrote: >>> The maximum and minimum values for T0SZ depend on level of >>> translation as per AArch64 Virtual Memory System Architecture. >>> The current code sets T0SZ to zero in TCR2_EL2 which is not >> >> s/TCR2_EL2/TCR_EL2/ >> > > Sorry for typo, I will fix in next patch > >>> valid and also might see unexpected behavior on some CPUs. >> >> Can you provide more details? >> > > We are not able to boot XEN on Qualcomm platforms and CPU hung after > after executing this line of ASM code. > >> I looked at the specification, programming the field T0SZ to 0 is valid >> (see D4-1463 ARM DDI 0487A.b): >> >> "For a stage 1 translation >> The minimum TxSZ value is 16. If TxSZ is programmed to a value smaller >> than 16 then the implementation behaves as if the field were programmed >> to 16 for all purposes other than reading back the value of the field." >> > > The behavior of T0SZ=0 is described in ARM spec (DDI0487A_h, page 1752). Still I think setting > the T0SZ to 48bit is the right fix similar to LINUX KVM64 EL2 code. > > For a stage 1 translation > The minimum TxSZ value is 16. If TxSZ is programmed to a value smaller than 16 then it is > IMPLEMENTATION DEFINED whether: > > • The implementation behaves as if the field were programmed to 16 for all purposes other than > reading back the value of the field. > > • Any use of the TxSZ value generates a stage 1 Level 0 Translation fault. Sorry, I was looking at an older version of the ARM ARM where only a single possible behavior was described. So this change looks valid to me. Can you please mention the version and the section of the spec in the commit message? Regards,
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index 19fa2bb..28ee404 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -343,7 +343,7 @@ skip_bss: * PT walks use Inner-Shareable accesses, * PT walks are write-back, write-allocate in both cache levels, * Full 64-bit address space goes through this table. */ - ldr x0, =(TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(0)) + ldr x0, =(TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(64-48)) /* ID_AA64MMFR0_EL1[3:0] (PARange) corresponds to TCR_EL2[18:16] (PS) */ mrs x1, ID_AA64MMFR0_EL1 bfi x0, x1, #16, #3
The maximum and minimum values for T0SZ depend on level of translation as per AArch64 Virtual Memory System Architecture. The current code sets T0SZ to zero in TCR2_EL2 which is not valid and also might see unexpected behavior on some CPUs. This patch sets T0SZ to (64-48)bits since XEN uses all 4 levels to cover 48bit (256TB) virtual address space. Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org> --- xen/arch/arm/arm64/head.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)