Re: [Xen-devel] [PATCH MM-PART3 v2 05/12] xen/arm: mm: Introduce _PAGE_PRESENT and _PAGE_POPULATE

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 14 May 2019, Julien Grall wrote:
> At the moment, the flags are not enough to describe what kind of update
> will done on the VA range. They need to be used in conjunction with the
> enum xenmap_operation.
> 
> It would be more convenient to have all the information for the update
> in a single place.
> 
> Two new flags are added to remove the relience on xenmap_operation:
>     - _PAGE_PRESENT: Indicate whether we are adding/removing the mapping
>     - _PAGE_POPULATE: Indicate whether we only populate page-tables
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> Reviewed-by: Andrii Anisov <andrii_anisov@xxxxxxxx>

Looking ahead in this series, I know that this is done so that later on
you can remove enum xenmap_operation. But what is the end goal? Why do
we want to remove enum xenmap_operation? I guess it is to make the code
more reusable?


> ---
>     Changes in v2:
>         - Add Andrii's reviewed-by
> ---
>  xen/arch/arm/mm.c          | 2 +-
>  xen/include/asm-arm/page.h | 9 +++++++--
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 9de2a1150f..2192dede55 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1083,7 +1083,7 @@ int map_pages_to_xen(unsigned long virt,
>  
>  int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
>  {
> -    return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, 0);
> +    return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, 
> _PAGE_POPULATE);
>  }
>  
>  int destroy_xen_mappings(unsigned long v, unsigned long e)
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 2bcdb0f1a5..caf2fac1ff 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -76,6 +76,8 @@
>   *
>   * [0:2] Memory Attribute Index
>   * [3:4] Permission flags
> + * [5]   Present bit
> + * [6]   Populate page table

[5] is pretty clear. As a nit, I would probably write:

 [5] Page Present

because there is no need to say "bit", the [5] means it is a bit.
Otherwise, something like the following:

 [5] Present in memory

but it's unimportant.

It's [6] that we might want to explain a bit better: if we say "Populate
page table" then every time we want the Xen pagetable to be populated we
would need to pass _PAGE_POPULATE, even when the page is present in
memory. Do you see what I mean? I am not entirely sure how to make it
clearer. Maybe:

 [6] Only populate page tables

"Only" being the key word.


>   */
>  #define PAGE_AI_MASK(x) ((x) & 0x7U)
>  
> @@ -86,12 +88,15 @@
>  #define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x1U)
>  #define PAGE_RO_MASK(x) (((x) >> _PAGE_RO_BIT) & 0x1U)
>  
> +#define _PAGE_PRESENT    (1U << 5)
> +#define _PAGE_POPULATE   (1U << 6)
> +
>  /*
>   * _PAGE_DEVICE and _PAGE_NORMAL are convenience defines. They are not
>   * meant to be used outside of this header.
>   */
> -#define _PAGE_DEVICE    _PAGE_XN
> -#define _PAGE_NORMAL    MT_NORMAL
> +#define _PAGE_DEVICE    (_PAGE_XN|_PAGE_PRESENT)
> +#define _PAGE_NORMAL    (MT_NORMAL|_PAGE_PRESENT)
>  
>  #define PAGE_HYPERVISOR_RO      (_PAGE_NORMAL|_PAGE_RO|_PAGE_XN)
>  #define PAGE_HYPERVISOR_RX      (_PAGE_NORMAL|_PAGE_RO)
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel