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

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

> 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.

Ok great. This is what I have now. But I haven't been able to test it, so
RFC for the moment.
Andres

# HG changeset patch
# User Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
# Date 1334762521 14400
# Node ID d91665827cbc9e4c7e649a664b93966e8bb00f09
# Parent  9c241969ea6db6590de1f7079309d7cd7b4d6435
x86/mem_sharing: Rectify test for "empty" physmap entry in
sharing_add_to_physmap.

Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>

diff -r 9c241969ea6d -r d91665827cbc 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. 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 ( !p2m_is_hole(cmfn_type) )
     {
         ret = XENMEM_SHARING_OP_C_HANDLE_INVALID;
         goto err_unlock;
diff -r 9c241969ea6d -r d91665827cbc xen/include/asm-x86/p2m.h
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -133,6 +133,12 @@ typedef unsigned int p2m_query_t;
                        | p2m_to_mask(p2m_ram_paging_in)       \
                        | p2m_to_mask(p2m_ram_shared))

+/* Types that represent a physmap hole that is ok to replace with a shared
+ * entry */
+#define P2M_HOLE_TYPES (p2m_to_mask(p2m_mmio_dm)     \
+                       | p2m_to_mask(p2m_invalid)   \
+                       | p2m_to_mask(p2m_ram_paged))
+
 /* Grant mapping types, which map to a real machine frame in another
  * VM */
 #define P2M_GRANT_TYPES (p2m_to_mask(p2m_grant_map_rw)  \
@@ -173,6 +179,7 @@ typedef unsigned int p2m_query_t;

 /* Useful predicates */
 #define p2m_is_ram(_t) (p2m_to_mask(_t) & P2M_RAM_TYPES)
+#define p2m_is_hole(_t) (p2m_to_mask(_t) & P2M_HOLE_TYPES)
 #define p2m_is_mmio(_t) (p2m_to_mask(_t) & P2M_MMIO_TYPES)
 #define p2m_is_readonly(_t) (p2m_to_mask(_t) & P2M_RO_TYPES)
 #define p2m_is_magic(_t) (p2m_to_mask(_t) & P2M_MAGIC_TYPES)


>
> Tim.
>



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