Re: [Xen-devel] [PATCH v5] altp2m: Allow shared entries to be copied to altp2m views during lazycopy

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On Wed, Jul 20, 2016 at 7:55 PM, Tamas K Lengyel
<tamas.lengyel@xxxxxxxxxxxx> wrote:
> Move sharing locks above altp2m to avoid locking order violation and crashing
> the hypervisor during unsharing operations when altp2m is active.
>
> Applying mem_access settings or remapping gfns in altp2m views will
> automatically unshare the page if it was shared previously. Also,
> disallow nominating pages for which there are pre-existing altp2m
> mem_access settings or remappings present. However, allow altp2m
> to populate altp2m views with shared entries during lazycopy as
> unsharing will automatically propagate the change to these entries in
> altp2m views as well.
>
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>

This looks a lot better.  One comment...

> ---
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>
> v5: Allow only lazycopy to copy shared entries to altp2m views
>     Use get_gfn_type_access for unsharing as get_entry doesn't do that
> ---
>  xen/arch/x86/mm/mem_sharing.c | 25 ++++++++++++++++++++++++-
>  xen/arch/x86/mm/mm-locks.h    | 30 +++++++++++++++---------------
>  xen/arch/x86/mm/p2m.c         | 12 +++++++-----
>  3 files changed, 46 insertions(+), 21 deletions(-)
>
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index a522423..3939cd0 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -33,6 +33,7 @@
>  #include <asm/page.h>
>  #include <asm/string.h>
>  #include <asm/p2m.h>
> +#include <asm/altp2m.h>
>  #include <asm/atomic.h>
>  #include <asm/event.h>
>  #include <xsm/xsm.h>
> @@ -828,20 +829,42 @@ int mem_sharing_nominate_page(struct domain *d,
>                                int expected_refcnt,
>                                shr_handle_t *phandle)
>  {
> +    struct p2m_domain *hp2m = p2m_get_hostp2m(d);
>      p2m_type_t p2mt;
> +    p2m_access_t p2ma;
>      mfn_t mfn;
>      struct page_info *page = NULL; /* gcc... */
>      int ret;
>
>      *phandle = 0UL;
>
> -    mfn = get_gfn(d, gfn, &p2mt);
> +    mfn = get_gfn_type_access(hp2m, gfn, &p2mt, &p2ma, 0, NULL);
>
>      /* Check if mfn is valid */
>      ret = -EINVAL;
>      if ( !mfn_valid(mfn) )
>          goto out;
>
> +    /* Check if there are mem_access/remapped altp2m entries for this page */
> +    if ( altp2m_active(d) )
> +    {
> +        unsigned int i;
> +        struct p2m_domain *ap2m;
> +        mfn_t amfn;
> +        p2m_access_t ap2ma;
> +
> +        for ( i = 0; i < MAX_ALTP2M; i++ )
> +        {
> +            ap2m = d->arch.altp2m_p2m[i];
> +            if ( !ap2m )
> +                continue;
> +
> +            amfn = get_gfn_type_access(ap2m, gfn, NULL, &ap2ma, 0, NULL);
> +            if ( mfn_valid(amfn) && (mfn_x(amfn) != mfn_x(mfn) || ap2ma != 
> p2ma) )
> +                goto out;
> +        }

You need to altp2m_list_[un]lock(d) around this loop, don't you?

Everything else looks great, thanks.

 -George

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