Re: [PATCH v5 02/16] x86/hvm: remove multiple open coded 'chunking' loops

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 30/06/15 14:05, Paul Durrant wrote:
> ...in hvmemul_read/write()
>
> Add hvmemul_phys_mmio_access() and hvmemul_linear_mmio_access() functions
> to reduce code duplication.
>
> NOTE: This patch also introduces a change in 'chunking' around a page
>       boundary. Previously (for example) an 8 byte access at the last
>       byte of a page would get carried out as 8 single-byte accesses.
>       It will now be carried out as a single-byte access, followed by
>       a 4-byte access, a 2-byte access and then another single-byte
>       access.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  xen/arch/x86/hvm/emulate.c |  223 +++++++++++++++++++++++---------------------
>  1 file changed, 116 insertions(+), 107 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 8b60843..b67f5db 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -539,6 +539,117 @@ static int hvmemul_virtual_to_linear(
>      return X86EMUL_EXCEPTION;
>  }
>  
> +static int hvmemul_phys_mmio_access(
> +    paddr_t gpa, unsigned int size, uint8_t dir, uint8_t *buffer)
> +{
> +    unsigned long one_rep = 1;
> +    unsigned int chunk;
> +    int rc;
> +
> +    /* Accesses must fall within a page */

Full stop.

> +    BUG_ON((gpa & (PAGE_SIZE - 1)) + size > PAGE_SIZE);

~PAGE_MASK as opposed to (PAGE_SIZE - 1)

> +
> +    /*
> +     * hvmemul_do_io() cannot handle non-power-of-2 accesses or
> +     * accesses larger than sizeof(long), so choose the highest power
> +     * of 2 not exceeding sizeof(long) as the 'chunk' size.
> +     */
> +    chunk = 1 << (fls(size) - 1);

Depending on size, chunk can become undefined (shifting by 31 or -1) or
zero (shifting by 32).

How about

if ( size > sizeof(long) )
    chunk = sizeof(long);
else
    chunk = 1U << (fls(size) - 1);

?

> +    if ( chunk > sizeof (long) )
> +        chunk = sizeof (long);
> +
> +    for ( ;; )
> +    {
> +        rc = hvmemul_do_mmio_buffer(gpa, &one_rep, chunk, dir, 0,
> +                                    buffer);
> +        if ( rc != X86EMUL_OKAY )
> +            break;
> +
> +        /* Advance to the next chunk */

Full stop.

> +        gpa += chunk;
> +        buffer += chunk;
> +        size -= chunk;
> +
> +        if ( size == 0 )
> +            break;
> +
> +        /*
> +         * If the chunk now exceeds the remaining size, choose the next
> +         * lowest power of 2 that will fit.
> +         */
> +        while ( chunk > size )
> +            chunk >>= 1;
> +    }
> +
> +    return rc;
> +}
> +
> +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 known_gpfn)
> +{
> +    struct hvm_vcpu_io *vio = &current->arch.hvm_vcpu.hvm_io;
> +    unsigned long page_off = gla & (PAGE_SIZE - 1);

Can be int as opposed to long, and "offset" appears to be the prevailing
name.  Also, ~PAGE_MASK.

> +    unsigned int chunk;
> +    paddr_t gpa;
> +    unsigned long one_rep = 1;
> +    int rc;
> +
> +    chunk = min_t(unsigned int, size, PAGE_SIZE - page_off);
> +
> +    if ( known_gpfn )
> +        gpa = pfn_to_paddr(vio->mmio_gpfn) | page_off;
> +    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;
> +        buffer += chunk;
> +        size -= chunk;
> +
> +        if ( size == 0 )
> +            break;
> +
> +        ASSERT((gla & (PAGE_SIZE - 1)) == 0);

~PAGE_MASK.

> +        ASSERT(size < PAGE_SIZE);

Nothing I can see here prevents size being greater than PAGE_SIZE. 
chunk strictly will be, but size -= chunk can still leave size greater
than a page.

~Andrew

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