Xen devel - Feb 2012 - [PATCH] x86/mm: Make asserts on types and counts of shared pages more accurate

xen/arch/x86/mm/mem_sharing.c |  4 +++-
 xen/arch/x86/mm/p2m.c         |  6 ++++--
 2 files changed, 7 insertions(+), 3 deletions(-)


Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r 667191f054c3 -r f2efbfaa8d26 xen/arch/x86/mm/mem_sharing.c
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -200,7 +200,9 @@ static struct page_info* mem_sharing_loo
             /* Count has to be at least two, because we''re called
              * with the mfn locked (1) and this is supposed to be 
              * a shared page (1). */
-            ASSERT((page->u.inuse.type_info & PGT_count_mask) >= 2); 
+            ASSERT((page->u.inuse.type_info & 
+                        (PGT_shared_page | PGT_count_mask)) >= 
+                            (PGT_shared_page | 2)); 
             ASSERT(get_gpfn_from_mfn(mfn) == SHARED_M2P_ENTRY); 
             return page;
         }
diff -r 667191f054c3 -r f2efbfaa8d26 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -745,8 +745,10 @@ set_shared_p2m_entry(struct domain *d, u
      * sharable first */
     ASSERT(p2m_is_shared(ot));
     ASSERT(mfn_valid(omfn));
-    if ( ((mfn_to_page(omfn)->u.inuse.type_info & PGT_type_mask) 
-                    != PGT_shared_page) )
+    /* Set the m2p entry to invalid only if there are no further type
+     * refs to this page as shared */
+    if ( (mfn_to_page(omfn)->u.inuse.type_info & 
+            (PGT_shared_page | PGT_count_mask)) <= PGT_shared_page )
         set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
 
     P2M_DEBUG("set shared %lx %lx\n", gfn, mfn_x(mfn));

At 01:01 -0500 on 09 Feb (1328749297), Andres Lagar-Cavilla
wrote:
>  xen/arch/x86/mm/mem_sharing.c |  4 +++-
>  xen/arch/x86/mm/p2m.c         |  6 ++++--
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
NACK, I''m afraid, especially the second one.  ''<=''
comparisons with a
number that''s made up of a count ORed with a type don''t make
sense.
If you want to test both type and count, just test them separately.

Cheers,

Tim.
> diff -r 667191f054c3 -r f2efbfaa8d26 xen/arch/x86/mm/mem_sharing.c
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -200,7 +200,9 @@ static struct page_info* mem_sharing_loo
>              /* Count has to be at least two, because we''re called
>               * with the mfn locked (1) and this is supposed to be 
>               * a shared page (1). */
> -            ASSERT((page->u.inuse.type_info & PGT_count_mask) >=
2);
> +            ASSERT((page->u.inuse.type_info & 
> +                        (PGT_shared_page | PGT_count_mask)) >= 
> +                            (PGT_shared_page | 2)); 
>              ASSERT(get_gpfn_from_mfn(mfn) == SHARED_M2P_ENTRY); 
>              return page;
>          }
> diff -r 667191f054c3 -r f2efbfaa8d26 xen/arch/x86/mm/p2m.c
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -745,8 +745,10 @@ set_shared_p2m_entry(struct domain *d, u
>       * sharable first */
>      ASSERT(p2m_is_shared(ot));
>      ASSERT(mfn_valid(omfn));
> -    if ( ((mfn_to_page(omfn)->u.inuse.type_info & PGT_type_mask) 
> -                    != PGT_shared_page) )
> +    /* Set the m2p entry to invalid only if there are no further type
> +     * refs to this page as shared */
> +    if ( (mfn_to_page(omfn)->u.inuse.type_info & 
> +            (PGT_shared_page | PGT_count_mask)) <= PGT_shared_page )
>          set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
>  
>      P2M_DEBUG("set shared %lx %lx\n", gfn, mfn_x(mfn));
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

> At 01:01 -0500 on 09 Feb (1328749297), Andres Lagar-Cavilla wrote:
>>  xen/arch/x86/mm/mem_sharing.c |  4 +++-
>>  xen/arch/x86/mm/p2m.c         |  6 ++++--
>>  2 files changed, 7 insertions(+), 3 deletions(-)
>>
>>
>> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>
> NACK, I''m afraid, especially the second one. 
''<='' comparisons with a
> number that''s made up of a count ORed with a type don''t
make sense.
> If you want to test both type and count, just test them separately.
The type I''m ORing with is a mask with single bit set, not a multi-bit
type mask. And the type is defined as a higher order bit than the count
mask. So, nothing that I''m ORing with could get in the way of the
numeric
comparison.

Having said that I understand it''s not terribly elegant or easy to
understand.

Andres
>
> Cheers,
>
> Tim.
>
>> diff -r 667191f054c3 -r f2efbfaa8d26 xen/arch/x86/mm/mem_sharing.c
>> --- a/xen/arch/x86/mm/mem_sharing.c
>> +++ b/xen/arch/x86/mm/mem_sharing.c
>> @@ -200,7 +200,9 @@ static struct page_info* mem_sharing_loo
>>              /* Count has to be at least two, because we''re
called
>>               * with the mfn locked (1) and this is supposed to be
>>               * a shared page (1). */
>> -            ASSERT((page->u.inuse.type_info & PGT_count_mask)
>= 2);
>> +            ASSERT((page->u.inuse.type_info &
>> +                        (PGT_shared_page | PGT_count_mask))
>>> +                            (PGT_shared_page | 2));
>>              ASSERT(get_gpfn_from_mfn(mfn) == SHARED_M2P_ENTRY);
>>              return page;
>>          }
>> diff -r 667191f054c3 -r f2efbfaa8d26 xen/arch/x86/mm/p2m.c
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -745,8 +745,10 @@ set_shared_p2m_entry(struct domain *d, u
>>       * sharable first */
>>      ASSERT(p2m_is_shared(ot));
>>      ASSERT(mfn_valid(omfn));
>> -    if ( ((mfn_to_page(omfn)->u.inuse.type_info &
PGT_type_mask)
>> -                    != PGT_shared_page) )
>> +    /* Set the m2p entry to invalid only if there are no further type
>> +     * refs to this page as shared */
>> +    if ( (mfn_to_page(omfn)->u.inuse.type_info &
>> +            (PGT_shared_page | PGT_count_mask)) <= PGT_shared_page
)
>>          set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
>>
>>      P2M_DEBUG("set shared %lx %lx\n", gfn, mfn_x(mfn));
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>

At 06:50 -0800 on 09 Feb (1328770222), Andres Lagar-Cavilla
wrote:
> > At 01:01 -0500 on 09 Feb (1328749297), Andres Lagar-Cavilla wrote:
> >>  xen/arch/x86/mm/mem_sharing.c |  4 +++-
> >>  xen/arch/x86/mm/p2m.c         |  6 ++++--
> >>  2 files changed, 7 insertions(+), 3 deletions(-)
> >>
> >>
> >> Signed-off-by: Andres Lagar-Cavilla
<andres@lagarcavilla.org>
> >
> > NACK, I''m afraid, especially the second one. 
''<='' comparisons with a
> > number that''s made up of a count ORed with a type
don''t make sense.
> > If you want to test both type and count, just test them separately.
> 
> The type I''m ORing with is a mask with single bit set, not a
multi-bit
> type mask. And the type is defined as a higher order bit than the count
> mask.
It is right now, but if someone reshuffles the page_info struct again
you don''t want it to banjax your super-cunning code.  Just test the
thing you need to test. :)

Tim.