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

[Tim Deegan <tim@xen.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.

Cheers,

Tim.