Re: [Xen-devel] [PATCH v3 7/8] xen/arm: Rework the interface of p2m_lookup and use typesafe gfn and mfn

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 23/06/16 15:14, Stefano Stabellini wrote:
> On Tue, 21 Jun 2016, Julien Grall wrote:
> > The prototype and the declaration of p2m_lookup disagree on how the
> > function should be used. One expect a frame number whilst the other
> > an address.
> >
> > Thankfully, everyone is using with an address today. However, most of
> > the callers have to convert a guest frame to an address. Modify
> > the interface to take a guest physical frame in parameter and return
> > a machine frame.
> >
> > Whilst modifying the interface, use typesafe gfn and mfn for clarity
> > and catching possible misusage.
> >
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > ---
> >   xen/arch/arm/p2m.c        | 37 ++++++++++++++++++++-----------------
> >   xen/arch/arm/traps.c      | 21 +++++++++++----------
> >   xen/include/asm-arm/p2m.h |  7 +++----
> >   3 files changed, 34 insertions(+), 31 deletions(-)
> >
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index 47cb383..f3330dd 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -140,14 +140,15 @@ void flush_tlb_domain(struct domain *d)
> >   }
> >
> >   /*
> > - * Lookup the MFN corresponding to a domain's PFN.
> > + * Lookup the MFN corresponding to a domain's GFN.
> >    *
> >    * There are no processor functions to do a stage 2 only lookup therefore we
> >    * do a a software walk.
> >    */
> > -static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
> > +static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
> >   {
> >       struct p2m_domain *p2m = &d->arch.p2m;
> > +    const paddr_t paddr = pfn_to_paddr(gfn_x(gfn));
> >       const unsigned int offsets[4] = {
> >           zeroeth_table_offset(paddr),
> >           first_table_offset(paddr),
> > @@ -158,7 +159,7 @@ static paddr_t __p2m_lookup(struct domain *d, paddr_t 
> > paddr, p2m_type_t *t)
> >           ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK
> >       };
> >       lpae_t pte, *map;
> > -    paddr_t maddr = INVALID_PADDR;
> > +    mfn_t mfn = _mfn(INVALID_MFN);
> It might be worth defining INVALID_MFN_T and just assign that to mfn.
Good idea. It will be useful in other places too.

> >       paddr_t mask = 0;
> >       p2m_type_t _t;
> >       unsigned int level, root_table;

[...]

> > @@ -1561,11 +1565,10 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned 
> > long flag)
> >        * We had a mem_access permission limiting the access, but the page type
> >        * could also be limiting, so we need to check that as well.
> >        */
> > -    maddr = __p2m_lookup(current->domain, ipa, &t);
> > -    if ( maddr == INVALID_PADDR )
> > +    mfn = mfn_x(__p2m_lookup(current->domain, gfn, &t));
> > +    if ( mfn == INVALID_MFN )
> The conversion would go away if we had an INVALID_MFN which is mfn_t
Will do.

Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel