Re: [PATCH v7 01/15] x86/hvm: remove multiple open coded 'chunking' loops

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 09 July 2015 17:24
> To: Paul Durrant
> Cc: xen-devel@lists.xen.org; Keir (Xen.org)
> Subject: RE: [PATCH v7 01/15] x86/hvm: remove multiple open coded
> 'chunking' loops
> 
> >>> On 09.07.15 at 18:16, <Paul.Durrant@citrix.com> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 09 July 2015 16:13
> >> To: Paul Durrant
> >> Cc: xen-devel@lists.xen.org; Keir (Xen.org)
> >> Subject: Re: [PATCH v7 01/15] x86/hvm: remove multiple open coded
> >> 'chunking' loops
> >>
> >> >>> On 09.07.15 at 15:10, <paul.durrant@citrix.com> wrote:
> >> > +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 offset = gla & ~PAGE_MASK;
> >> > +    unsigned int chunk;
> >> > +    paddr_t gpa;
> >> > +    unsigned long one_rep = 1;
> >> > +    int rc;
> >> > +
> >> > +    chunk = min_t(unsigned int, size, PAGE_SIZE - offset);
> >> > +
> >> > +    if ( known_gpfn )
> >> > +        gpa = pfn_to_paddr(vio->mmio_gpfn) | offset;
> >> > +    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_MASK) == 0);
> >>
> >> While I don't mean you to re-submit another time, I'd still like to
> >> get my question answered: Does this really matter for the code
> >> below?
> >
> > No, it doesn't, but if it's not true then an incorrect chunk size was chosen
> > above.
> 
> I suspected as much. It's then just slightly less odd than having
> 
>     x = 0;
>     ASSERT(x == 0);
> 

Well, not quite since the lines aren't adjacent in this case.

> I guess I'll strip the ASSERT() when applying.
> 

Ok. That's fine

  Paul

> Jan