Re: [Xen-devel] [PATCH v2] altp2m: Allow the hostp2m to be shared

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 27/04/16 16:48, Tamas K Lengyel wrote:
> Don't propagate altp2m changes from ept_set_entry for memshare as memshare
> already has the lock. We call altp2m propagate changes once memshare
> successfully finishes. Allow the hostp2m entries to be of type
> p2m_ram_shared when applying mem_access. Also, do not trigger PoD for hostp2m
> when setting altp2m mem_access to be in-line with non-altp2m mem_access path.
> 
> Signed-off-by: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> ---
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> Cc: Keir Fraser <keir@xxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> 
> v2: Cosmetic fixes and addressing PoD in the commit message
> ---
>  xen/arch/x86/mm/mem_sharing.c | 10 +++++++++-
>  xen/arch/x86/mm/p2m-ept.c     |  2 +-
>  xen/arch/x86/mm/p2m.c         |  5 ++---
>  3 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index a522423..6693661 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -35,6 +35,7 @@
>  #include <asm/p2m.h>
>  #include <asm/atomic.h>
>  #include <asm/event.h>
> +#include <asm/altp2m.h>
>  #include <xsm/xsm.h>
>  
>  #include "mm-locks.h"
> @@ -926,11 +927,12 @@ int mem_sharing_share_pages(struct domain *sd, unsigned 
> long sgfn, shr_handle_t
>      int ret = -EINVAL;
>      mfn_t smfn, cmfn;
>      p2m_type_t smfn_type, cmfn_type;
> +    p2m_access_t cmfn_a;
>      struct two_gfns tg;
>      struct rmap_iterator ri;
>  
>      get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
> -                 cd, cgfn, &cmfn_type, NULL, &cmfn,
> +                 cd, cgfn, &cmfn_type, &cmfn_a, &cmfn,
>                   0, &tg);
>  
>      /* This tricky business is to avoid two callers deadlocking if 
> @@ -1026,6 +1028,12 @@ int mem_sharing_share_pages(struct domain *sd, 
> unsigned long sgfn, shr_handle_t
>      /* We managed to free a domain page. */
>      atomic_dec(&nr_shared_mfns);
>      atomic_inc(&nr_saved_mfns);
> +
> +    /* Save the change to the altp2m tables as well if active */
> +    if ( altp2m_active(cd) )
> +        p2m_altp2m_propagate_change(cd, _gfn(cgfn), smfn, PAGE_ORDER_4K,
> +                                    p2m_ram_shared, cmfn_a);
> +
>      ret = 0;
>      
>  err_out:
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index 1ed5b47..ff84242 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -847,7 +847,7 @@ out:
>      if ( is_epte_present(&old_entry) )
>          ept_free_entry(p2m, &old_entry, target);
>  
> -    if ( rc == 0 && p2m_is_hostp2m(p2m) )
> +    if ( rc == 0 && p2m_is_hostp2m(p2m) && p2mt != p2m_ram_shared )
>          p2m_altp2m_propagate_change(d, _gfn(gfn), mfn, order, p2mt, p2ma);

Sorry, just double-checking here: This is the right thing to do because
the only code that is *setting* a page p2m_ram_shared should be the
sharing code, which (after this patch) should be propagating the change
itself?

I'm just conscious that this introduces an invariant that would be easy
to accidentally break.

At the moment the only place that calls set_entry() with p2m_ram_shared
is in p2m.c:set_shared_p2m_entry(), which is called from
mem_sharing.c:__mem_sharing_unshare_page() and
mem_sharing.c:mem_sharing_share_pages().  You've handled the second case
by adding the if() clause at the end of the operation; and it looks like
the first case is  handled because the shared page will be overwritten
by the p2m_change_type_one().

I think at very least adding a comment here saying something like the
following would be helpful:

"Memsharing is responsible for updating altp2ms after the sharing
operation is complete"

And perhaps a note before the p2m_change_type_one() in
__mem_sharing_unshare_page() saying something like:

"This will take care of updating the altp2m mappings."

What do you think?

 -George


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