Re: [RFC PATCH v2 1/8] arm/mem_access: Add (TCR_|TTBCR_)* defines

From: Julien Grall <julien.grall@arm.com>
To: Sergej Proskurin <proskurin@sec.in.tum.de>,
	xen-devel@lists.xenproject.org
Cc: Stefano Stabellini <sstabellini@kernel.org>
Subject: Re: [RFC PATCH v2 1/8] arm/mem_access: Add (TCR_|TTBCR_)* defines
Date: Fri, 2 Jun 2017 08:31:03 +0100	[thread overview]
Message-ID: <2d851af9-4429-f14c-7946-e4c955111721@arm.com> (raw)
In-Reply-To: <20170601151906.10213-2-proskurin@sec.in.tum.de>

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel