Re: [Xen-devel] [PATCH] x86/ioreq server: Asynchronously reset outstanding p2m_ioreq_server entries.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.04.17 at 16:44, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> 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@xxxxxxxxxxxxxxx>
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with two (cosmetic) remarks (may be addressed while committing):

> @@ -542,10 +544,17 @@ 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) )
> +
> +                    nt = p2m_recalc_type(e.recalc, e.sa_p2mt, p2m, gfn + i);
> +                    if ( nt != e.sa_p2mt )
>                      {
> -                         e.sa_p2mt = p2m_is_logdirty_range(p2m, gfn + i, gfn 
> + i)
> -                                     ? p2m_ram_logdirty : p2m_ram_rw;
> +                         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);
>                      }

Indentation in that scope is till wrong (and hence the badness is
being extended).

> @@ -744,6 +746,27 @@ 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 recalc, p2m_type_t t,

While this is bool now, ...

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

... this one still isn't.

Jan


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