Re: [Xen-devel] [PATCH v2] xen: grant-table: Simplify get_paged_frame

[Julien Grall <julien.grall@xxxxxxx>]

Hi Wei,

On 19/09/17 11:38, Wei Liu wrote:
> On Mon, Sep 18, 2017 at 05:27:52PM +0100, Julien Grall wrote:
> > The implementation of get_paged_frame is currently different whether the
> > architecture support sharing memory or paging memory. Both
> > version are extremely similar so it is possible to consolidate in a
> > single implementation.
> >
> > The main difference is the x86 version will allow grant on foreign page
> > when using HVM/PVH whilst Arm does not. At the moment, on x86 foreign pages
> > are only allowed for PVH Dom0. It seems that foreign pages should never
> > be granted so deny them
> >
> > The check for shared/paged memory are now gated with the respective ifdef.
> > Potentially, dummy p2m_is_shared/p2m_is_paging could be implemented for
> > Arm.
> >
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> >
> > ---
> >
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > Cc: Tim Deegan <tim@xxxxxxx>
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> >
> >      Changes in v2:
> >          - Deny grant on foreign page (aligned with the ARM code)
> >          - Use #ifdef rather than #if defined
> >          - Update commit message
> >          - Fix typo in the title
> >
> > get_page_from_gfn will be able to get reference on foreign page and as
> > per my understanding will allow to grant page on foreign memory.
> >
> > This was not allowed with a simple get_page(...) on the ARM
> > implementation (no sharing nor paging supprot) but is allowed on the x86
> > implementation due to get_page_from_gfn.
> >
> > On x86, foreign pages are currently only allowed for PVH dom0, so I
> > think it is not a big deal for now.
> >
> > On Arm, foreign pages can be present on any domain. So this patch would
> > permit grant on foreing pages.
> >
> > This patch will deny granting foreign pages. Jan Beulich is happy with
> > it. Any other opinions?
> > ---
> >   xen/common/grant_table.c | 19 ++++++++-----------
> >   1 file changed, 8 insertions(+), 11 deletions(-)
> >
> > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> > index c3895e6201..a6a168df6e 100644
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -259,7 +259,6 @@ static int get_paged_frame(unsigned long gfn, unsigned long 
> > *frame,
> >                              struct domain *rd)
> >   {
> >       int rc = GNTST_okay;
> > -#if defined(P2M_PAGED_TYPES) || defined(P2M_SHARED_TYPES)
> >       p2m_type_t p2mt;
> >   
> >       *page = get_page_from_gfn(rd, gfn, &p2mt,
> While you're at it, mind dropping the pointless brackets around
> readonly?
Will do.

> > @@ -267,26 +266,24 @@ static int get_paged_frame(unsigned long gfn, unsigned 
> > long *frame,
> >       if ( !(*page) )
> And here as well.
>
> >       {
> >           *frame = mfn_x(INVALID_MFN);
> > +#ifdef P2M_SHARED_TYPES
> >           if ( p2m_is_shared(p2mt) )
> >               return GNTST_eagain;
> > +#endif
> > +#ifdef P2M_PAGES_TYPES
> >           if ( p2m_is_paging(p2mt) )
> >           {
> >               p2m_mem_paging_populate(rd, gfn);
> >               return GNTST_eagain;
> >           }
> > +#endif
> >           return GNTST_bad_page;
> >       }
> > +
> > +    if ( p2m_is_foreign(p2mt) )
> > +        return GNTST_bad_page;
> You only get here when you have taken a ref on the page. You should drop
> the ref before returning.
Hmmm yes. I will send a new version with that fixed.

Cheers,

--
Julien Grall

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