Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry.

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Don Slutz [mailto:dslutz@verizon.com]
> Sent: 30 January 2015 19:19
> To: Don Slutz; Jan Beulich; Paul Durrant
> Cc: Andrew Cooper; Ian Campbell; Wei Liu; George Dunlap; Ian Jackson;
> Stefano Stabellini; xen-devel@lists.xen.org; Keir (Xen.org)
> Subject: Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server
> failed do not retry.
> 
> On 01/30/15 13:17, Don Slutz wrote:
> > On 01/30/15 05:23, Jan Beulich wrote:
> >>>>> On 30.01.15 at 01:52, <dslutz@verizon.com> wrote:
> >>> I.E. do just what no backing DM does.
> >>
> >> _If_ this is correct, the if() modified here should be folded with the
> >> one a few lines up.
> >
> > Ok, will fold with the one a few lines up.  As expected without this
> > change the guest will hang (more details below).
> >
> >
> >> But looking at the description of the commit that
> >> introduced this (bac0999325 "x86 hvm: Do not incorrectly retire an
> >> instruction emulation...", almost immediately modified by f20f3c8ece
> >> "x86 hvm: On failed hvm_send_assist_req(), io emulation...") I doubt
> >> this is really what we want, or at the very least your change
> >> description should explain what was wrong with the original commit.
> >>
> >
> > Looking at these 2 commits, it looks to me like I need to handle these
> > cases also.  So it looks like having hvm_send_assist_req() return an int
> > 0, 1, & 2 is the simpler way.  V2 on the way soon.
> >
> 
> Soon after sending this, I came up with a 2nd way for fix.
> Change hvm_has_dm() to use hvm_select_ioreq_server().  Then
> the correct answer will be found, and so will not retry.
> 
> To avoid 2 calls to hvm_select_ioreq_server(), it makes
> snes to me to return s.  Also adding a call to hvm_complete_assist_req()
> would help.  So based on all this is is a possible v2:
> 

Yes, I think this approach does make a lot of sense.

> 
> commit 24eb5a839427ba80c1adf16ab656c19729f906be
> Author: Don Slutz <dslutz@verizon.com>
> Date:   Fri Jan 30 13:54:43 2015 -0500
> 
>     fixup use of hvm_send_assist_req
> 
>     Signed-off-by: Don Slutz <dslutz@verizon.com>
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 2ed4344..7c3c654 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -218,8 +218,11 @@ static int hvmemul_do_io(
>              vio->io_state = HVMIO_handle_mmio_awaiting_completion;
>          break;
>      case X86EMUL_UNHANDLEABLE:
> +    {
> +        void *s = hvm_has_dm(curr->domain, &p);
> +
>          /* If there is no backing DM, just ignore accesses */
> -        if ( !hvm_has_dm(curr->domain) )
> +        if ( !s )
>          {
>              rc = X86EMUL_OKAY;
>              vio->io_state = HVMIO_none;
> @@ -227,11 +230,12 @@ static int hvmemul_do_io(
>          else
>          {
>              rc = X86EMUL_RETRY;
> -            if ( !hvm_send_assist_req(&p) )
> +            if ( !hvm_send_assist_req(s, &p) )
>                  vio->io_state = HVMIO_none;
>              else if ( p_data == NULL )
>                  rc = X86EMUL_OKAY;
>          }
> +    }
>          break;
>      default:
>          BUG();
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 984af81..21d4a73 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2507,9 +2507,45 @@ int hvm_buffered_io_send(ioreq_t *p)
>      return 1;
>  }
> 
> -bool_t hvm_has_dm(struct domain *d)
> +static bool_t hvm_complete_assist_req(ioreq_t *p)

You'll need to change this to a void return as Jan requested.

>  {
> -    return !list_empty(&d->arch.hvm_domain.ioreq_server.list);
> +    HVMTRACE_1D(COMPLETE_ASSIST, p->type);
> +    ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG);
> +    switch ( p->type )
> +    {
> +    case IOREQ_TYPE_COPY:
> +    case IOREQ_TYPE_PIO:
> +        if ( p->dir == IOREQ_READ )
> +        {
> +            if ( !p->data_is_ptr )
> +                p->data = ~0ul;
> +            else
> +            {
> +                int i, step = p->df ? -p->size : p->size;
> +                uint32_t data = ~0;
> +
> +                for ( i = 0; i < p->count; i++ )
> +                    hvm_copy_to_guest_phys(p->data + step * i, &data,
> +                                           p->size);
> +            }
> +        }
> +        /* FALLTHRU */
> +    default:
> +        p->state = STATE_IORESP_READY;
> +        hvm_io_assist(p);
> +        break;
> +    }
> +
> +    return 1;
> +}
> +
> +void *hvm_has_dm(struct domain *d, ioreq_t *p)
> +{
> +    struct hvm_ioreq_server *s = hvm_select_ioreq_server(d, p);
> +
> +    if ( !s )
> +        hvm_complete_assist_req(p);

If you're going to complete the I/O here then...

> +    return (void *)s;
>  }
> 
>  bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
> @@ -2571,44 +2607,15 @@ bool_t
> hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
>      return 0;
>  }
> 
> -static bool_t hvm_complete_assist_req(ioreq_t *p)
> -{
> -    HVMTRACE_1D(COMPLETE_ASSIST, p->type);
> -    ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG);
> -    switch ( p->type )
> -    {
> -    case IOREQ_TYPE_COPY:
> -    case IOREQ_TYPE_PIO:
> -        if ( p->dir == IOREQ_READ )
> -        {
> -            if ( !p->data_is_ptr )
> -                p->data = ~0ul;
> -            else
> -            {
> -                int i, step = p->df ? -p->size : p->size;
> -                uint32_t data = ~0;
> -
> -                for ( i = 0; i < p->count; i++ )
> -                    hvm_copy_to_guest_phys(p->data + step * i, &data,
> -                                           p->size);
> -            }
> -        }
> -        /* FALLTHRU */
> -    default:
> -        p->state = STATE_IORESP_READY;
> -        hvm_io_assist(p);
> -        break;
> -    }
> -
> -    return 0; /* implicitly bins the i/o operation */
> -}
> -
> -bool_t hvm_send_assist_req(ioreq_t *p)
> +bool_t hvm_send_assist_req(void *vs, ioreq_t *p)
>  {
> -    struct hvm_ioreq_server *s =
> hvm_select_ioreq_server(current->domain, p);
> +    struct hvm_ioreq_server *s = vs;
> 
>      if ( !s )

... I think you can ASSERT(s) here, as you should never get here if an ioreq server was chosen.

> -        return hvm_complete_assist_req(p);
> +    {
> +        hvm_complete_assist_req(p);
> +        return 1;
> +    }

So, this function basically collapses down to just this call:

> 
>      return hvm_send_assist_req_to_ioreq_server(s, p);
>  }

  Paul

> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-
> x86/hvm/hvm.h
> index e3d2d9a..1923842 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -228,7 +228,7 @@ int hvm_vcpu_cacheattr_init(struct vcpu *v);
>  void hvm_vcpu_cacheattr_destroy(struct vcpu *v);
>  void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip);
> 
> -bool_t hvm_send_assist_req(ioreq_t *p);
> +bool_t hvm_send_assist_req(void *s, ioreq_t *p);
>  void hvm_broadcast_assist_req(ioreq_t *p);
> 
>  void hvm_get_guest_pat(struct vcpu *v, u64 *guest_pat);
> @@ -359,7 +359,7 @@ void hvm_hypervisor_cpuid_leaf(uint32_t sub_idx,
>  void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
>                                     unsigned int *ecx, unsigned int *edx);
>  void hvm_migrate_timers(struct vcpu *v);
> -bool_t hvm_has_dm(struct domain *d);
> +void *hvm_has_dm(struct domain *d, ioreq_t *p);
>  bool_t hvm_io_pending(struct vcpu *v);
>  void hvm_do_resume(struct vcpu *v);
>  void hvm_migrate_pirqs(struct vcpu *v);
> 
> 
>    -Don Slutz
> 
> 
> 
> > Which is the better codding style:
> >
> > 1) Add #defines for the return values?
> > 2) Just use numbers?
> > 3) Invert the sense 0 means worked, 1 is shutdown_deferral or
> >    domain_crash, 2 is hvm_complete_assist_req()?
> >
> >
> > I.E. (for just adding 2):
> >
> >
> > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> > index 2ed4344..c565151 100644
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -227,10 +227,19 @@ static int hvmemul_do_io(
> >          else
> >          {
> >              rc = X86EMUL_RETRY;
> > -            if ( !hvm_send_assist_req(&p) )
> > -                vio->io_state = HVMIO_none;
> > -            else if ( p_data == NULL )
> > +            switch ( hvm_send_assist_req(&p) )
> > +            {
> > +            case 2:
> >                  rc = X86EMUL_OKAY;
> > +                /* fall through */
> > +            case 0:
> > +                vio->io_state = HVMIO_none;
> > +                break;
> > +            case 1:
> > +                if ( p_data == NULL )
> > +                    rc = X86EMUL_OKAY;
> > +                break;
> > +            }
> >          }
> >
> >
> > ???
> >
> >
> >
> > I would think it would be good for the code using hvm_has_dm()
> > to also call on hvm_complete_assist_req().  hvm_has_dm() is a subset of
> > the cases that hvm_select_ioreq_server() checks for.
> >
> > Based on this, I think it would be better to remove the call on
> > hvm_has_dm() instead of adding a call to hvm_complete_assist_req().
> >
> >
> >    -Don Slutz
> > P.S. Details for hang:
> >
> > Using:
> >
> >  static bool_t hvm_complete_assist_req(ioreq_t *p)
> >  {
> > +    HVMTRACE_1D(COMPLETE_ASSIST, p->type);
> > ...
> > ):
> >
> > I get the trace:
> >
> > CPU1  745455716325 (+    1362)  VMEXIT      [ exitcode = 0x0000001e, rIP
> >  = 0x00101581 ]
> > CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
> > CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
> > io_state = 0 ret = 0 ]
> > CPU1  745455717846 (+    1521)  vlapic_accept_pic_intr [ i8259_target =
> > 1, accept_pic_int = 1 ]
> > CPU1  745455718209 (+     363)  VMENTRY
> > CPU1  745455719568 (+    1359)  VMEXIT      [ exitcode = 0x0000001e, rIP
> >  = 0x00101581 ]
> > CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
> > CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
> > io_state = 0 ret = 0 ]
> > CPU1  745455721083 (+    1515)  vlapic_accept_pic_intr [ i8259_target =
> > 1, accept_pic_int = 1 ]
> > CPU1  745455721422 (+     339)  VMENTRY
> > CPU1  745455722781 (+    1359)  VMEXIT      [ exitcode = 0x0000001e, rIP
> >  = 0x00101581 ]
> > CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
> > CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
> > io_state = 0 ret = 0 ]
> > CPU1  745455724299 (+    1518)  vlapic_accept_pic_intr [ i8259_target =
> > 1, accept_pic_int = 1 ]
> > CPU1  745455724656 (+     357)  VMENTRY
> > CPU1  745455726009 (+    1353)  VMEXIT      [ exitcode = 0x0000001e, rIP
> >  = 0x00101581 ]
> > CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
> > CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
> > io_state = 0 ret = 0 ]
> > CPU1  745455727539 (+    1530)  vlapic_accept_pic_intr [ i8259_target =
> > 1, accept_pic_int = 1 ]
> > CPU1  745455727899 (+     360)  VMENTRY
> > CPU1  745455729276 (+    1377)  VMEXIT      [ exitcode = 0x0000001e, rIP
> >  = 0x00101581 ]
> > CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
> > CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
> > io_state = 0 ret = 0 ]
> > CPU1  745455730803 (+    1527)  vlapic_accept_pic_intr [ i8259_target =
> > 1, accept_pic_int = 1 ]
> > CPU1  745455731163 (+     360)  VMENTRY
> > CPU1  745455732525 (+    1362)  VMEXIT      [ exitcode = 0x0000001e, rIP
> >  = 0x00101581 ]
> > CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
> > CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
> > io_state = 0 ret = 0 ]
> > CPU1  745455734049 (+    1524)  vlapic_accept_pic_intr [ i8259_target =
> > 1, accept_pic_int = 1 ]
> > CPU1  745455734385 (+     336)  VMENTRY
> > ...
> >
> >
> >> Jan
> >>
> >>> --- a/xen/arch/x86/hvm/emulate.c
> >>> +++ b/xen/arch/x86/hvm/emulate.c
> >>> @@ -228,7 +228,11 @@ static int hvmemul_do_io(
> >>>          {
> >>>              rc = X86EMUL_RETRY;
> >>>              if ( !hvm_send_assist_req(&p) )
> >>> +            {
> >>> +                /* Since the send failed, do not retry */
> >>> +                rc = X86EMUL_OKAY;
> >>>                  vio->io_state = HVMIO_none;
> >>> +            }
> >>>              else if ( p_data == NULL )
> >>>                  rc = X86EMUL_OKAY;
> >>>          }
> >>> --
> >>> 1.8.4
> >>
> >>
> >>