Re: [Xen-devel] [PATCH V4] x86/ioreq_server: Make p2m_finish_type_change actually work

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.05.17 at 04:42, <xiong.y.zhang@xxxxxxxxx> wrote:
> Commit 6d774a951696 ("x86/ioreq server: synchronously reset outstanding
> p2m_ioreq_server entries when an ioreq server unmaps") introduced
> p2m_finish_type_change(), which was meant to synchronously finish a
> previously initiated type change over a gpfn range.  It did this by
> calling get_entry(), checking if it was the appropriate type, and then
> calling set_entry().
> 
> Unfortunately, a previous commit (1679e0df3df6 "x86/ioreq server:
> asynchronously reset outstanding p2m_ioreq_server entries") modified
> get_entry() to always return the new type after the type change, meaning
> that p2m_finish_type_change() never changed any entries.  Which means
> when an ioreq server was detached and then re-attached (as happens in
> XenGT on reboot) the re-attach failed.
> 
> Fix this by using the existing p2m-specific recalculation logic instead
> of doing a read-check-write loop.
> 
> Fix: 'commit 6d774a951696 ("x86/ioreq server: synchronously reset
>       outstanding p2m_ioreq_server entries when an ioreq server unmaps")'
> 
> Signed-off-by: Xiong Zhang <xiong.y.zhang@xxxxxxxxx>
> Signed-off-by: Yu Zhang <yu.c.zhang@xxxxxxxxxxxxxxx>
> Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
albeit I'm not overly happy with ...

> +int p2m_finish_type_change(struct domain *d,
> +                           gfn_t first_gfn, unsigned long max_nr)
>  {
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
> -    p2m_type_t t;
>      unsigned long gfn = gfn_x(first_gfn);
>      unsigned long last_gfn = gfn + max_nr - 1;
> -
> -    ASSERT(ot != nt);
> -    ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt));
> +    int rc = 0;
>  
>      p2m_lock(p2m);
>  
>      last_gfn = min(last_gfn, p2m->max_mapped_pfn);
>      while ( gfn <= last_gfn )
>      {
> -        get_gfn_query_unlocked(d, gfn, &t);
> -
> -        if ( t == ot )
> -            p2m_change_type_one(d, gfn, t, nt);
> +        rc = p2m->recalc(p2m, gfn);
> +        /* 
> +         * ept->recalc could return 0/1/-ENOMEM. pt->recalc could return
> +         * 0/-ENOMEM/-ENOENT, -ENOENT isn't an error as we are looping
> +         * gfn here.
> +         */
> +        if ( rc == -ENOENT)
> +            rc = 0;
> +        else if ( rc < 0 )
> +        {
> +            gdprintk(XENLOG_ERR, "p2m->recalc failed! Dom%d gfn=%lx\n",
> +                     d->domain_id, gfn);

... a message being logged here.

Also I'm sure it was pointed out before that if this is meant for
4.9 (which I assume it is) you should have Cc-ed Julien (now
added).

Jan


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