Re: [PATCH 2 of 2] x86/mem_sharing: Rectify test for "empty" physmap entry in sharing_add_to_physmap

["Andres Lagar-Cavilla" <andres@lagarcavilla.org>]

> At 15:20 -0400 on 24 Apr (1335280819), Andres Lagar-Cavilla wrote:
>>  xen/arch/x86/mm/mem_sharing.c |  7 ++++---
>>  xen/include/asm-x86/p2m.h     |  8 ++++++++
>>  2 files changed, 12 insertions(+), 3 deletions(-)
>>
>>
>> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>>
>> diff -r 5be9a05f17fd -r 51646b89b182 xen/arch/x86/mm/mem_sharing.c
>> --- a/xen/arch/x86/mm/mem_sharing.c
>> +++ b/xen/arch/x86/mm/mem_sharing.c
>> @@ -1073,9 +1073,10 @@ int mem_sharing_add_to_physmap(struct do
>>      if ( spage->sharing->handle != sh )
>>          goto err_unlock;
>>
>> -    /* Make sure the target page is a hole in the physmap */
>> -    if ( mfn_valid(cmfn) ||
>> -         (!(p2m_is_ram(cmfn_type))) )
>> +    /* Make sure the target page is a hole in the physmap. These are
>> typically
>> +     * p2m_mmio_dm, but also accept p2m_invalid and paged out pages.
>> See the
>> +     * definition of p2m_is_hole in p2m.h. */
>> +    if ( !p2m_is_hole(cmfn_type) || mfn_valid(cmfn) )
>
> Hmm.  Is the mfn_valid() to handle p2m_ram_paging_in entries sometimes
> having an MFN and sometimes not?  I think it would be nicer to either
> always replace paging-in pages or never do it.  In any case, it's bogus
> to test mfn_valid() for any of the other types.

Yes, that is the concern. paging_in entries may have a backing mfn. I am
not opposed to just replacing the entry if it is in paging_in state,
regardless of the mfn. It would be similar to any of the cases in which
guest_physmap_add_entry is used with a valid mfn in the entry to be
updated.

Andres

>
> Cheers,
>
> Tim.
>