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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.01.15 at 20:19, <dslutz@xxxxxxxxxxx> wrote:
> 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:

Looks reasonable (awaiting Paul's opinion though) except for
mechanical aspects:

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

I can't see why this needs to return void * instead of a properly typed
pointer.

> @@ -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 */
> -}

Moving hvm_has_dm() down here would make the patch smaller and
hence quite a bit easier to review (as one doesn't need to manually
check the differences between the much larger added/removed
pieces of code.

Jan


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