Re: [Xen-devel] [PATCH 11/15] xen/arm: Allow lpae_is_{table, mapping} helpers to work on invalid entry

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 14 Aug 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 08/14/2018 10:33 PM, Stefano Stabellini wrote:
> > On Mon, 16 Jul 2018, Julien Grall wrote:
> > > Currently, lpae_is_{table, mapping} helpers will always return false on
> > > entry with the valid bit unset. However, it would be useful to have them
> >    ^entries
> > 
> > > operating on any entry. For instance to store information in advance but
> > > still request a fault.
> > > 
> > > With that change, the p2m is now providing an overlay for *_is_{table,
> > > mapping} that will check the valid bit of the entry.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > > 
> > > ---
> > >   xen/arch/arm/guest_walk.c  |  2 +-
> > >   xen/arch/arm/mm.c          |  2 +-
> > >   xen/arch/arm/p2m.c         | 22 ++++++++++++++++++----
> > >   xen/include/asm-arm/lpae.h | 11 +++++++----
> > >   4 files changed, 27 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> > > index e3e21bdad3..4a1b4cf2c8 100644
> > > --- a/xen/arch/arm/guest_walk.c
> > > +++ b/xen/arch/arm/guest_walk.c
> > > @@ -566,7 +566,7 @@ static int guest_walk_ld(const struct vcpu *v,
> > >        * PTE is invalid or holds a reserved entry (PTE<1:0> == x0)) or if
> > > the PTE
> > >        * maps a memory block at level 3 (PTE<1:0> == 01).
> > >        */
> > > -    if ( !lpae_is_mapping(pte, level) )
> > > +    if ( !lpae_is_valid(pte) || !lpae_is_mapping(pte, level) )
> > >           return -EFAULT;
> > >         /* Make sure that the lower bits of the PTE's base address are
> > > zero. */
> > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > > index e3dafe5fd7..52e57fef2f 100644
> > > --- a/xen/arch/arm/mm.c
> > > +++ b/xen/arch/arm/mm.c
> > > @@ -996,7 +996,7 @@ static int create_xen_entries(enum xenmap_operation
> > > op,
> > >       for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
> > >       {
> > >           entry = &xen_second[second_linear_offset(addr)];
> > > -        if ( !lpae_is_table(*entry, 2) )
> > > +        if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
> > >           {
> > >               rc = create_xen_table(entry);
> > >               if ( rc < 0 ) {
> > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > index ec3fdcb554..07925a1be4 100644
> > > --- a/xen/arch/arm/p2m.c
> > > +++ b/xen/arch/arm/p2m.c
> > > @@ -219,6 +219,20 @@ static p2m_access_t p2m_mem_access_radix_get(struct
> > > p2m_domain *p2m, gfn_t gfn)
> > >           return radix_tree_ptr_to_int(ptr);
> > >   }
> > >   +/*
> > > + * lpae_is_* helpers don't check whether the valid bit is set in the
> > > + * PTE. Provide our own overlay to check the valid bit.
> > > + */
> > > +static inline bool p2m_is_mapping(lpae_t pte, unsigned int level)
> > > +{
> > > +    return lpae_is_valid(pte) && lpae_is_mapping(pte, level);
> > > +}
> > > +
> > > +static inline bool p2m_is_superpage(lpae_t pte, unsigned int level)
> > > +{
> > > +    return lpae_is_valid(pte) && lpae_is_superpage(pte, level);
> > > +}
> > 
> > I like the idea, but it would be clearer to me if they were called
> > lpae_is_valid_mapping and lpae_is_valid_superpage respectively. What do
> > you think?
> >  > Also, we might as well move them to lpae.h and use them from mm.c and
> > guest_walk.c as appropriate?
> 
> lpae.h is not suitable because as I said in the commit message each page-table
> (stage-1, stage-2) may have a different view of what "valid" means.
> 
> At the moment, that view is the same but it is not going to stay long like
> that. Hence the reason of prefixing with p2m_. They are specific to the p2m.
> This is giving us some more liberty in the future while making the lpae code a
> bit more generic.
> 
> In guest_walk.c there are only one user, so the introduction of an helper is
> quite limited there.

I see, so by "p2m_is_mapping" you meant specifically
"stage2_is_mapping", right? That makes sense now.

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