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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 2/19/19 6:14 AM, Tian, Kevin wrote:
>> 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...

That's an interesting idea.  But it would have you do an extra check for
all operations, rather than doing an extra increment / decrement in a
very unusual corner case.  Probably faster just to let the uncommon case
be a tiny bit slower.

 -George

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