RE: [PATCH 2/3] x86/HVM: re-work hvm_wait_for_io() a little

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 15 July 2020 13:04
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné 
> <roger.pau@xxxxxxxxxx>; Wei Liu
> <wl@xxxxxxx>; Paul Durrant <paul@xxxxxxx>
> Subject: [PATCH 2/3] x86/HVM: re-work hvm_wait_for_io() a little
> 
> Convert the function's main loop to a more ordinary one, without goto
> and without initial steps not needing to be inside a loop at all.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -106,24 +106,17 @@ bool hvm_io_pending(struct vcpu *v)
>  static bool hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, ioreq_t *p)
>  {
>      unsigned int prev_state = STATE_IOREQ_NONE;
> +    unsigned int state = p->state;
>      uint64_t data = ~0;
> 
> -    do {
> -        unsigned int state = p->state;


Oh, you lose the loop here anyway so the conversion in patch #1 was only 
transient.

> -
> -        smp_rmb();
> -
> -    recheck:
> -        if ( unlikely(state == STATE_IOREQ_NONE) )
> -        {
> -            /*
> -             * The only reason we should see this case is when an
> -             * emulator is dying and it races with an I/O being
> -             * requested.
> -             */
> -            break;
> -        }
> +    smp_rmb();
> 
> +    /*
> +     * The only reason we should see this condition be false is when an
> +     * emulator dying races with I/O being requested.
> +     */
> +    while ( likely(state != STATE_IOREQ_NONE) )
> +    {
>          if ( unlikely(state < prev_state) )
>          {
>              gdprintk(XENLOG_ERR, "Weird HVM ioreq state transition %u -> 
> %u\n",
> @@ -139,20 +132,24 @@ static bool hvm_wait_for_io(struct hvm_i
>              p->state = STATE_IOREQ_NONE;
>              data = p->data;
>              break;
> +

Possibly mention the style fix-up in the comment comment.

>          case STATE_IOREQ_READY:  /* IOREQ_{READY,INPROCESS} -> IORESP_READY 
> */
>          case STATE_IOREQ_INPROCESS:
>              wait_on_xen_event_channel(sv->ioreq_evtchn,
>                                        ({ state = p->state;
>                                           smp_rmb();
>                                           state != prev_state; }));
> -            goto recheck;
> +            continue;
> +

You could just break out of the switch now, I guess. Anyway...

Reviewed-by: Paul Durrant <paul@xxxxxxx>

>          default:
>              gdprintk(XENLOG_ERR, "Weird HVM iorequest state %u\n", state);
>              sv->pending = false;
>              domain_crash(sv->vcpu->domain);
>              return false; /* bail */
>          }
> -    } while ( false );
> +
> +        break;
> +    }
> 
>      p = &sv->vcpu->arch.hvm.hvm_io.io_req;
>      if ( hvm_ioreq_needs_completion(p) )