> -----Original Message-----
> From: Jan Beulich [mailto:[email protected]]
> Sent: 11 August 2015 16:20
> To: Paul Durrant
> Cc: Andrew Cooper; [email protected]; 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, <[email protected]> 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 <[email protected]>
> > Reported-by: Sander Eikelenboom <[email protected]>
> > Tested-by: Roger Pau Monné <[email protected]>
>
> 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
[email protected]
http://lists.xen.org/xen-devel
