Re: [PATCH v12 5/6] x86/ioreq server: Asynchronously reset outstanding p2m_ioreq_server entries.

[Yu Zhang <yu.c.zhang@linux.intel.com>]

On 4/7/2017 9:56 PM, George Dunlap wrote:
> On 07/04/17 12:31, Jan Beulich wrote:
>>>>> On 07.04.17 at 12:55, <yu.c.zhang@linux.intel.com> wrote:
>>> On 4/7/2017 6:26 PM, Jan Beulich wrote:
>>>>>>> On 07.04.17 at 11:53, <yu.c.zhang@linux.intel.com> wrote:
>>>>> On 4/7/2017 5:40 PM, Jan Beulich wrote:
>>>>>>>>> On 06.04.17 at 17:53, <yu.c.zhang@linux.intel.com> wrote:
>>>>>>> @@ -965,7 +987,7 @@ static mfn_t ept_get_entry(struct p2m_domain *p2m,
>>>>>>>         if ( is_epte_valid(ept_entry) )
>>>>>>>         {
>>>>>>>             if ( (recalc || ept_entry->recalc) &&
>>>>>>> -             p2m_is_changeable(ept_entry->sa_p2mt) )
>>>>>>> +             p2m_check_changeable(ept_entry->sa_p2mt) )
>>>>>> I think the distinction between these two is rather arbitrary, and I
>>>>>> also think this is part of the problem above: Distinguishing log-dirty
>>>>>> from ram-rw requires auxiliary data to be consulted. The same
>>>>>> ought to apply to ioreq-server, and then there wouldn't be a need
>>>>>> to have two p2m_*_changeable() flavors.
>>>>> Well, I think we have also discussed this quite long ago, here is the link.
>>>>> https://lists.xen.org/archives/html/xen-devel/2016-09/msg01017.html
>>>> That was a different discussion, namely not considering this ...
>>>>
>>>>>> Of course the subsequent use p2m_is_logdirty_range() may then
>>>>>> need amending.
>>>>>>
>>>>>> In the end it looks like you have the inverse problem here compared
>>>>>> to above: You should return ram-rw when the reset was already
>>>>>> initiated. At least that's how I would see the logic to match up with
>>>>>> the log-dirty handling (where the _effective_ rather than the last
>>>>>> stored type is being returned).
>>>> ... at all.
>>> Sorry I still do not get it.
>> Leaving ioreq-server out of the picture, what the function returns
>> to the caller is the type as it would be if a re-calculation would have
>> been done on the entry, even if that hasn't happened yet (the
>> function here shouldn't change any entries after all). I think we
>> want the same behavior here for what have been ioreq-server
>> entries (and which would become ram-rw after the next re-calc).
> How about something like the attached? (In-lined for convenience.)
>
>   -George
>
> After an ioreq server has unmapped, the remaining p2m_ioreq_server
> entries need to be reset back to p2m_ram_rw. This patch does this
> asynchronously with the current p2m_change_entry_type_global()
> interface.
>
> New field entry_count is introduced in struct p2m_domain, to record
> the number of p2m_ioreq_server p2m page table entries. One nature of
> these entries is that they only point to 4K sized page frames, because
> all p2m_ioreq_server entries are originated from p2m_ram_rw ones in
> p2m_change_type_one(). We do not need to worry about the counting for
> 2M/1G sized pages.
>
> This patch disallows mapping of an ioreq server, when there's still
> p2m_ioreq_server entry left, in case another mapping occurs right after
> the current one being unmapped, releases its lock, with p2m table not
> synced yet.
>
> This patch also disallows live migration, when there's remaining
> p2m_ioreq_server entry in p2m table. The core reason is our current
> implementation of p2m_change_entry_type_global() lacks information
> to resync p2m_ioreq_server entries correctly if global_logdirty is
> on.
>
> We still need to handle other recalculations, however; which means
> that when doing a recalculation, if the current type is
> p2m_ioreq_server, we check to see if p2m->ioreq.server is valid or
> not.  If it is, we leave it as type p2m_ioreq_server; if not, we reset
> it to p2m_ram as appropriate.
>
> To avoid code duplication, lift recalc_type() out of p2m-pt.c and use
> it for all type recalculations (both in p2m-pt.c and p2m-ept.c).
>
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
> ---
>   xen/arch/x86/hvm/ioreq.c  |  8 ++++++
>   xen/arch/x86/mm/hap/hap.c |  9 ++++++
>   xen/arch/x86/mm/p2m-ept.c | 73
> +++++++++++++++++++++++++++++------------------
>   xen/arch/x86/mm/p2m-pt.c  | 58 ++++++++++++++++++++++++-------------
>   xen/arch/x86/mm/p2m.c     |  9 ++++++
>   xen/include/asm-x86/p2m.h | 26 ++++++++++++++++-
>   6 files changed, 135 insertions(+), 48 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> index 5bf3b6d..07a6c26 100644
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -955,6 +955,14 @@ int hvm_map_mem_type_to_ioreq_server(struct domain
> *d, ioservid_t id,
>
>       spin_unlock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
>
> +    if ( rc == 0 && flags == 0 )
> +    {
> +        struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +
> +        if ( read_atomic(&p2m->ioreq.entry_count) )
> +            p2m_change_entry_type_global(d, p2m_ioreq_server, p2m_ram_rw);
> +    }
> +
>       return rc;
>   }
>
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index a57b385..4b591fe 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -187,6 +187,15 @@ out:
>    */
>   static int hap_enable_log_dirty(struct domain *d, bool_t log_global)
>   {
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +
> +    /*
> +     * Refuse to turn on global log-dirty mode if
> +     * there are outstanding p2m_ioreq_server pages.
> +     */
> +    if ( log_global && read_atomic(&p2m->ioreq.entry_count) )
> +        return -EBUSY;
> +
>       /* turn on PG_log_dirty bit in paging mode */
>       paging_lock(d);
>       d->arch.paging.mode |= PG_log_dirty;
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index cc1eb21..9f41067 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -533,6 +533,7 @@ static int resolve_misconfig(struct p2m_domain *p2m,
> unsigned long gfn)
>               {
>                   for ( gfn -= i, i = 0; i < EPT_PAGETABLE_ENTRIES; ++i )
>                   {
> +                    p2m_type_t nt;
>                       e = atomic_read_ept_entry(&epte[i]);
>                       if ( e.emt == MTRR_NUM_TYPES )
>                           e.emt = 0;
> @@ -542,10 +543,15 @@ static int resolve_misconfig(struct p2m_domain
> *p2m, unsigned long gfn)
>                                                  _mfn(e.mfn), 0, &ipat,
>                                                  e.sa_p2mt ==
> p2m_mmio_direct);
>                       e.ipat = ipat;
> -                    if ( e.recalc && p2m_is_changeable(e.sa_p2mt) )
> -                    {
> -                         e.sa_p2mt = p2m_is_logdirty_range(p2m, gfn +
> i, gfn + i)
> -                                     ? p2m_ram_logdirty : p2m_ram_rw;
> +                    nt = p2m_recalc_type(e.recalc, e.sa_p2mt, p2m, gfn
> + i);
> +                    if ( nt != e.sa_p2mt ) {
> +                         if ( e.sa_p2mt == p2m_ioreq_server )
> +                         {
> +                             ASSERT(p2m->ioreq.entry_count > 0);
> +                             p2m->ioreq.entry_count--;
> +                         }
> +
> +                         e.sa_p2mt = nt;
>                            ept_p2m_type_to_flags(p2m, &e, e.sa_p2mt,
> e.access);
>                       }
>                       e.recalc = 0;
> @@ -562,23 +568,24 @@ static int resolve_misconfig(struct p2m_domain
> *p2m, unsigned long gfn)
>
>                   if ( recalc && p2m_is_changeable(e.sa_p2mt) )
>                   {
> -                     unsigned long mask = ~0UL << (level *
> EPT_TABLE_ORDER);
> -
> -                     switch ( p2m_is_logdirty_range(p2m, gfn & mask,
> -                                                    gfn | ~mask) )
> -                     {
> -                     case 0:
> -                          e.sa_p2mt = p2m_ram_rw;
> -                          e.recalc = 0;
> -                          break;
> -                     case 1:
> -                          e.sa_p2mt = p2m_ram_logdirty;
> -                          e.recalc = 0;
> -                          break;
> -                     default: /* Force split. */
> -                          emt = -1;
> -                          break;
> -                     }
> +                    unsigned long mask = ~0UL << (level * EPT_TABLE_ORDER);
> +
> +                    ASSERT(e.sa_p2mt != p2m_ioreq_server);
> +                    switch ( p2m_is_logdirty_range(p2m, gfn & mask,
> +                                                   gfn | ~mask) )
> +                    {
> +                    case 0:
> +                        e.sa_p2mt = p2m_ram_rw;
> +                        e.recalc = 0;
> +                        break;
> +                    case 1:
> +                        e.sa_p2mt = p2m_ram_logdirty;
> +                        e.recalc = 0;
> +                        break;
> +                    default: /* Force split. */
> +                        emt = -1;
> +                        break;
> +                    }


So here I guess only the "ASSERT(e.sa_p2mt != p2m_ioreq_server);" is 
new, right?
Thanks George, it's great to know your helper routine works also for the 
NPT.

Yu

>                   }
>                   if ( unlikely(emt < 0) )
>                   {
> @@ -816,6 +823,22 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long
> gfn, mfn_t mfn,
>           new_entry.suppress_ve = is_epte_valid(&old_entry) ?
>                                       old_entry.suppress_ve : 1;
>
> +    /*
> +     * p2m_ioreq_server is only used for 4K pages, so the
> +     * count shall only happen on ept page table entries.
> +     */
> +    if ( p2mt == p2m_ioreq_server )
> +    {
> +        ASSERT(i == 0);
> +        p2m->ioreq.entry_count++;
> +    }
> +
> +    if ( ept_entry->sa_p2mt == p2m_ioreq_server )
> +    {
> +        ASSERT(p2m->ioreq.entry_count > 0 && i == 0);
> +        p2m->ioreq.entry_count--;
> +    }
> +
>       rc = atomic_write_ept_entry(ept_entry, new_entry, target);
>       if ( unlikely(rc) )
>           old_entry.epte = 0;
> @@ -964,12 +987,8 @@ static mfn_t ept_get_entry(struct p2m_domain *p2m,
>
>       if ( is_epte_valid(ept_entry) )
>       {
> -        if ( (recalc || ept_entry->recalc) &&
> -             p2m_is_changeable(ept_entry->sa_p2mt) )
> -            *t = p2m_is_logdirty_range(p2m, gfn, gfn) ? p2m_ram_logdirty
> -                                                      : p2m_ram_rw;
> -        else
> -            *t = ept_entry->sa_p2mt;
> +        *t = p2m_recalc_type(recalc || ept_entry->recalc,
> +                             ept_entry->sa_p2mt, p2m, gfn);
>           *a = ept_entry->access;
>           if ( sve )
>               *sve = ept_entry->suppress_ve;
> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> index c0055f3..e481f53 100644
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -389,6 +389,7 @@ static int do_recalc(struct p2m_domain *p2m,
> unsigned long gfn)
>           {
>               unsigned long mask = ~0UL << (level * PAGETABLE_ORDER);
>
> +            ASSERT(p2m_flags_to_type(l1e_get_flags(*pent)) !=
> p2m_ioreq_server);
>               if ( !needs_recalc(l1, *pent) ||
>
> !p2m_is_changeable(p2m_flags_to_type(l1e_get_flags(*pent))) ||
>                    p2m_is_logdirty_range(p2m, gfn & mask, gfn | ~mask) >= 0 )
> @@ -436,17 +437,18 @@ static int do_recalc(struct p2m_domain *p2m,
> unsigned long gfn)
>            needs_recalc(l1, *pent) )
>       {
>           l1_pgentry_t e = *pent;
> +        p2m_type_t ot, nt;
> +        unsigned long mask = ~0UL << (level * PAGETABLE_ORDER);
>
>           if ( !valid_recalc(l1, e) )
>               P2M_DEBUG("bogus recalc leaf at d%d:%lx:%u\n",
>                         p2m->domain->domain_id, gfn, level);
> -        if ( p2m_is_changeable(p2m_flags_to_type(l1e_get_flags(e))) )
> +        ot = p2m_flags_to_type(l1e_get_flags(e));
> +        nt = p2m_recalc_type_range(true, ot, p2m, gfn & mask, gfn | ~mask);
> +        if ( nt != ot )
>           {
> -            unsigned long mask = ~0UL << (level * PAGETABLE_ORDER);
> -            p2m_type_t p2mt = p2m_is_logdirty_range(p2m, gfn & mask,
> gfn | ~mask)
> -                              ? p2m_ram_logdirty : p2m_ram_rw;
>               unsigned long mfn = l1e_get_pfn(e);
> -            unsigned long flags = p2m_type_to_flags(p2m, p2mt,
> +            unsigned long flags = p2m_type_to_flags(p2m, nt,
>                                                       _mfn(mfn), level);
>
>               if ( level )
> @@ -460,9 +462,17 @@ static int do_recalc(struct p2m_domain *p2m,
> unsigned long gfn)
>                        mfn &= ~((unsigned long)_PAGE_PSE_PAT >> PAGE_SHIFT);
>                   flags |= _PAGE_PSE;
>               }
> +
> +            if ( ot == p2m_ioreq_server )
> +            {
> +                ASSERT(p2m->ioreq.entry_count > 0);
> +                ASSERT(level == 0);
> +                p2m->ioreq.entry_count--;
> +            }
> +
>               e = l1e_from_pfn(mfn, flags);
>               p2m_add_iommu_flags(&e, level,
> -                                (p2mt == p2m_ram_rw)
> +                                (nt == p2m_ram_rw)
>                                   ? IOMMUF_readable|IOMMUF_writable : 0);
>               ASSERT(!needs_recalc(l1, e));
>           }
> @@ -606,6 +616,8 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned
> long gfn, mfn_t mfn,
>
>       if ( page_order == PAGE_ORDER_4K )
>       {
> +        p2m_type_t p2mt_old;
> +
>           rc = p2m_next_level(p2m, &table, &gfn_remainder, gfn,
>                               L2_PAGETABLE_SHIFT - PAGE_SHIFT,
>                               L2_PAGETABLE_ENTRIES, PGT_l1_page_table, 1);
> @@ -629,6 +641,21 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned
> long gfn, mfn_t mfn,
>           if ( entry_content.l1 != 0 )
>               p2m_add_iommu_flags(&entry_content, 0, iommu_pte_flags);
>
> +        p2mt_old = p2m_flags_to_type(l1e_get_flags(*p2m_entry));
> +
> +        /*
> +         * p2m_ioreq_server is only used for 4K pages, so
> +         * the count shall only be performed for level 1 entries.
> +         */
> +        if ( p2mt == p2m_ioreq_server )
> +            p2m->ioreq.entry_count++;
> +
> +        if ( p2mt_old == p2m_ioreq_server )
> +        {
> +            ASSERT(p2m->ioreq.entry_count > 0);
> +            p2m->ioreq.entry_count--;
> +        }
> +
>           /* level 1 entry */
>           p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 1);
>           /* NB: paging_write_p2m_entry() handles tlb flushes properly */
> @@ -726,15 +753,6 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned
> long gfn, mfn_t mfn,
>       return rc;
>   }
>
> -static inline p2m_type_t recalc_type(bool_t recalc, p2m_type_t t,
> -                                     struct p2m_domain *p2m, unsigned
> long gfn)
> -{
> -    if ( !recalc || !p2m_is_changeable(t) )
> -        return t;
> -    return p2m_is_logdirty_range(p2m, gfn, gfn) ? p2m_ram_logdirty
> -                                                : p2m_ram_rw;
> -}
> -
>   static mfn_t
>   p2m_pt_get_entry(struct p2m_domain *p2m, unsigned long gfn,
>                    p2m_type_t *t, p2m_access_t *a, p2m_query_t q,
> @@ -820,8 +838,8 @@ pod_retry_l3:
>               mfn = _mfn(l3e_get_pfn(*l3e) +
>                          l2_table_offset(addr) * L1_PAGETABLE_ENTRIES +
>                          l1_table_offset(addr));
> -            *t = recalc_type(recalc || _needs_recalc(flags),
> -                             p2m_flags_to_type(flags), p2m, gfn);
> +            *t = p2m_recalc_type(recalc || _needs_recalc(flags),
> +                                 p2m_flags_to_type(flags), p2m, gfn);
>               unmap_domain_page(l3e);
>
>               ASSERT(mfn_valid(mfn) || !p2m_is_ram(*t));
> @@ -859,8 +877,8 @@ pod_retry_l2:
>       if ( flags & _PAGE_PSE )
>       {
>           mfn = _mfn(l2e_get_pfn(*l2e) + l1_table_offset(addr));
> -        *t = recalc_type(recalc || _needs_recalc(flags),
> -                         p2m_flags_to_type(flags), p2m, gfn);
> +        *t = p2m_recalc_type(recalc || _needs_recalc(flags),
> +                             p2m_flags_to_type(flags), p2m, gfn);
>           unmap_domain_page(l2e);
>
>           ASSERT(mfn_valid(mfn) || !p2m_is_ram(*t));
> @@ -896,7 +914,7 @@ pod_retry_l1:
>           return INVALID_MFN;
>       }
>       mfn = _mfn(l1e_get_pfn(*l1e));
> -    *t = recalc_type(recalc || _needs_recalc(flags), l1t, p2m, gfn);
> +    *t = p2m_recalc_type(recalc || _needs_recalc(flags), l1t, p2m, gfn);
>       unmap_domain_page(l1e);
>
>       ASSERT(mfn_valid(mfn) || !p2m_is_ram(*t) || p2m_is_paging(*t));
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index b84add0..4169d18 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -317,6 +317,15 @@ int p2m_set_ioreq_server(struct domain *d,
>           if ( p2m->ioreq.server != NULL )
>               goto out;
>
> +        /*
> +         * It is possible that an ioreq server has just been unmapped,
> +         * released the spin lock, with some p2m_ioreq_server entries
> +         * in p2m table remained. We shall refuse another ioreq server
> +         * mapping request in such case.
> +         */
> +        if ( read_atomic(&p2m->ioreq.entry_count) )
> +            goto out;
> +
>           p2m->ioreq.server = s;
>           p2m->ioreq.flags = flags;
>       }
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 4521620..8077d0d 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -120,7 +120,10 @@ typedef unsigned int p2m_query_t;
>
>   /* Types that can be subject to bulk transitions. */
>   #define P2M_CHANGEABLE_TYPES (p2m_to_mask(p2m_ram_rw) \
> -                              | p2m_to_mask(p2m_ram_logdirty) )
> +                              | p2m_to_mask(p2m_ram_logdirty) \
> +                              | p2m_to_mask(p2m_ioreq_server) )
> +
> +#define P2M_IOREQ_TYPES (p2m_to_mask(p2m_ioreq_server))
>
>   #define P2M_POD_TYPES (p2m_to_mask(p2m_populate_on_demand))
>
> @@ -157,6 +160,7 @@ typedef unsigned int p2m_query_t;
>   #define p2m_is_readonly(_t) (p2m_to_mask(_t) & P2M_RO_TYPES)
>   #define p2m_is_discard_write(_t) (p2m_to_mask(_t) &
> P2M_DISCARD_WRITE_TYPES)
>   #define p2m_is_changeable(_t) (p2m_to_mask(_t) & P2M_CHANGEABLE_TYPES)
> +#define p2m_is_ioreq(_t) (p2m_to_mask(_t) & P2M_IOREQ_TYPES)
>   #define p2m_is_pod(_t) (p2m_to_mask(_t) & P2M_POD_TYPES)
>   #define p2m_is_grant(_t) (p2m_to_mask(_t) & P2M_GRANT_TYPES)
>   /* Grant types are *not* considered valid, because they can be
> @@ -349,6 +353,7 @@ struct p2m_domain {
>             * are to be emulated by an ioreq server.
>             */
>            unsigned int flags;
> +         unsigned long entry_count;
>        } ioreq;
>   };
>
> @@ -744,6 +749,25 @@ static inline p2m_type_t p2m_flags_to_type(unsigned
> long flags)
>       return (flags >> 12) & 0x7f;
>   }
>
> +static inline p2m_type_t p2m_recalc_type_range(bool_t recalc, p2m_type_t t,
> +                                               struct p2m_domain *p2m,
> +                                               unsigned long gfn_start,
> +                                               unsigned long gfn_end)
> +{
> +    if ( !recalc || !p2m_is_changeable(t) )
> +        return t;
> +    if ( t == p2m_ioreq_server && p2m->ioreq.server != NULL )
> +        return t;
> +    return p2m_is_logdirty_range(p2m, gfn_start, gfn_end) ?
> p2m_ram_logdirty
> +        : p2m_ram_rw;
> +}
> +
> +static inline p2m_type_t p2m_recalc_type(bool_t recalc, p2m_type_t t,
> +                                         struct p2m_domain *p2m,
> unsigned long gfn)
> +{
> +    return p2m_recalc_type_range(recalc, t, p2m, gfn, gfn);
> +}
> +
>   int p2m_pt_handle_deferred_changes(uint64_t gpa);
>
>   /*
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel


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