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:46
> 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 11.08.15 at 17:32, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>  -----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.
> 
> I don't think we need to be concerned of vCPU-s disappearing,
> since that doesn't happen during the lifetime of a VM. And the
> hvm_do_resume() path is used only for domains still alive. Of
> course, if any of the lockless loops sit on paths reachable after
> a domain got marked dying, that would need fixing.
> 
> What I'm more concerned about are list manipulations behind
> the back of a list traversing CPU. Or do those happen only upon
> vCPU creation/destruction?

Theoretically we could do vcpu hot remove, couldn't we? That's the case I was 
thinking of.

  Paul

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