Re: [PATCH v4 04/17] x86/hvm: remove multiple open coded 'chunking' loops

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

>>> On 24.06.15 at 13:24, <paul.durrant@citrix.com> wrote:
> +static int hvmemul_phys_mmio_access(
> +    paddr_t gpa, unsigned int size, uint8_t dir, uint8_t **buffer)

As much as the earlier offset you returned via indirection to the
caller was unnecessary, the indirection here seems pointless too.
All callers know how (or don't care) to update the buffer pointer.

> +static int hvmemul_linear_mmio_access(
> +    unsigned long gla, unsigned int size, uint8_t dir, uint8_t *buffer,
> +    uint32_t pfec, struct hvm_emulate_ctxt *hvmemul_ctxt, bool_t translate)
> +{
> +    struct hvm_vcpu_io *vio = &current->arch.hvm_vcpu.hvm_io;
> +    unsigned long page_off = gla & (PAGE_SIZE - 1);
> +    unsigned int chunk;
> +    paddr_t gpa;
> +    unsigned long one_rep = 1;
> +    int rc;
> +
> +    chunk = min_t(unsigned int, size, PAGE_SIZE - page_off);
> +
> +    if ( translate )
> +        gpa = pfn_to_paddr(vio->mmio_gpfn) | page_off;

"translate" as name for the parameter signaling that the translation
is known is kind of odd - "translated" or "known_gpfn" or some such?
Or invert the meaning?

> +    else
> +    {
> +        rc = hvmemul_linear_to_phys(gla, &gpa, chunk, &one_rep, pfec,
> +                                    hvmemul_ctxt);
> +        if ( rc != X86EMUL_OKAY )
> +            return rc;
> +    }
> +
> +    for ( ;; )
> +    {
> +        rc = hvmemul_phys_mmio_access(gpa, chunk, dir, &buffer);
> +        if ( rc != X86EMUL_OKAY )
> +            break;
> +
> +        gla += chunk;
> +        gpa += chunk;
> +        size -= chunk;
> +
> +        if ( size == 0 )
> +            break;
> +
> +        ASSERT((gla & (PAGE_SIZE - 1)) == 0);
> +        chunk = min_t(unsigned int, size, PAGE_SIZE);

I think you could just assert that size is now less than PAGE_SIZE.

> +        if ( !translate )
> +        {
> +            rc = hvmemul_linear_to_phys(gla, &gpa, chunk, &one_rep, pfec,
> +                                        hvmemul_ctxt);
> +            if ( rc != X86EMUL_OKAY )
> +                return rc;
> +        }

This must be done unconditionally (and gpa doesn't need updating
above then), as the known translation is only for the first byte
(and whatever falls on the same page).

Jan