From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Jan Beulich" <JBeulich@suse.com>
Subject: Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq
 server failed do not retry.
Date: Mon, 02 Feb 2015 08:36:58 +0000
Message-ID: <54CF453A020000780005BC13@mail.emea.novell.com>
References: <1422579150-6970-1-git-send-email-dslutz@verizon.com>
	<1422579150-6970-4-git-send-email-dslutz@verizon.com>
	<54CB69C7020000780005B2D0@mail.emea.novell.com>
	<54CBCAD4.2090106@terremark.com> <54CBD93E.3060005@terremark.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <54CBD93E.3060005@terremark.com>
Content-Disposition: inline
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Paul Durrant <Paul.Durrant@citrix.com>, Don Slutz <dslutz@verizon.com>
Cc: Wei Liu <wei.liu2@citrix.com>, Ian Campbell <ian.campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, George Dunlap <george.dunlap@eu.citrix.com>, Andrew Cooper <andrew.cooper3@citrix.com>, Ian Jackson <ian.jackson@eu.citrix.com>, xen-devel@lists.xen.org, Keir Fraser <keir@xen.org>
List-Id: xen-devel@lists.xenproject.org

>>> On 30.01.15 at 20:19, <dslutz@verizon.com> 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