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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 07.04.17 at 15:56, <george.dunlap@citrix.com> wrote:

Most important thing first: The logic looks correct to me now.

> --- 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]);

Blank line between these two please.

> @@ -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 ) {

Brace on its own line please.

> +                         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);
>                      }

This changes why ept_p2m_type_to_flags() is bypassed. I think
this is correct, but that wasn't immediately clear.

> @@ -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 if I'm getting it right the change here really is just the addition of
an ASSERT() and re-indentation? I think you will want to also adjust
indentation in the previous hunk then. (I'm afraid it was me who
broke it back when introducing that code...)

> @@ -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;
>  }

I would have hoped for the two ASSERT()s to be added for the
2M and 1G cases, which we did talk about.

> --- 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)

I don't think this and P2M_IOREQ_TYPES are needed anymore.

> @@ -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,

Just "bool" please (also below).

> +                                               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;

Commonly we'd align the : with either the ? or the start of the
conditional expression.

Jan

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