Re: [Xen-devel] [PATCH] x86/hvm: don't rely on shared ioreq state for completion handling

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 11 August 2015 16:20
> To: Paul Durrant
> Cc: Andrew Cooper; xen-devel@xxxxxxxxxxxxxxxxxxxx; Keir (Xen.org)
> Subject: Re: [PATCH] x86/hvm: don't rely on shared ioreq state for
> completion handling
> 
> >>> On 31.07.15 at 17:34, <paul.durrant@xxxxxxxxxx> wrote:
> > Both hvm_io_pending() and hvm_wait_for_io() use the shared (with
> emulator)
> > ioreq structure to determined whether there is a pending I/O. The latter
> > will
> > misbehave if the shared state is driven to STATE_IOREQ_NONE by the
> emulator,
> > or when the shared ioreq page is cleared for re-insertion into the guest
> > P2M when the ioreq server is disabled (STATE_IOREQ_NONE == 0) because
> it
> > will terminate its wait without calling hvm_io_assist() to adjust Xen's
> > internal I/O emulation state. This may then lead to an io completion
> > handler finding incorrect internal emulation state and calling
> > domain_crash().
> >
> > This patch fixes the problem by adding a pending flag to the ioreq server's
> > per-vcpu structure which cannot be directly manipulated by the emulator
> > and thus can be used to determine whether an I/O is actually pending for
> > that vcpu on that ioreq server. If an I/O is pending and the shared state
> > is seen to go to STATE_IOREQ_NONE then it can be treated as an abnormal
> > completion of emulation (hence the data placed in the shared structure
> > is not used) and the internal state is adjusted as for a normal completion.
> > Thus, when a completion handler subsequently runs, the internal state is as
> > expected and domain_crash() will not be called.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > Reported-by: Sander Eikelenboom <linux@xxxxxxxxxxxxxx>
> > Tested-by: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
> 
> I realize this went in already, but ...
> 
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -412,44 +412,57 @@ bool_t hvm_io_pending(struct vcpu *v)
> >                            &d->arch.hvm_domain.ioreq_server.list,
> >                            list_entry )
> >      {
> > -        ioreq_t *p = get_ioreq(s, v);
> > +        struct hvm_ioreq_vcpu *sv;
> >
> > -        if ( p->state != STATE_IOREQ_NONE )
> > -            return 1;
> > +        list_for_each_entry ( sv,
> > +                              &s->ioreq_vcpu_list,
> > +                              list_entry )
> > +        {
> > +            if ( sv->vcpu == v && sv->pending )
> > +                return 1;
> > +        }
> 
> ... while from the review of the original series I recall that doing the
> outer loop without any lock is fine (due to using domain_pause()
> when registering servers) I'm not convinced this extends to the
> inner loop. Can you clarify please? (There are a couple more such
> loops that I can't immediately see being protected by a lock.)

Yes, I think you are right. If a cpu were to disappear then the list walk would 
be compromised. It should either be locked or rcu in all places.

> 
> > -static void hvm_io_assist(ioreq_t *p)
> > +static void hvm_io_assist(struct hvm_ioreq_vcpu *sv, uint64_t data)
> >  {
> > -    struct vcpu *curr = current;
> > -    struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io;
> > -
> > -    p->state = STATE_IOREQ_NONE;
> > +    struct vcpu *v = sv->vcpu;
> > +    struct hvm_vcpu_io *vio = &v->arch.hvm_vcpu.hvm_io;
> >
> >      if ( hvm_vcpu_io_need_completion(vio) )
> >      {
> >          vio->io_req.state = STATE_IORESP_READY;
> > -        vio->io_req.data = p->data;
> > +        vio->io_req.data = data;
> >      }
> >      else
> >          vio->io_req.state = STATE_IOREQ_NONE;
> >
> > -    msix_write_completion(curr);
> > -    vcpu_end_shutdown_deferral(curr);
> > +    msix_write_completion(v);
> > +    vcpu_end_shutdown_deferral(v);
> > +
> > +    sv->pending = 0;
> >  }
> 
> Also the renaming of "curr" here is less than optimal, not the least
> because msix_write_completion() requires v == current. I.e. I'd
> like to ask for a cleanup patch converting v back to curr, adding
> ASSERT(curr == current) (and if that doesn't hold we've got a
> problem).

The naming was changed because I am now digging the vcpu pointer out of the 
hvm_ioreq_vcpu struct. IMO any renaming or ASSERT really belongs all the way 
out in hvm_do_resume(). I'll come up with a patch.

  Paul

> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel