Re: [Xen-devel] [PATCH 5 of 9] x86/mm: When removing/adding a page from/to the physmap, keep in mind it could be shared

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

> At 14:51 -0500 on 01 Feb (1328107917), Andres Lagar-Cavilla wrote:
>>  xen/arch/x86/mm/p2m.c |  21 ++++++++++++++++++++-
>>  1 files changed, 20 insertions(+), 1 deletions(-)
>>
>>
>> When removing the m2p mapping it is unconditionally set to invalid,
>> which
>> breaks sharing.
>>
>> When adding to the physmap, if the previous holder of that entry is a
>> shared
>> page, we unshare to default to normal case handling.
>>
>> And, we cannot add a shared page directly to the physmap. Proper
>> interfaces
>> must be employed, otherwise book-keeping goes awry.
>>
>> Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
>>
>> diff -r 8a920bcddd0f -r 1c61573d1765 xen/arch/x86/mm/p2m.c
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -419,7 +419,7 @@ p2m_remove_page(struct p2m_domain *p2m,
>>          for ( i = 0; i < (1UL << page_order); i++ )
>>          {
>>              mfn_return = p2m->get_entry(p2m, gfn + i, &t, &a,
>> p2m_query, NULL);
>> -            if ( !p2m_is_grant(t) )
>> +            if ( !p2m_is_grant(t) && !p2m_is_shared(t) )
>>                  set_gpfn_from_mfn(mfn+i, INVALID_M2P_ENTRY);
>>              ASSERT( !p2m_is_valid(t) || mfn + i == mfn_x(mfn_return) );
>>          }
>> @@ -481,6 +481,17 @@ guest_physmap_add_entry(struct domain *d
>>      for ( i = 0; i < (1UL << page_order); i++ )
>>      {
>>          omfn = p2m->get_entry(p2m, gfn + i, &ot, &a, p2m_query, NULL);
>> +        if ( p2m_is_shared(ot) )
>> +        {
>> +            /* Do an unshare to cleanly take care of all corner
>> +             * cases. */
>> +            int rc;
>> +            rc = mem_sharing_unshare_page(p2m->domain, gfn + i, 0);
>> +            if ( rc )
>> +                return rc;
>
> You're holding the p2m lock here!  Also, I don't think you can call
> mem_sharing_unshare_page() with that held - wasn't that the reason for
> cset f6c33cfe7333 ?
D'oh!
This patch has to follow the locking p2m series. Before reposting, I'll
have to make sure nothing breaks when applying after locking p2m.

Good catch!
Andres
>
> Tim.
>
>



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