| Message ID | 20170430194838.29932-3-proskurin@sec.in.tum.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sergej, On 30/04/17 20:48, Sergej Proskurin wrote: > The TTBCR_SZ holds only 3 bits and thus must be masked with the value > 0x7 instead of the previously used value 0xf. Please quote the spec (paragaph + version) when you do a such change. TTBCR_* flags are used for both TCR_EL1 (AArch64) and TTBCR (AArch32). Looking at the spec (ARM DDI 0487A.k_iss10775) TCR_EL1.{T0SZ,T1SZ) is encoded on 6 bits and TTBCR_EL1.{T0SZ,T1SZ} is encoded on 3 bits, with the following 3 bits RES0. So the mask here should be 0x3f. Cheers, > > Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> > --- > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Julien Grall <julien.grall@arm.com> > --- > xen/include/asm-arm/processor.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h > index 4fdf86070b..c8b8cff311 100644 > --- a/xen/include/asm-arm/processor.h > +++ b/xen/include/asm-arm/processor.h > @@ -94,7 +94,7 @@ > #define TTBCR_N_2KB _AC(0x03,U) > #define TTBCR_N_1KB _AC(0x04,U) > > -#define TTBCR_SZ_MASK 0xf > +#define TTBCR_SZ_MASK _AC(0x7,UL) > > /* SCTLR System Control Register. */ > /* HSCTLR is a subset of this. */ >
On 02/05/17 12:56, Julien Grall wrote: > Hi Sergej, > > On 30/04/17 20:48, Sergej Proskurin wrote: >> The TTBCR_SZ holds only 3 bits and thus must be masked with the value >> 0x7 instead of the previously used value 0xf. > > Please quote the spec (paragaph + version) when you do a such change. > > TTBCR_* flags are used for both TCR_EL1 (AArch64) and TTBCR (AArch32). > Looking at the spec (ARM DDI 0487A.k_iss10775) TCR_EL1.{T0SZ,T1SZ) is > encoded on 6 bits and TTBCR_EL1.{T0SZ,T1SZ} is encoded on 3 bits, with > the following 3 bits RES0. > > So the mask here should be 0x3f. Hmmm, I have just noticed we do a mix of TTBCR and TCR in the code. I would prefer if we use only TCR_* everywhere. Cheers,
Hi Julien, On 05/02/2017 01:56 PM, Julien Grall wrote: > Hi Sergej, > > On 30/04/17 20:48, Sergej Proskurin wrote: >> The TTBCR_SZ holds only 3 bits and thus must be masked with the value >> 0x7 instead of the previously used value 0xf. > > Please quote the spec (paragaph + version) when you do a such change. > > TTBCR_* flags are used for both TCR_EL1 (AArch64) and TTBCR (AArch32). > Looking at the spec (ARM DDI 0487A.k_iss10775) TCR_EL1.{T0SZ,T1SZ) is > encoded on 6 bits and TTBCR_EL1.{T0SZ,T1SZ} is encoded on 3 bits, with > the following 3 bits RES0. > > So the mask here should be 0x3f. > That's fair, thanks. It is already part of my v2 patch. Cheers, ~Sergej
Hi Julien, On 05/02/2017 02:01 PM, Julien Grall wrote: > > > On 02/05/17 12:56, Julien Grall wrote: >> Hi Sergej, >> >> On 30/04/17 20:48, Sergej Proskurin wrote: >>> The TTBCR_SZ holds only 3 bits and thus must be masked with the value >>> 0x7 instead of the previously used value 0xf. >> >> Please quote the spec (paragaph + version) when you do a such change. >> >> TTBCR_* flags are used for both TCR_EL1 (AArch64) and TTBCR (AArch32). >> Looking at the spec (ARM DDI 0487A.k_iss10775) TCR_EL1.{T0SZ,T1SZ) is >> encoded on 6 bits and TTBCR_EL1.{T0SZ,T1SZ} is encoded on 3 bits, with >> the following 3 bits RES0. >> >> So the mask here should be 0x3f. > > Hmmm, I have just noticed we do a mix of TTBCR and TCR in the code. I > would prefer if we use only TCR_* everywhere. > Thank you. I have adopted my current implementation so that it uses solely TCR_* defines. This is fine for the current implementation as it supports only Aarch64 and the long-descriptor translation table format of Aarch32/ARMv7. Yet, as soon as we provide support for the short-descriptor translation table format, I believe it would make sense to make use of the TTBCR_* defines, as well. Otherwise, the implementation would mixup the TCR_* with, e.g., the TTBCR_N_MASK defines. Because of this, I would suggest to use the TTBCR_* when it comes to the short-descriptor format. What do you think about that? Also, in order to reduce the complexity of the gpt-walk function, it would probably make sense to move all short-descriptor translation table related operations out of the suggested function in the patch 4/4. Cheers, ~Sergej
On 08/05/17 07:40, Sergej Proskurin wrote: > Hi Julien, > > > On 05/02/2017 02:01 PM, Julien Grall wrote: >> >> >> On 02/05/17 12:56, Julien Grall wrote: >>> Hi Sergej, >>> >>> On 30/04/17 20:48, Sergej Proskurin wrote: >>>> The TTBCR_SZ holds only 3 bits and thus must be masked with the value >>>> 0x7 instead of the previously used value 0xf. >>> >>> Please quote the spec (paragaph + version) when you do a such change. >>> >>> TTBCR_* flags are used for both TCR_EL1 (AArch64) and TTBCR (AArch32). >>> Looking at the spec (ARM DDI 0487A.k_iss10775) TCR_EL1.{T0SZ,T1SZ) is >>> encoded on 6 bits and TTBCR_EL1.{T0SZ,T1SZ} is encoded on 3 bits, with >>> the following 3 bits RES0. >>> >>> So the mask here should be 0x3f. >> >> Hmmm, I have just noticed we do a mix of TTBCR and TCR in the code. I >> would prefer if we use only TCR_* everywhere. >> > > Thank you. I have adopted my current implementation so that it uses > solely TCR_* defines. > > This is fine for the current implementation as it supports only Aarch64 > and the long-descriptor translation table format of Aarch32/ARMv7. Yet, > as soon as we provide support for the short-descriptor translation table > format, I believe it would make sense to make use of the TTBCR_* > defines, as well. Otherwise, the implementation would mixup the TCR_* > with, e.g., the TTBCR_N_MASK defines. Because of this, I would suggest > to use the TTBCR_* when it comes to the short-descriptor format. What do > you think about that? > > Also, in order to reduce the complexity of the gpt-walk function, it > would probably make sense to move all short-descriptor translation table > related operations out of the suggested function in the patch 4/4. It would be indeed better. But in that case, the LPAE page walk should be moved in a separate function too. The current helper would just select the correct one to call. Cheers,
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h index 4fdf86070b..c8b8cff311 100644 --- a/xen/include/asm-arm/processor.h +++ b/xen/include/asm-arm/processor.h @@ -94,7 +94,7 @@ #define TTBCR_N_2KB _AC(0x03,U) #define TTBCR_N_1KB _AC(0x04,U) -#define TTBCR_SZ_MASK 0xf +#define TTBCR_SZ_MASK _AC(0x7,UL) /* SCTLR System Control Register. */ /* HSCTLR is a subset of this. */
The TTBCR_SZ holds only 3 bits and thus must be masked with the value 0x7 instead of the previously used value 0xf. Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> --- Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Julien Grall <julien.grall@arm.com> --- xen/include/asm-arm/processor.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)