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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 24 June 2015 16:21
> To: Paul Durrant
> Cc: Andrew Cooper; xen-devel@xxxxxxxxxxxxxxxxxxxx; Keir (Xen.org)
> Subject: Re: [PATCH v4 10/17] x86/hvm: revert 82ed8716b "fix direct PCI port
> I/O emulation retry...
> 
> >>> On 24.06.15 at 13:24, <paul.durrant@xxxxxxxxxx> 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.

Ok. I can change the terminology in the message.

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

True. It's a bit lazy doing it this way. I'll change it.

  Paul

> Jan


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