[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v4 3/4] x86/mm: handle foreign mappings in p2m_entry_modify



> From: Roger Pau Monne [mailto:roger.pau@xxxxxxxxxx]
> Sent: Tuesday, February 19, 2019 1:27 AM
> 
> So that the specific handling can be removed from
> atomic_write_ept_entry and be shared with npt and shadow code.
> 
> This commit also removes the check that prevent non-ept PVH dom0 from
> mapping foreign pages.

ah, so please ignore my comment to [1/4]. p2m_entry_modify
is used more than type change now. :-)

> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> ---
> Changes since v3:
>  - Replace the mfn_valid BUG_ONs with an assert & return.
> 
> Changes since v2:
>  - Return an error code from p2m_entry_modify and propagate it to the
>    callers.
> 
> Changes since v1:
>  - Simply code since mfn_to_page cannot return NULL.
>  - Check if the mfn is valid before getting/dropping the page reference.
>  - Use BUG_ON instead of ASSERTs, since getting the reference counting
>    wrong is more dangerous than a DoS.
> ---
>  xen/arch/x86/mm/hap/hap.c       | 12 +++++--
>  xen/arch/x86/mm/p2m-ept.c       | 56 +++------------------------------
>  xen/arch/x86/mm/p2m-pt.c        |  7 -----
>  xen/arch/x86/mm/shadow/common.c |  3 +-
>  xen/include/asm-x86/p2m.h       | 34 +++++++++++++++++---
>  5 files changed, 47 insertions(+), 65 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index 5b507376bc..2daf8424f6 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -714,6 +714,7 @@ hap_write_p2m_entry(struct domain *d, unsigned
> long gfn, l1_pgentry_t *p,
>  {
>      uint32_t old_flags;
>      bool_t flush_nestedp2m = 0;
> +    int rc;
> 
>      /* We know always use the host p2m here, regardless if the vcpu
>       * is in host or guest mode. The vcpu can be in guest mode by
> @@ -734,8 +735,15 @@ hap_write_p2m_entry(struct domain *d, unsigned
> long gfn, l1_pgentry_t *p,
>              && perms_strictly_increased(old_flags, l1e_get_flags(new)) );
>      }
> 
> -    p2m_entry_modify(p2m_get_hostp2m(d),
> p2m_flags_to_type(l1e_get_flags(new)),
> -                     p2m_flags_to_type(old_flags), level);
> +    rc = p2m_entry_modify(p2m_get_hostp2m(d),
> +                          p2m_flags_to_type(l1e_get_flags(new)),
> +                          p2m_flags_to_type(old_flags), l1e_get_mfn(new),
> +                          l1e_get_mfn(*p), level);

why not passing old/new pte to reduce #parameters and thus stable
against future changes?

> +    if ( rc )
> +    {
> +        paging_unlock(d);
> +        return rc;
> +    }
> 
>      safe_write_pte(p, new);
>      if ( old_flags & _PAGE_PRESENT )
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index 0ece6608cb..83bd602fc4 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -45,65 +45,19 @@ static inline bool_t is_epte_valid(ept_entry_t *e)
>      return ((e->epte & ~(1ul << 63)) != 0 && e->sa_p2mt != p2m_invalid);
>  }
> 
> -/* returns : 0 for success, -errno otherwise */
>  static int atomic_write_ept_entry(struct p2m_domain *p2m,
>                                    ept_entry_t *entryptr, ept_entry_t new,
>                                    int level)
>  {
> -    int rc;
> -    unsigned long oldmfn = mfn_x(INVALID_MFN);
> -    bool_t check_foreign = (new.mfn != entryptr->mfn ||
> -                            new.sa_p2mt != entryptr->sa_p2mt);
> -
> -    if ( level )
> -    {
> -        ASSERT(!is_epte_superpage(&new)
> || !p2m_is_foreign(new.sa_p2mt));
> -        write_atomic(&entryptr->epte, new.epte);
> -        return 0;
> -    }
> -
> -    if ( unlikely(p2m_is_foreign(new.sa_p2mt)) )
> -    {
> -        rc = -EINVAL;
> -        if ( !is_epte_present(&new) )
> -                goto out;
> -
> -        if ( check_foreign )
> -        {
> -            struct domain *fdom;
> -
> -            if ( !mfn_valid(_mfn(new.mfn)) )
> -                goto out;
> -
> -            rc = -ESRCH;
> -            fdom = page_get_owner(mfn_to_page(_mfn(new.mfn)));
> -            if ( fdom == NULL )
> -                goto out;
> +    int rc = p2m_entry_modify(p2m, new.sa_p2mt, entryptr->sa_p2mt,
> +                              _mfn(new.mfn), _mfn(entryptr->mfn), level);
> 
> -            /* get refcount on the page */
> -            rc = -EBUSY;
> -            if ( !get_page(mfn_to_page(_mfn(new.mfn)), fdom) )
> -                goto out;
> -        }
> -    }
> -
> -    if ( unlikely(p2m_is_foreign(entryptr->sa_p2mt)) && check_foreign )
> -        oldmfn = entryptr->mfn;
> -
> -    p2m_entry_modify(p2m, new.sa_p2mt, entryptr->sa_p2mt, level);
> +    if ( rc )
> +        return rc;
> 
>      write_atomic(&entryptr->epte, new.epte);
> 
> -    if ( unlikely(oldmfn != mfn_x(INVALID_MFN)) )
> -        put_page(mfn_to_page(_mfn(oldmfn)));
> -
> -    rc = 0;
> -
> - out:
> -    if ( rc )
> -        gdprintk(XENLOG_ERR, "epte o:%"PRIx64" n:%"PRIx64" rc:%d\n",
> -                 entryptr->epte, new.epte, rc);
> -    return rc;
> +    return 0;
>  }
> 
>  static void ept_p2m_type_to_flags(struct p2m_domain *p2m, ept_entry_t
> *entry,
> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> index 3a8dc04efc..57afa37c71 100644
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -564,13 +564,6 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
> gfn_t gfn_, mfn_t mfn,
>          __trace_var(TRC_MEM_SET_P2M_ENTRY, 0, sizeof(t), &t);
>      }
> 
> -    if ( unlikely(p2m_is_foreign(p2mt)) )
> -    {
> -        /* hvm fixme: foreign types are only supported on ept at present */
> -        gdprintk(XENLOG_WARNING, "Unimplemented foreign p2m type.\n");
> -        return -EINVAL;
> -    }
> -
>      /* Carry out any eventually pending earlier changes first. */
>      rc = do_recalc(p2m, gfn);
>      if ( rc < 0 )
> diff --git a/xen/arch/x86/mm/shadow/common.c
> b/xen/arch/x86/mm/shadow/common.c
> index fe48c4a02b..ad670de515 100644
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -3189,7 +3189,8 @@ shadow_write_p2m_entry(struct domain *d,
> unsigned long gfn,
>           sh_unshadow_for_p2m_change(d, gfn, p, new, level);
> 
>      p2m_entry_modify(p2m_get_hostp2m(d),
> p2m_flags_to_type(l1e_get_flags(new)),
> -                     p2m_flags_to_type(l1e_get_flags(*p)), level);
> +                     p2m_flags_to_type(l1e_get_flags(*p)), l1e_get_mfn(new),
> +                     l1e_get_mfn(*p), level);
> 
>      /* Update the entry with new content */
>      safe_write_pte(p, new);
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index f4ec2becbd..2801a8ccca 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -932,11 +932,14 @@ int p2m_set_ioreq_server(struct domain *d,
> unsigned int flags,
>  struct hvm_ioreq_server *p2m_get_ioreq_server(struct domain *d,
>                                                unsigned int *flags);
> 
> -static inline void p2m_entry_modify(struct p2m_domain *p2m,
> p2m_type_t nt,
> -                                    p2m_type_t ot, unsigned int level)
> +static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t
> nt,
> +                                   p2m_type_t ot, mfn_t nfn, mfn_t ofn,
> +                                   unsigned int level)
>  {
> -    if ( level != 1 || nt == ot )
> -        return;
> +    BUG_ON(level > 1 && (nt == p2m_ioreq_server || nt ==
> p2m_map_foreign));
> +
> +    if ( level != 1 || (nt == ot && mfn_eq(nfn, ofn)) )
> +        return 0;

could also return here in case of nt==ot==p2m_ioreq_server,
otherwise p2m->ioreq.entry_count might be unnecessarily
inc/dec if mfn changes while type stays as p2m_ioreq_server...

> 
>      switch ( nt )
>      {
> @@ -948,6 +951,18 @@ static inline void p2m_entry_modify(struct
> p2m_domain *p2m, p2m_type_t nt,
>          p2m->ioreq.entry_count++;
>          break;
> 
> +    case p2m_map_foreign:
> +        if ( !mfn_valid(nfn) )
> +        {
> +            ASSERT_UNREACHABLE();
> +            return -EINVAL;
> +        }
> +
> +        if ( !page_get_owner_and_reference(mfn_to_page(nfn)) )
> +            return -EBUSY;
> +
> +        break;
> +
>      default:
>          break;
>      }
> @@ -959,9 +974,20 @@ static inline void p2m_entry_modify(struct
> p2m_domain *p2m, p2m_type_t nt,
>          p2m->ioreq.entry_count--;
>          break;
> 
> +    case p2m_map_foreign:
> +        if ( !mfn_valid(ofn) )
> +        {
> +            ASSERT_UNREACHABLE();
> +            return -EINVAL;
> +        }
> +        put_page(mfn_to_page(ofn));
> +        break;
> +
>      default:
>          break;
>      }
> +
> +    return 0;
>  }
> 
>  #endif /* _XEN_ASM_X86_P2M_H */
> --
> 2.17.2 (Apple Git-113)

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.