Re: [Xen-devel] [PATCH v3 10/13] x86/altp2m: add remaining support routines.

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Wed, Jul 1, 2015 at 7:09 PM, Ed White <edmund.h.white@xxxxxxxxx> wrote:
> Add the remaining routines required to support enabling the alternate
> p2m functionality.
>
> Signed-off-by: Ed White <edmund.h.white@xxxxxxxxx>
> ---
>  xen/arch/x86/hvm/hvm.c           |  58 +++++-
>  xen/arch/x86/mm/hap/Makefile     |   1 +
>  xen/arch/x86/mm/hap/altp2m_hap.c |  98 ++++++++++
>  xen/arch/x86/mm/p2m-ept.c        |   3 +
>  xen/arch/x86/mm/p2m.c            | 385 
> +++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/hvm/altp2m.h |   4 +
>  xen/include/asm-x86/p2m.h        |  33 ++++
>  7 files changed, 576 insertions(+), 6 deletions(-)
>  create mode 100644 xen/arch/x86/mm/hap/altp2m_hap.c
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index f21d34d..d2d90c8 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2806,10 +2806,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>      mfn_t mfn;
>      struct vcpu *curr = current;
>      struct domain *currd = curr->domain;
> -    struct p2m_domain *p2m;
> +    struct p2m_domain *p2m, *hostp2m;
>      int rc, fall_through = 0, paged = 0;
>      int sharing_enomem = 0;
>      vm_event_request_t *req_ptr = NULL;
> +    bool_t ap2m_active = 0;
>
>      /* On Nested Virtualization, walk the guest page table.
>       * If this succeeds, all is fine.
> @@ -2869,11 +2870,31 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>          goto out;
>      }
>
> -    p2m = p2m_get_hostp2m(currd);
> -    mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma,
> +    ap2m_active = altp2m_active(currd);
> +
> +    /* Take a lock on the host p2m speculatively, to avoid potential
> +     * locking order problems later and to handle unshare etc.
> +     */
> +    hostp2m = p2m_get_hostp2m(currd);
> +    mfn = get_gfn_type_access(hostp2m, gfn, &p2mt, &p2ma,
>                                P2M_ALLOC | (npfec.write_access ? P2M_UNSHARE 
> : 0),
>                                NULL);
>
> +    if ( ap2m_active )
> +    {
> +        if ( altp2m_hap_nested_page_fault(curr, gpa, gla, npfec, &p2m) == 1 )
> +        {
> +            /* entry was lazily copied from host -- retry */
> +            __put_gfn(hostp2m, gfn);
> +            rc = 1;
> +            goto out;
> +        }
> +
> +        mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma, 0, NULL);
> +    }
> +    else
> +        p2m = hostp2m;
> +
>      /* Check access permissions first, then handle faults */
>      if ( mfn_x(mfn) != INVALID_MFN )
>      {
> @@ -2913,6 +2934,20 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>
>          if ( violation )
>          {
> +            /* Should #VE be emulated for this fault? */
> +            if ( p2m_is_altp2m(p2m) && !cpu_has_vmx_virt_exceptions )
> +            {
> +                bool_t sve;
> +
> +                p2m->get_entry_full(p2m, gfn, &p2mt, &p2ma, 0, NULL, &sve);
> +
> +                if ( !sve && ap2m_vcpu_emulate_ve(curr) )
> +                {
> +                    rc = 1;
> +                    goto out_put_gfn;
> +                }
> +            }
> +
>              if ( p2m_mem_access_check(gpa, gla, npfec, &req_ptr) )
>              {
>                  fall_through = 1;
> @@ -2932,7 +2967,9 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>           (npfec.write_access &&
>            (p2m_is_discard_write(p2mt) || (p2mt == p2m_mmio_write_dm))) )
>      {
> -        put_gfn(currd, gfn);
> +        __put_gfn(p2m, gfn);
> +        if ( ap2m_active )
> +            __put_gfn(hostp2m, gfn);
>
>          rc = 0;
>          if ( unlikely(is_pvh_domain(currd)) )
> @@ -2961,6 +2998,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>      /* Spurious fault? PoD and log-dirty also take this path. */
>      if ( p2m_is_ram(p2mt) )
>      {
> +        rc = 1;
>          /*
>           * Page log dirty is always done with order 0. If this mfn resides in
>           * a large page, we do not change other pages type within that large
> @@ -2969,9 +3007,15 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>          if ( npfec.write_access )
>          {
>              paging_mark_dirty(currd, mfn_x(mfn));
> +            /* If p2m is really an altp2m, unlock here to avoid lock ordering
> +             * violation when the change below is propagated from host p2m */
> +            if ( ap2m_active )
> +                __put_gfn(p2m, gfn);
>              p2m_change_type_one(currd, gfn, p2m_ram_logdirty, p2m_ram_rw);
> +            __put_gfn(ap2m_active ? hostp2m : p2m, gfn);
> +
> +            goto out;
>          }
> -        rc = 1;
>          goto out_put_gfn;
>      }
>
> @@ -2981,7 +3025,9 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>      rc = fall_through;
>
>  out_put_gfn:
> -    put_gfn(currd, gfn);
> +    __put_gfn(p2m, gfn);
> +    if ( ap2m_active )
> +        __put_gfn(hostp2m, gfn);
>  out:
>      /* All of these are delayed until we exit, since we might
>       * sleep on event ring wait queues, and we must not hold
> diff --git a/xen/arch/x86/mm/hap/Makefile b/xen/arch/x86/mm/hap/Makefile
> index 68f2bb5..216cd90 100644
> --- a/xen/arch/x86/mm/hap/Makefile
> +++ b/xen/arch/x86/mm/hap/Makefile
> @@ -4,6 +4,7 @@ obj-y += guest_walk_3level.o
>  obj-$(x86_64) += guest_walk_4level.o
>  obj-y += nested_hap.o
>  obj-y += nested_ept.o
> +obj-y += altp2m_hap.o
>
>  guest_walk_%level.o: guest_walk.c Makefile
>         $(CC) $(CFLAGS) -DGUEST_PAGING_LEVELS=$* -c $< -o $@
> diff --git a/xen/arch/x86/mm/hap/altp2m_hap.c 
> b/xen/arch/x86/mm/hap/altp2m_hap.c
> new file mode 100644
> index 0000000..52c7877
> --- /dev/null
> +++ b/xen/arch/x86/mm/hap/altp2m_hap.c
> @@ -0,0 +1,98 @@
> +/******************************************************************************
> + * arch/x86/mm/hap/altp2m_hap.c
> + *
> + * Copyright (c) 2014 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#include <asm/domain.h>
> +#include <asm/page.h>
> +#include <asm/paging.h>
> +#include <asm/p2m.h>
> +#include <asm/hap.h>
> +#include <asm/hvm/altp2m.h>
> +
> +#include "private.h"
> +
> +/*
> + * If the fault is for a not present entry:
> + *     if the entry in the host p2m has a valid mfn, copy it and retry
> + *     else indicate that outer handler should handle fault
> + *
> + * If the fault is for a present entry:
> + *     indicate that outer handler should handle fault
> + */
> +
> +int
> +altp2m_hap_nested_page_fault(struct vcpu *v, paddr_t gpa,
> +                                unsigned long gla, struct npfec npfec,
> +                                struct p2m_domain **ap2m)

It looks like even at the end of this series, all that this function
ever does is propagate changes from the host p2m to he guest p2m.

In that case, wouldn't it be better to rename this function something
like "altp2m_propagate_from_hostp2m()" or something like that, so that
people reading the nested page fault handler know there's nothing else
going on?

Also, it appears that this is the only function in this file, even at
the end of the series.  Would it make more sense to include this
function in another file?


> +{
> +    struct p2m_domain *hp2m = p2m_get_hostp2m(v->domain);
> +    p2m_type_t p2mt;
> +    p2m_access_t p2ma;
> +    unsigned int page_order;
> +    gfn_t gfn = _gfn(paddr_to_pfn(gpa));
> +    unsigned long mask;
> +    mfn_t mfn;
> +    int rv;
> +
> +    *ap2m = p2m_get_altp2m(v);
> +
> +    mfn = get_gfn_type_access(*ap2m, gfn_x(gfn), &p2mt, &p2ma,
> +                              0, &page_order);
> +    __put_gfn(*ap2m, gfn_x(gfn));
> +
> +    if ( mfn_x(mfn) != INVALID_MFN )
> +        return 0;
> +
> +    mfn = get_gfn_type_access(hp2m, gfn_x(gfn), &p2mt, &p2ma,
> +                              P2M_ALLOC | P2M_UNSHARE, &page_order);
> +    put_gfn(hp2m->domain, gfn_x(gfn));

Why are we calling put_gfn here instead of __put_gfn (as above)?

--

These aren't blockers, I don't think, but it would be nice to have
them addressed if possible.

Also, just some feedback re the patch organization here for future
refernce -- introducing all these "support routines" which are not yet
called anywhere -- makes it difficult to review.  If I don't know
where they're going to be called from, how can I know if they're doing
what they're supposed to do?  It seems like introducing them as needed
would make review easier, as then the reviewer can go through the
whole path in which they're being used and decide (for instance) if
the boundaries of the functions make sense.

Everything that is functional as of this patch looks good from a
functional perspective; just the naming / file comments above.  I may
have comments on the non-functional bits when I find out how they're
used later.

 -George

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