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

Message ID 20170919112228.22566-1-julien.grall@arm.com
State New
Headers show
Series
  • [Xen-devel,v3] xen: grant-table: Simplify get_paged_frame
Related show

Commit Message

Julien Grall Sept. 19, 2017, 11:22 a.m.
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@arm.com>

---

Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>

    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(-)

Comments

Wei Liu Sept. 19, 2017, 11:26 a.m. | #1
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@arm.com>
> 
> ---
> 
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> 
>     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@citrix.com>
Julien Grall Sept. 19, 2017, 11:34 a.m. | #2
On 19/09/17 12:26, Wei Liu wrote:
> On Tue, Sep 19, 2017 at 12:22:28PM +0100, Julien Grall wrote:
>> 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@citrix.com>

Would the below patch be fine?

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index c3895e6201..dc1bacacb0 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -259,34 +259,36 @@ 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;
 
+    *frame = mfn_x(INVALID_MFN);
     *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);
+        put_page(*page);
         *page = NULL;
-        rc = GNTST_bad_page;
+
+        return GNTST_bad_page;
     }
-#endif
+
+    *frame = page_to_mfn(*page);
 
     return rc;
 }
Wei Liu Sept. 19, 2017, 11:40 a.m. | #3
On Tue, Sep 19, 2017 at 12:34:28PM +0100, Julien Grall wrote:
> Would the below patch be fine?

Yes.

Patch

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);
+        return GNTST_bad_page;
     }
-#endif
+
+    *frame = page_to_mfn(*page);
 
     return rc;
 }