Re: [PATCH v4 10/17] x86/hvm: revert 82ed8716b "fix direct PCI port I/O emulation retry...

["Jan Beulich" <JBeulich@suse.com>]

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