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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Sep 19, 2017 at 12:22:28PM +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.
> 
> Lastly remove pointless parenthesis in the code modified.
> 
> 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 v3:
>         - Add missing put_page in the error path
>         - Remove pointless parenthesis
> 
>     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 | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index c3895e6201..b7deb57b85 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -259,34 +259,34 @@ 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,
> -                              (readonly) ? P2M_ALLOC : P2M_UNSHARE);
> -    if ( !(*page) )
> +                              readonly ? P2M_ALLOC : P2M_UNSHARE);
> +    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;
>      }
> -    *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)) )
> +
> +    if ( p2m_is_foreign(p2mt) )
>      {
> -        *frame = mfn_x(INVALID_MFN);
> -        *page = NULL;
> -        rc = GNTST_bad_page;
> +        put_page(*page);

Please set page to NULL and frame to INVALID_MFN to match the comment of
the function.

I suppose you can set *frame = INVALID_MFN at the beginning of the
function to avoid code duplication in two error paths.

With that:

Reviewed-by: Wei Liu <wei.liu2@xxxxxxxxxx>

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