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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 11/05/17 12:12, Jan Beulich wrote:
> > > > 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)
NIT: space missing before )

> > +            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).
Xiong Zhang, can you confirm this is meant for Xen 4.9?

Cheers,

--
Julien Grall

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