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

[Tim Deegan <tim@xen.org>]

At 07:18 -0700 on 18 Apr (1334733529), Andres Lagar-Cavilla wrote:
> > 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@lagarcavilla.org>
> >>
> >> 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.

Fair enough.  Maybe you could define a new p2m-type-mask of all the
things you think it's OK to replace in this kind of situation, and apply
it in all cases?

Tim.