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

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxx>

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;
 }

-- 
Julien Grall

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