[PATCH] x86/mm: Make asserts on types and counts of shared pages more accurate

[PATCH] x86/mm: Make asserts on types and counts of shared pages more accurate

[Andres Lagar-Cavilla]

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

Re: [PATCH] x86/mm: Make asserts on types and counts of shared pages more accurate

[Tim Deegan]

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

Re: [PATCH] x86/mm: Make asserts on types and counts of shared pages more accurate

[Andres Lagar-Cavilla]

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

Re: [PATCH] x86/mm: Make asserts on types and counts of shared pages more accurate

[Tim Deegan]

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.