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

[Julien Grall <julien.grall@xxxxxxx>]

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@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?
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,

--
Julien Grall

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