Re: [Xen-devel] [PATCH] x86/mem_sharing: Rectify test for "empty" physmap entry in sharing_add_to_physmap

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

> At 09:06 -0400 on 18 Apr (1334740000), Andres Lagar-Cavilla wrote:
>>  xen/arch/x86/mm/mem_sharing.c |  6 ++++--
>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>
>>
>> Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
>>
>> diff -r 8609bbbba141 -r 495d3df95c1d 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,11 @@ 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 */
>> +    /* Make sure the target page is a hole in the physmap. This is not
>> as
>> +     * simple a test as we'd like it to be. Non-existent p2m entries
>> return
>> +     * invalid mfn and p2m_mmio_dm type when queried. */
>>      if ( mfn_valid(cmfn) ||
>> -         (!(p2m_is_ram(cmfn_type))) )
>> +         ( (!(p2m_is_ram(cmfn_type))) && (cmfn_type != p2m_mmio_dm) ) )
>
> This test looks wrong, even after the fix.  I think it should be testing
> for cmfn_type == p2m_mmio_dm || cmfn_type == p2m_invalid and ignoring
> mfn_valid().

Maybe :)

I'm coming up with the semantics for this one, loosely based on  the
regular populate physmap code path. That one can end up replacing existing
regular pages, or paged out entries, or PoD entries, with the new
populated pages (in guest_physmap_add_entry). I don't know that all of the
above is reasonable.

But certainly I would like to keep the ability to replace paged out
entries with (identical) pages that are already present in other domains.

Andres
>
> Cheers,
>
> Tim.
>



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel