Re: [RFC PATCH v2 4/8] arm/mem_access: Add short-descriptor pte typedefs

[Julien Grall <julien.grall@arm.com>]

Hi Sergej,

On 06/01/2017 04:18 PM, Sergej Proskurin wrote:
> The current implementation does not provide appropriate types for
> short-descriptor translation table entries. As such, this commit adds new
> types, which simplify managing the respective translation table entries.
> 
> 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/page.h | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
> 
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 6222b1d4a2..5ea97ba95b 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -205,6 +205,25 @@ typedef union {
>       lpae_walk_t walk;
>   } lpae_t;
>   
> +/*
> + * Comprises the bits required to walk page tables adhering to the
> + * short-descriptor translation table format.
> + */
> +typedef struct __packed {
> +    unsigned int dt:2;          /* Descriptor type */
> +    unsigned int pad1:8;
> +    unsigned int base:22;       /* Base address of block or next table */

This is clearly confusing. The base address size varies with the level 
you are currently walking. Without looking at the code, I can guess that 
you would need shift/mask to adapt the base address. This is a call for 
providing structure for each level (there is only 2 anyway).

> +} pte_sd_walk_t;

I am a bit surprised this is the only bits you required for the walking 
as you also need to return the permissions.

Looking at the patch doing the implement in patch #7, there is a lot of 
hardcoding value. This is a call for a better structure definition here.

> +/*
> + * Represents page table entries adhering to the short-descriptor translation
> + * table format.
> + */
> +typedef union {
> +    uint32_t bits;
> +    pte_sd_walk_t walk;
> +} pte_sd_t;
> +
>   /* Standard entry type that we'll use to build Xen's own pagetables.
>    * We put the same permissions at every level, because they're ignored
>    * by the walker in non-leaf entries. */
> 

Cheers,

-- 
Julien Grall

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