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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.)

> -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).

Jan

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