Re: [Xen-devel] [PATCH MM-PART3 v2 10/12] xen/arm: mm: Rework Xen page-tables walk during update

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 13 Jun 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 6/12/19 11:52 PM, Stefano Stabellini wrote:
> > On Tue, 14 May 2019, Julien Grall wrote:
> > > Currently, xen_pt_update_entry() is only able to update the region covered
> > > by xen_second (i.e 0 to 0x7fffffff).
> > > 
> > > Because of the restriction we end to have multiple functions in mm.c
> > > modifying the page-tables differently.
> > > 
> > > Furthermore, we never walked the page-tables fully. This means that any
> > > change in the layout may requires major rewrite of the page-tables code.
> > > 
> > > Lastly, we have been quite lucky that no one ever tried to pass an address
> > > outside this range because it would have blown-up.
> > > 
> > > xen_pt_update_entry() is reworked to walk over the page-tables every
> > > time. The logic has been borrowed from arch/arm/p2m.c and contain some
> > > limitations for the time being:
> > >      - Superpage cannot be shattered
> > >      - Only level 3 (i.e 4KB) can be done
> > > 
> > > Note that the parameter 'addr' has been renamed to 'virt' to make clear
> > > we are dealing with a virtual address.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > > Reviewed-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
> > > 
> > > ---
> > >      Changes in v2:
> > >          - Add Andrii's reviewed-by
> > > ---
> > >   xen/arch/arm/mm.c | 121
> > > +++++++++++++++++++++++++++++++++++++++++++++++-------
> > >   1 file changed, 106 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > > index f5979f549b..9a40754f44 100644
> > > --- a/xen/arch/arm/mm.c
> > > +++ b/xen/arch/arm/mm.c
> > > @@ -984,6 +984,53 @@ static void xen_unmap_table(const lpae_t *table)
> > >       unmap_domain_page(table);
> > >   }
> > >   +#define XEN_TABLE_MAP_FAILED 0
> > > +#define XEN_TABLE_SUPER_PAGE 1
> > > +#define XEN_TABLE_NORMAL_PAGE 2
> > 
> > Minor NIT: do we want to have XEN_TABLE_MAP_FAILED be -1 to follow the
> > pattern that errors are < 0 ? Not important though.
> 
> The value of XEN_TABLE_* here does not matter, you can see it as an open-coded
> enum. This was borrowed from arm/p2m.c (which was based on x86/mm/p2m-pt.c).
> 
> For the time being, I would prefer to keep it as is because it makes easier to
> spot the difference with the p2m code. I can consider switching the two to
> enum afterwards.

OK


> > 
> > > +/*
> > > + * Take the currently mapped table, find the corresponding entry,
> > > + * and map the next table, if available.
> > > + *
> > > + * The read_only parameters indicates whether intermediate tables should
> > > + * be allocated when not present.
> > 
> > I wonder if it would be a good idea to rename read_only to something
> > more obviously connected to the idea that tables get created. Maybe
> > create_missing? It would have to match the variable and comment added
> > below in xen_pt_update_entry. I don't have a strong opinion on this.
> 
> Same as above here, the comment is a replicate of p2m.c
 
OK


> > > + * Return values:
> > > + *  XEN_TABLE_MAP_FAILED: Either read_only was set and the entry
> > > + *  was empty, or allocating a new page failed.
> > > + *  XEN_TABLE_NORMAL_PAGE: next level mapped normally
> > > + *  XEN_TABLE_SUPER_PAGE: The next entry points to a superpage.
> > > + */
> > > +static int xen_pt_next_level(bool read_only, unsigned int level,
> > > +                             lpae_t **table, unsigned int offset)
> > > +{
> > > +    lpae_t *entry;
> > > +    int ret;
> > > +
> > > +    entry = *table + offset;
> > > +
> > > +    if ( !lpae_is_valid(*entry) )
> > > +    {
> > > +        if ( read_only )
> > > +            return XEN_TABLE_MAP_FAILED;
> > > +
> > > +        ret = create_xen_table(entry);
> > > +        if ( ret )
> > > +            return XEN_TABLE_MAP_FAILED;
> > > +    }
> > > +
> > > +    ASSERT(lpae_is_valid(*entry));
> > 
> > Why the ASSERT just after the lpae_is_valid check above?
> 
> When the entry is invalid, the new page table will be allocated and the entry
> will be generated. The rest of the function will then be executed. The
> ASSERT() here confirms the entry we have in hand is valid in all the cases.

So it's to double-check that after getting into the `if' statement, the
entry becomes valid, which is kind of redundant due to the two errors
check above but it is still valid. OK.

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