From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Jan Beulich" <JBeulich@suse.com>
Subject: Re: [PATCH v4 10/17] x86/hvm: revert 82ed8716b "fix
 direct PCI port I/O emulation retry...
Date: Wed, 24 Jun 2015 16:21:15 +0100
Message-ID: <558AE70C0200007800089075@mail.emea.novell.com>
References: <1435145089-21999-1-git-send-email-paul.durrant@citrix.com>
	<1435145089-21999-11-git-send-email-paul.durrant@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta3.messagelabs.com ([195.245.230.39])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <JBeulich@suse.com>) id 1Z7mUL-0006Zo-9R
	for xen-devel@lists.xenproject.org; Wed, 24 Jun 2015 15:21:21 +0000
In-Reply-To: <1435145089-21999-11-git-send-email-paul.durrant@citrix.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>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>, Keir Fraser <keir@xen.org>, xen-devel@lists.xenproject.org
List-Id: xen-devel@lists.xenproject.org

>>> On 24.06.15 at 13:24, <paul.durrant@citrix.com> wrote:
> @@ -288,17 +272,66 @@ static int hvmemul_do_io_addr(
>      bool_t is_mmio, paddr_t addr, unsigned long *reps,
>      unsigned int size, uint8_t dir, bool_t df, paddr_t ram_gpa)
>  {
> -    struct page_info *ram_page;
> +    struct vcpu *v = current;
> +    unsigned long ram_gmfn = paddr_to_pfn(ram_gpa);
> +    struct page_info *ram_page[2];
> +    int nr_pages = 0;
> +    unsigned long count;
>      int rc;
>  
> -    rc = hvmemul_acquire_page(paddr_to_pfn(ram_gpa), &ram_page);
> +    rc = hvmemul_acquire_page(ram_gmfn, &ram_page[nr_pages]);
>      if ( rc != X86EMUL_OKAY )
> -        return rc;
> +        goto out;
>  
> -    rc = hvmemul_do_io(is_mmio, addr, reps, size, dir, df, 1,
> +    nr_pages++;
> +
> +    /* Detemine how many reps will fit within this page */
> +    for ( count = 0; count < *reps; count++ )
> +    {
> +        paddr_t start, end;
> +
> +        if ( df )
> +        {
> +            start = ram_gpa - count * size;
> +            end = ram_gpa + size - 1;
> +        }
> +        else
> +        {
> +            start = ram_gpa;
> +            end = ram_gpa + (count + 1) * size - 1;
> +        }
> +
> +        if ( paddr_to_pfn(start) != ram_gmfn ||
> +             paddr_to_pfn(end) != ram_gmfn )
> +            break;
> +    }
> +
> +    if ( count == 0 )
> +    {
> +        /*
> +         * This access must span two pages, so grab a reference to
> +         * the next page and do a single rep.
> +         */
> +        rc = hvmemul_acquire_page(df ? ram_gmfn - 1 : ram_gmfn + 1,
> +                                  &ram_page[nr_pages]);
> +        if ( rc != X86EMUL_OKAY )
> +            goto out;
> +
> +        nr_pages++;
> +        count = 1;
> +    }
> +
> +    rc = hvmemul_do_io(is_mmio, addr, count, size, dir, df, 1,
>                         ram_gpa);

Looking at this change alone I think calling this a revert is pretty odd.
Yes, you undo some of the original commit, but it looks like about 50%
of the patch are doing things other than reverting. Mentioning the
original commit in the description is certainly fine, but beyond that it
should be an ordinary patch.

As to the code above - do you really think determining "count" in a
loop is efficient? It ought to be possible to obtain this via simple
calculation...

Jan