thr3ads.net - Xen devel - [PATCH] x86/mem_sharing: Rectify test for "empty" physmap entry in sharing_add_to

If this information is useful, please help other people find it:
Share via:

Andres Lagar-Cavilla

2012-Apr-18 13:06 UTC

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

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;

Tim Deegan

2012-Apr-18 13:59 UTC

head link

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

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.

Andres Lagar-Cavilla

2012-Apr-18 14:18 UTC

head link

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

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

Tim Deegan

2012-Apr-18 15:01 UTC

head link

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

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.

Andres Lagar-Cavilla

2012-Apr-18 15:13 UTC

head link

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

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

Tim Deegan

2012-Apr-18 15:17 UTC

head link

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

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.

Andres Lagar-Cavilla

2012-Apr-18 15:55 UTC

head link

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

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

Andres Lagar-Cavilla

2012-May-16 14:05 UTC

head link

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

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)

Tim Deegan

2012-May-17 09:40 UTC

head link

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

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

Andres Lagar-Cavilla

2012-May-17 11:36 UTC

head link

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

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

Tim Deegan

2012-May-17 12:02 UTC

head link

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

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

Andres Lagar-Cavilla

2012-May-17 15:55 UTC

head link

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

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

Tim Deegan

2012-May-18 15:22 UTC

head link

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

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.

Andres Lagar-Cavilla

2012-May-18 15:25 UTC

head link

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

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.

Andres Lagar-Cavilla

2012-May-23 14:34 UTC

head link

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

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

Tim Deegan

2012-May-23 16:17 UTC

head link

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

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.

Xen devel - Apr 2012 - [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

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

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

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

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

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

Re: [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

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

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

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

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

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

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

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

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