Re: [Xen-devel] [PATCH v2 11/13] memory: add get_paged_gfn() as a wrapper...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: George Dunlap [mailto:george.dunlap@xxxxxxxxxx]
> Sent: 11 July 2018 14:05
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Jan Beulich <jbeulich@xxxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>;
> Julien Grall <julien.grall@xxxxxxx>; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Tim
> (Xen.org) <tim@xxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>
> Subject: Re: [PATCH v2 11/13] memory: add get_paged_gfn() as a wrapper...
> 
> On 07/11/2018 01:31 PM, Paul Durrant wrote:
> >>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> >>> index c6b99c4116..510f37f100 100644
> >>> --- a/xen/common/grant_table.c
> >>> +++ b/xen/common/grant_table.c
> >>> @@ -375,39 +375,23 @@ static int get_paged_frame(unsigned long gfn,
> >> mfn_t *mfn,
> >>>                             struct page_info **page, bool readonly,
> >>>                             struct domain *rd)
> >>>  {
> >>> -    int rc = GNTST_okay;
> >>> -    p2m_type_t p2mt;
> >>> -
> >>> -    *mfn = INVALID_MFN;
> >>> -    *page = get_page_from_gfn(rd, gfn, &p2mt,
> >>> -                              readonly ? P2M_ALLOC : P2M_UNSHARE);
> >>> -    if ( !*page )
> >>> -    {
> >>> -#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;
> >>> -    }
> >>> +    int rc;
> >>>
> >>> -    if ( p2m_is_foreign(p2mt) )
> >> [snip]
> >>>      {
> >> [snip]
> >>> -        put_page(*page);
> >>> -        *page = NULL;
> >>> -
> >>
> >> Comparing before-and-after, this seems to remove this
> 'p2m_is_foreign()'
> >> check.  Am I missing something?
> >>
> >
> > I may be. I thought p2m_is_ram() ruled out foreign pages
> (p2m_is_any_ram() being the way to include foreign maps if required). I'll
> check.
> 
> Looks like you're right.  But then, are you sure that's what we want for
> the other callers?  Might we not need to do an emulation that ends up
> writing into a foreign mapping, for instance?

If we do then I'd expect the emulation to know the domid that owns the page and 
thus the page would not be foreign to the specified domid. In fact, for x86 at 
least, get_page_from_gfn() will fail unless the domid of the page owner is 
specified so there's no prospect of the page being foreign in the p2mt check 
anyway.

  Paul 

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