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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of
> Julien Grall
> Sent: 18 September 2017 17:28
> To: xen-devel@xxxxxxxxxxxxx
> Cc: sstabellini@xxxxxxxxxx; Wei Liu <wei.liu2@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Tim
> (Xen.org) <tim@xxxxxxx>; Julien Grall <julien.grall@xxxxxxx>;
> jbeulich@xxxxxxxx
> Subject: [Xen-devel] [PATCH v2] xen: grant-table: Simplify get_paged_frame
> 
> 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?

That seems reasonable to me. You can't grant frames that have been grant mapped 
from another domain (except by using v2 transitive grants) so disallowing 
granting of priv mapped frames is at least consistent.

I do wonder whether this function belongs in the grant table code though. 
Getting the page from a (d, gfn) tuple is probably something that's needed in a 
few places and hence putting the code in common/memory.c (with suitable 
adjustment to the error values) would seem more appropriate.

  Paul

> ---
>  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,
> @@ -267,26 +266,24 @@ static int get_paged_frame(unsigned long gfn,
> unsigned long *frame,
>      if ( !(*page) )
>      {
>          *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;
> +
>      *frame = page_to_mfn(*page);
> -#else
> -    *frame = mfn_x(gfn_to_mfn(rd, _gfn(gfn)));
> -    *page = mfn_valid(_mfn(*frame)) ? mfn_to_page(*frame) : NULL;
> -    if ( (!(*page)) || (!get_page(*page, rd)) )
> -    {
> -        *frame = mfn_x(INVALID_MFN);
> -        *page = NULL;
> -        rc = GNTST_bad_page;
> -    }
> -#endif
> 
>      return rc;
>  }
> --
> 2.11.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel