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

[Tim Deegan <tim@xxxxxxx>]

At 08:13 -0700 on 18 Apr (1334736812), Andres Lagar-Cavilla wrote:
> > 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@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.
> >
> > 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?
> 
> Is there a chance for a p2m_mmio_dm entry to hold a valid mfn?

At the moment the gfn is all that's needed to emulate the MFN but in
future we might encode, say, which of several servers to send the ioreq
to, and that might be a number that passes mfn_valid().  But it wouldn't
actually be an MFN, IYSWIM.

Tim.

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