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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 24 June 2015 13:34
> To: Paul Durrant
> Cc: Andrew Cooper; xen-devel@xxxxxxxxxxxxxxxxxxxx; Keir (Xen.org)
> Subject: Re: [PATCH v4 04/17] x86/hvm: remove multiple open coded
> 'chunking' loops
> 
> >>> On 24.06.15 at 13:24, <paul.durrant@xxxxxxxxxx> 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.

Ok. Personally I'd prefer one thing to be in charge of updating the pointer 
though.

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

Ok. I think I'll go with the latter.

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

True.

> 
> > +        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).
> 

Yes indeed, I somehow missed that before.

  Paul

> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel