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

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@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

---
    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(-)

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@arm.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

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
>

Hi Stefano,

On 11/06/2019 23:35, Stefano Stabellini wrote:
> 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@arm.com>
>> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
> 
> 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?

The end goal is to streamline as much as possible to page-table update. I wanted 
to have all the information in flags because it is much easier to reason with 
one variable over two variables.

Furthermore, x86 code allows map_pages_to_xen(...) to destroy mappings but not 
the underlying page-tables. This is used for instance for the vunmap to avoid 
re-creating the page-tables afterwards. I have been thinking to introduce 
similar things on Arm.

Keeping the xenmap_operation would make it awkward to support it because you 
would have to translate the flags to the actual operations.


>> ---
>>      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

Better alternative, I will update the comment.

> 
> 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:

To be honest, I was not entirely happy with the current comment. But I also 
wasn't able to find a better one :).

> 
>   [6] Only populate page tables
I am happy with this alternative. I will update the comment.

Cheers,