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

["Andres Lagar-Cavilla" <andres@xxxxxxxxxxxxxxxx>]

> 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@xxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxx
>> http://lists.xensource.com/xen-devel
>



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel