| Message ID | 20170601151906.10213-2-proskurin@sec.in.tum.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sergej, On 06/01/2017 04:18 PM, Sergej Proskurin wrote: > This commit adds (TCR_|TTBCR_)* defines to simplify access to the respective > register contents. > > Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> > --- > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Julien Grall <julien.grall@arm.com> > --- > v2: Define TCR_SZ_MASK in a way so that it can be also applied to 32-bit guests > using the long-descriptor translation table format. > > Extend the previous commit by further defines allowing a simplified access > to the registers TCR_EL1 and TTBCR. > --- > xen/include/asm-arm/processor.h | 49 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h > index 855ded1b07..c095dad7e9 100644 > --- a/xen/include/asm-arm/processor.h > +++ b/xen/include/asm-arm/processor.h > @@ -94,6 +94,9 @@ > #define TTBCR_N_2KB _AC(0x03,U) > #define TTBCR_N_1KB _AC(0x04,U) > > +#define TTBCR_PD0 (_AC(1,U)<<4) > +#define TTBCR_PD1 (_AC(1,U)<<5) Looking at it, it is not clear whether the apply when EAE is set or not. In fact, they only apply when EAE is not set. This should at least be clear in a comment and potentially in the name. > + > /* SCTLR System Control Register. */ > /* HSCTLR is a subset of this. */ > #define SCTLR_TE (_AC(1,U)<<30) > @@ -155,6 +158,19 @@ > /* TCR: Stage 1 Translation Control */ > > #define TCR_T0SZ(x) ((x)<<0) Please replace the hardcode 0 by TCR_T0SZ_SHIFT at the same time (and mention it in the commit message). > +#define TCR_T0SZ_SHIFT (0) > +#define TCR_T1SZ_SHIFT (16) > + > +/* > + * According to ARM DDI 0487A.g, TCR_EL1.{T0SZ,T1SZ} (Aarch64, Section D7-2021) NIT: s/Aarch64/AArch64/ Also, 0487A.g is a 2 years old spec, there was quite a few revisions since then. Please download and quote the latest spec (i.e 0487B.a). > + * comprises 6 bits and TTBCR.{T0SZ,T1SZ} (Aarch32, Section G6-4624) comprises NIT: s/Aarch32/AArch64/ > + * 3 bits following another 3 bits for RES0. Thus, the mask for both registers > + * should be 0x3f. > + */ > +#define TCR_SZ_MASK (_AC(0x3f,UL)<<0) > + > +#define TCR_EPD0 (_AC(0x1,UL)<<7) > +#define TCR_EPD1 (_AC(0x1,UL)<<23) > > #define TCR_IRGN0_NC (_AC(0x0,UL)<<8) > #define TCR_IRGN0_WBWA (_AC(0x1,UL)<<8) > @@ -173,11 +189,35 @@ > #define TCR_TG0_4K (_AC(0x0,UL)<<14) > #define TCR_TG0_64K (_AC(0x1,UL)<<14) > #define TCR_TG0_16K (_AC(0x2,UL)<<14) > +#define TCR_TG0_MASK (_AC(0x3,UL)<<14) > +#define TCR_TG0_SHIFT (14) > + > +#define TCR_TG1_16K (_AC(0x1,UL)<<30) > +#define TCR_TG1_4K (_AC(0x2,UL)<<30) > +#define TCR_TG1_64K (_AC(0x3,UL)<<30) > +#define TCR_TG1_MASK (_AC(0x3,UL)<<30) > +#define TCR_TG1_SHIFT (30) > + > +#define TCR_IPS_32_BIT (_AC(0x0,ULL)<<32) > +#define TCR_IPS_36_BIT (_AC(0x1,ULL)<<32) > +#define TCR_IPS_40_BIT (_AC(0x2,ULL)<<32) > +#define TCR_IPS_42_BIT (_AC(0x3,ULL)<<32) > +#define TCR_IPS_44_BIT (_AC(0x4,ULL)<<32) > +#define TCR_IPS_48_BIT (_AC(0x5,ULL)<<32) > +#define TCR_IPS_MASK (_AC(0x7,ULL)<<32) > +#define TCR_IPS_SHIFT (32) The fields TG1 and IPS does not exist for AArch32. You should probably mention it at least in a comment. Also, a lot of the new defines you add are for TCR_EL1 and not TCR_EL2. Please make the distinction in the name to avoid misusing them. > + > +#define TCR_TB_31 (31) > > #ifdef CONFIG_ARM_64 > > #define TCR_PS(x) ((x)<<16) > #define TCR_TBI (_AC(0x1,UL)<<20) > +#define TCR_TBI0 (_AC(0x1,UL)<<37) > +#define TCR_TBI1 (_AC(0x1,UL)<<38) Those fields don't exist in TCR_EL2. > + > +#define TCR_TB_63 (63) > +#define TCR_TB_55 (55) Same here. > > #define TCR_RES1 (_AC(1,UL)<<31|_AC(1,UL)<<23) > > @@ -187,6 +227,15 @@ > > #endif > > +#define IPS_MIN (25) > +#define IPS_MAX (48) > +#define IPS_32_BIT (32) > +#define IPS_36_BIT (36) > +#define IPS_40_BIT (40) > +#define IPS_42_BIT (42) > +#define IPS_44_BIT (44) > +#define IPS_48_BIT (48) What is it for? Which register? > + > /* VTCR: Stage 2 Translation Control */ > > #define VTCR_T0SZ(x) ((x)<<0) > Cheers,
Hi Julien, [...] > > Also, a lot of the new defines you add are for TCR_EL1 and not TCR_EL2. > Please make the distinction in the name to avoid misusing them. > >> + >> +#define TCR_TB_31 (31) >> #ifdef CONFIG_ARM_64 >> #define TCR_PS(x) ((x)<<16) >> #define TCR_TBI (_AC(0x1,UL)<<20) >> +#define TCR_TBI0 (_AC(0x1,UL)<<37) >> +#define TCR_TBI1 (_AC(0x1,UL)<<38) > > Those fields don't exist in TCR_EL2. This is not entirely correct. All of the introduced fields are also available in TCR_EL2, however, only if HCR_EL2.E2H==1. I will comment that appropriately. Do you think that we should use nevertheless different names for the introduced defines? [...] Cheers, ~Sergej
On 07/06/17 15:56, Sergej Proskurin wrote: > Hi Julien, > > [...] > > >> >> Also, a lot of the new defines you add are for TCR_EL1 and not TCR_EL2. >> Please make the distinction in the name to avoid misusing them. >> >>> + >>> +#define TCR_TB_31 (31) >>> #ifdef CONFIG_ARM_64 >>> #define TCR_PS(x) ((x)<<16) >>> #define TCR_TBI (_AC(0x1,UL)<<20) >>> +#define TCR_TBI0 (_AC(0x1,UL)<<37) >>> +#define TCR_TBI1 (_AC(0x1,UL)<<38) >> >> Those fields don't exist in TCR_EL2. > > This is not entirely correct. All of the introduced fields are also > available in TCR_EL2, however, only if HCR_EL2.E2H==1. I will comment > that appropriately. Do you think that we should use nevertheless > different names for the introduced defines? This was added by ARMv8.1. HCR_EL2.E2H = 1 as is to run Host Operating System in EL2 (e.g for Type-2 hypervisor). This will very never be used by Xen. In any case, the naming gives the impression it also exists when HCR_EL2.E2H = 0. Cheers,
On 07/06/17 16:07, Julien Grall wrote: > > > On 07/06/17 15:56, Sergej Proskurin wrote: >> Hi Julien, >> >> [...] >> >> >>> >>> Also, a lot of the new defines you add are for TCR_EL1 and not TCR_EL2. >>> Please make the distinction in the name to avoid misusing them. >>> >>>> + >>>> +#define TCR_TB_31 (31) >>>> #ifdef CONFIG_ARM_64 >>>> #define TCR_PS(x) ((x)<<16) >>>> #define TCR_TBI (_AC(0x1,UL)<<20) >>>> +#define TCR_TBI0 (_AC(0x1,UL)<<37) >>>> +#define TCR_TBI1 (_AC(0x1,UL)<<38) >>> >>> Those fields don't exist in TCR_EL2. >> >> This is not entirely correct. All of the introduced fields are also >> available in TCR_EL2, however, only if HCR_EL2.E2H==1. I will comment >> that appropriately. Do you think that we should use nevertheless >> different names for the introduced defines? > > > This was added by ARMv8.1. HCR_EL2.E2H = 1 as is to run Host Operating > System in EL2 (e.g for Type-2 hypervisor). This will very never be used > by Xen. > > In any case, the naming gives the impression it also exists when > HCR_EL2.E2H = 0. To clarify my point. They should be named with TCR_EL1_TB* as this is how it is used in Xen. Cheers,
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h index 855ded1b07..c095dad7e9 100644 --- a/xen/include/asm-arm/processor.h +++ b/xen/include/asm-arm/processor.h @@ -94,6 +94,9 @@ #define TTBCR_N_2KB _AC(0x03,U) #define TTBCR_N_1KB _AC(0x04,U) +#define TTBCR_PD0 (_AC(1,U)<<4) +#define TTBCR_PD1 (_AC(1,U)<<5) + /* SCTLR System Control Register. */ /* HSCTLR is a subset of this. */ #define SCTLR_TE (_AC(1,U)<<30) @@ -155,6 +158,19 @@ /* TCR: Stage 1 Translation Control */ #define TCR_T0SZ(x) ((x)<<0) +#define TCR_T0SZ_SHIFT (0) +#define TCR_T1SZ_SHIFT (16) + +/* + * According to ARM DDI 0487A.g, TCR_EL1.{T0SZ,T1SZ} (Aarch64, Section D7-2021) + * comprises 6 bits and TTBCR.{T0SZ,T1SZ} (Aarch32, Section G6-4624) comprises + * 3 bits following another 3 bits for RES0. Thus, the mask for both registers + * should be 0x3f. + */ +#define TCR_SZ_MASK (_AC(0x3f,UL)<<0) + +#define TCR_EPD0 (_AC(0x1,UL)<<7) +#define TCR_EPD1 (_AC(0x1,UL)<<23) #define TCR_IRGN0_NC (_AC(0x0,UL)<<8) #define TCR_IRGN0_WBWA (_AC(0x1,UL)<<8) @@ -173,11 +189,35 @@ #define TCR_TG0_4K (_AC(0x0,UL)<<14) #define TCR_TG0_64K (_AC(0x1,UL)<<14) #define TCR_TG0_16K (_AC(0x2,UL)<<14) +#define TCR_TG0_MASK (_AC(0x3,UL)<<14) +#define TCR_TG0_SHIFT (14) + +#define TCR_TG1_16K (_AC(0x1,UL)<<30) +#define TCR_TG1_4K (_AC(0x2,UL)<<30) +#define TCR_TG1_64K (_AC(0x3,UL)<<30) +#define TCR_TG1_MASK (_AC(0x3,UL)<<30) +#define TCR_TG1_SHIFT (30) + +#define TCR_IPS_32_BIT (_AC(0x0,ULL)<<32) +#define TCR_IPS_36_BIT (_AC(0x1,ULL)<<32) +#define TCR_IPS_40_BIT (_AC(0x2,ULL)<<32) +#define TCR_IPS_42_BIT (_AC(0x3,ULL)<<32) +#define TCR_IPS_44_BIT (_AC(0x4,ULL)<<32) +#define TCR_IPS_48_BIT (_AC(0x5,ULL)<<32) +#define TCR_IPS_MASK (_AC(0x7,ULL)<<32) +#define TCR_IPS_SHIFT (32) + +#define TCR_TB_31 (31) #ifdef CONFIG_ARM_64 #define TCR_PS(x) ((x)<<16) #define TCR_TBI (_AC(0x1,UL)<<20) +#define TCR_TBI0 (_AC(0x1,UL)<<37) +#define TCR_TBI1 (_AC(0x1,UL)<<38) + +#define TCR_TB_63 (63) +#define TCR_TB_55 (55) #define TCR_RES1 (_AC(1,UL)<<31|_AC(1,UL)<<23) @@ -187,6 +227,15 @@ #endif +#define IPS_MIN (25) +#define IPS_MAX (48) +#define IPS_32_BIT (32) +#define IPS_36_BIT (36) +#define IPS_40_BIT (40) +#define IPS_42_BIT (42) +#define IPS_44_BIT (44) +#define IPS_48_BIT (48) + /* VTCR: Stage 2 Translation Control */ #define VTCR_T0SZ(x) ((x)<<0)
This commit adds (TCR_|TTBCR_)* defines to simplify access to the respective register contents. Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> --- Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Julien Grall <julien.grall@arm.com> --- v2: Define TCR_SZ_MASK in a way so that it can be also applied to 32-bit guests using the long-descriptor translation table format. Extend the previous commit by further defines allowing a simplified access to the registers TCR_EL1 and TTBCR. --- xen/include/asm-arm/processor.h | 49 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)