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

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

[Andres Lagar-Cavilla]

 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 f83397dfb6c0 -r 3169fba29613 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) )
     {
         ret = XENMEM_SHARING_OP_C_HANDLE_INVALID;
         goto err_unlock;
diff -r f83397dfb6c0 -r 3169fba29613 xen/include/asm-x86/p2m.h
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -133,6 +133,13 @@ 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_paging_in) \
+                       | 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 +180,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)

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

[Tim Deegan]

> diff -r f83397dfb6c0 -r 3169fba29613 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) )

As I said last time, the mfn_valid test is wrong for everything except
p2m_paging_in.

I've checked in a reduced version of this that just drops p2m_paging_in
from P2M_HOLE_TYPES (so the pager wins this race instead of the sharer)
which I think will do for 4.2:
http://xenbits.xen.org/hg/staging/xen-unstable.hg/rev/25bdef4493ae
can you check that it makes sense, please?

Cheers,

Tim.

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

[Andres Lagar-Cavilla]

>
>> diff -r f83397dfb6c0 -r 3169fba29613 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) )
>
> As I said last time, the mfn_valid test is wrong for everything except
> p2m_paging_in.

I am sorry. I have been testing all along -- and therefore sent -- the
wrong patch. Can we please revert this? My sloppiness.

>
> I've checked in a reduced version of this that just drops p2m_paging_in
> from P2M_HOLE_TYPES (so the pager wins this race instead of the sharer)
> which I think will do for 4.2:
> http://xenbits.xen.org/hg/staging/xen-unstable.hg/rev/25bdef4493ae
> can you check that it makes sense, please?

Unfortunately not. The way it's been checked in, it nullifies the ability
to plug in a paging fault with a shared page -- which was healthily
working before.

I believe the fix we'll converge to is keep paging_in in the "hole" set of
types, and remove the check for valid mfn. Something along the lines of
what I've pasted below. But no more sloppinees, I'll do some testing and
hopefully submit the right thing!

Thanks
Andres

# HG changeset patch
# Parent 9fc0252536f0a4ddf48b7ec9cd7a7243271545f8
x86/mem_sharing: Rectify test for "empty" physmap entry in
sharing_add_to_physmap.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r 9fc0252536f0 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) )
     {
         ret = XENMEM_SHARING_OP_C_HANDLE_INVALID;
         goto err_unlock;
diff -r 9fc0252536f0 xen/include/asm-x86/p2m.h
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -133,6 +133,13 @@ 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_paging_in) \
+                       | 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 +180,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)

>
> Cheers,
>
> Tim.
>

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

[Tim Deegan]

At 04:36 -0700 on 17 May (1337229382), Andres Lagar-Cavilla wrote:
> I believe the fix we'll converge to is keep paging_in in the "hole" set of
> types, and remove the check for valid mfn. Something along the lines of
> what I've pasted below.

OK.  Please send it as a diff against what's already applied - I think
what we have now is more correct (if less useful) than what we had
before.

Do you have to worry about freeing the page as well?  Will it otherwise
be leaked into a state where it's allocated but not in the p2m?  I see
that guest_physmap_add_entry() doesn't free paging_in pages but maybe
that's wrong too?

Cheers,

Tim.

> # HG changeset patch
> # Parent 9fc0252536f0a4ddf48b7ec9cd7a7243271545f8
> x86/mem_sharing: Rectify test for "empty" physmap entry in
> sharing_add_to_physmap.
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> 
> diff -r 9fc0252536f0 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) )
>      {
>          ret = XENMEM_SHARING_OP_C_HANDLE_INVALID;
>          goto err_unlock;
> diff -r 9fc0252536f0 xen/include/asm-x86/p2m.h
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -133,6 +133,13 @@ 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_paging_in) \
> +                       | 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 +180,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)
> 
> >
> > Cheers,
> >
> > Tim.
> >
> 
> 

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

[Andres Lagar-Cavilla]

> At 04:36 -0700 on 17 May (1337229382), Andres Lagar-Cavilla wrote:
>> I believe the fix we'll converge to is keep paging_in in the "hole" set
>> of
>> types, and remove the check for valid mfn. Something along the lines of
>> what I've pasted below.
>
> OK.  Please send it as a diff against what's already applied - I think
> what we have now is more correct (if less useful) than what we had
> before.

Done. Tested and pasted below. Hopefully you can apply it before
end-of-day. Works for all known users of sharing+paging ;)

>
> Do you have to worry about freeing the page as well?  Will it otherwise
> be leaked into a state where it's allocated but not in the p2m?  I see
> that guest_physmap_add_entry() doesn't free paging_in pages but maybe
> that's wrong too?

Exactly. Leaked out of the p2m. Will still be cleaned up properly on
domain destruction.

The patch below takes care of the leak for sharing_add_to_physmap. However
I am not touching guest_physmap_add_entry -- it's been that way for pretty
much forever. My observation is that guest_physmap_add_entry is mostly
called from hypercalls (grant, XENMEM), so the domain is leaking its own
memory and risking running against the max_pages limit sooner, if being
sloppy. The only exception is populate physmap, which maybe should be
looked into.

Thanks,
Andres

# HG changeset patch
# Parent 485cd11f131da88b286b3b64e8f095508b67ab0b
x86/mem_sharing: Re-rectify sharing add to physmap hole test.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r 485cd11f131d xen/arch/x86/mm/mem_sharing.c
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1103,7 +1103,17 @@ int mem_sharing_add_to_physmap(struct do
         ret = 0;
         /* There is a chance we're plugging a hole where a paged out page
was */
         if ( p2m_is_paging(cmfn_type) && (cmfn_type != p2m_ram_paging_out) )
+        {
             atomic_dec(&cd->paged_pages);
+            /* Further, there is a chance this was a valid page. Don't
leak it. */
+            if ( mfn_valid(cmfn) )
+            {
+                struct page_info *cpage = mfn_to_page(cmfn);
+                ASSERT(cpage != NULL);
+                if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
+                    put_page(cpage);
+            }
+        }
     }

     atomic_inc(&nr_saved_mfns);
diff -r 485cd11f131d xen/include/asm-x86/p2m.h
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -137,6 +137,7 @@ typedef unsigned int p2m_query_t;
  * entry */
 #define P2M_HOLE_TYPES (p2m_to_mask(p2m_mmio_dm)        \
                        | p2m_to_mask(p2m_invalid)       \
+                       | p2m_to_mask(p2m_ram_paging_in) \
                        | p2m_to_mask(p2m_ram_paged))

 /* Grant mapping types, which map to a real machine frame in another

guest_p
>
> Cheers,
>
> Tim.
>
>> # HG changeset patch
>> # Parent 9fc0252536f0a4ddf48b7ec9cd7a7243271545f8
>> x86/mem_sharing: Rectify test for "empty" physmap entry in
>> sharing_add_to_physmap.
>>
>> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>>
>> diff -r 9fc0252536f0 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) )
>>      {
>>          ret = XENMEM_SHARING_OP_C_HANDLE_INVALID;
>>          goto err_unlock;
>> diff -r 9fc0252536f0 xen/include/asm-x86/p2m.h
>> --- a/xen/include/asm-x86/p2m.h
>> +++ b/xen/include/asm-x86/p2m.h
>> @@ -133,6 +133,13 @@ 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_paging_in) \
>> +                       | 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 +180,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)
>>
>> >
>> > Cheers,
>> >
>> > Tim.
>> >
>>
>>
>

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

[Tim Deegan]

At 08:55 -0700 on 17 May (1337244911), Andres Lagar-Cavilla wrote:
> # HG changeset patch
> # Parent 485cd11f131da88b286b3b64e8f095508b67ab0b
> x86/mem_sharing: Re-rectify sharing add to physmap hole test.
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

Applied, thanks.  I gave it a slightly fuller description and fixed some
whitespace around parentheses on the way past. 

I think that empties the queue, at least of things that are wanted for 4.2.

Cheers,

Tim.

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

[Andres Lagar-Cavilla]

Thanks. Yeah, queue empty. Waiting for ghouls awakened by rc testing. That almost never happens of course :)
Andres
On May 18, 2012, at 11:22 AM, Tim Deegan wrote:

> At 08:55 -0700 on 17 May (1337244911), Andres Lagar-Cavilla wrote:
>> # HG changeset patch
>> # Parent 485cd11f131da88b286b3b64e8f095508b67ab0b
>> x86/mem_sharing: Re-rectify sharing add to physmap hole test.
>> 
>> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> 
> Applied, thanks.  I gave it a slightly fuller description and fixed some
> whitespace around parentheses on the way past. 
> 
> I think that empties the queue, at least of things that are wanted for 4.2.
> 
> Cheers,
> 
> Tim.

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

[Andres Lagar-Cavilla]

> At 08:55 -0700 on 17 May (1337244911), Andres Lagar-Cavilla wrote:
>> # HG changeset patch
>> # Parent 485cd11f131da88b286b3b64e8f095508b67ab0b
>> x86/mem_sharing: Re-rectify sharing add to physmap hole test.
>>
>> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>
> Applied, thanks.  I gave it a slightly fuller description and fixed some
> whitespace around parentheses on the way past.
>
> I think that empties the queue, at least of things that are wanted for
> 4.2.

Actually, patch 6 of this series posted a while ago
http://lists.xen.org/archives/html/xen-devel/2012-04/msg01364.html
http://lists.xen.org/archives/html/xen-devel/2012-04/msg00962.html

turns on locking p2m queries for shadow mode and seemingly you've acked
it. This is good to go afaict and would be a shame to be left off of 4.2.

Andres
>
> Cheers,
>
> Tim.
>

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

[Tim Deegan]

At 07:34 -0700 on 23 May (1337758450), Andres Lagar-Cavilla wrote:
> > At 08:55 -0700 on 17 May (1337244911), Andres Lagar-Cavilla wrote:
> >> # HG changeset patch
> >> # Parent 485cd11f131da88b286b3b64e8f095508b67ab0b
> >> x86/mem_sharing: Re-rectify sharing add to physmap hole test.
> >>
> >> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> >
> > Applied, thanks.  I gave it a slightly fuller description and fixed some
> > whitespace around parentheses on the way past.
> >
> > I think that empties the queue, at least of things that are wanted for
> > 4.2.
> 
> Actually, patch 6 of this series posted a while ago
> http://lists.xen.org/archives/html/xen-devel/2012-04/msg01364.html
> http://lists.xen.org/archives/html/xen-devel/2012-04/msg00962.html
> 
> turns on locking p2m queries for shadow mode and seemingly you've acked
> it. This is good to go afaict and would be a shame to be left off of 4.2.

I think I'll leave it out for now.  Shadow doesn't coexist with
paging/sharing/memattr and already has its own interlocks against
everything else so it's not a bugfix.

Cheers,

Tim.

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

[Andres Lagar-Cavilla]

 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) ) )
     {
         ret = XENMEM_SHARING_OP_C_HANDLE_INVALID;
         goto err_unlock;

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

[Tim Deegan]

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

Cheers,

Tim.

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

[Andres Lagar-Cavilla]

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

Andres
>
> Cheers,
>
> Tim.
>

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

[Tim Deegan]

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.

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

[Andres Lagar-Cavilla]

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

Is there a chance for a p2m_mmio_dm entry to hold a valid mfn?
Thanks,
Andres

>
> Tim.
>

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

[Tim Deegan]

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

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

[Andres Lagar-Cavilla]

> 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@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?
>>
>> 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@lagarcavilla.org>
# 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@lagarcavilla.org>

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