RE: [PATCH 1/3] x86/HVM: fold hvm_io_assist() into its only caller

[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 1/3] x86/HVM: fold hvm_io_assist() into its only caller
> 
> While there are two call sites, the function they're in can be slightly
> re-arranged such that the code sequence can be added at its bottom. Note
> that the function's only caller has already checked sv->pending, and
> that the prior while() loop was just a slightly more fancy if()
> (allowing an early break out of the construct).
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -103,23 +103,12 @@ bool hvm_io_pending(struct vcpu *v)
>      return false;
>  }
> 
> -static void hvm_io_assist(struct hvm_ioreq_vcpu *sv, uint64_t data)
> -{
> -    struct vcpu *v = sv->vcpu;
> -    ioreq_t *ioreq = &v->arch.hvm.hvm_io.io_req;
> -
> -    if ( hvm_ioreq_needs_completion(ioreq) )
> -        ioreq->data = data;
> -
> -    sv->pending = false;
> -}
> -
>  static bool hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, ioreq_t *p)
>  {
>      unsigned int prev_state = STATE_IOREQ_NONE;
> +    uint64_t data = ~0;
> 
> -    while ( sv->pending )
> -    {
> +    do {
>          unsigned int state = p->state;

I guess this is beneficial from the point of view of restricting cope and...

> 
>          smp_rmb();
> @@ -132,7 +121,6 @@ static bool hvm_wait_for_io(struct hvm_i
>               * emulator is dying and it races with an I/O being
>               * requested.
>               */
> -            hvm_io_assist(sv, ~0ul);
>              break;

...(as you say) allowing this early break, but a forward jump to an 'out' label 
would be more consistent with other code. It works though so...

Reviewed-by: Paul Durrant <paul@xxxxxxx>


>          }
> 
> @@ -149,7 +137,7 @@ static bool hvm_wait_for_io(struct hvm_i
>          {
>          case STATE_IORESP_READY: /* IORESP_READY -> NONE */
>              p->state = STATE_IOREQ_NONE;
> -            hvm_io_assist(sv, p->data);
> +            data = p->data;
>              break;
>          case STATE_IOREQ_READY:  /* IOREQ_{READY,INPROCESS} -> IORESP_READY 
> */
>          case STATE_IOREQ_INPROCESS:
> @@ -164,7 +152,13 @@ static bool hvm_wait_for_io(struct hvm_i
>              domain_crash(sv->vcpu->domain);
>              return false; /* bail */
>          }
> -    }
> +    } while ( false );
> +
> +    p = &sv->vcpu->arch.hvm.hvm_io.io_req;
> +    if ( hvm_ioreq_needs_completion(p) )
> +        p->data = data;
> +
> +    sv->pending = false;
> 
>      return true;
>  }