Re: [Xen-devel] [PATCH v6 09/16] x86/hvm: limit reps to avoid the need to handle retry

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 09 July 2015 11:05
> To: Paul Durrant
> Cc: Andrew Cooper; xen-devel@xxxxxxxxxxxxxxxxxxxx; Keir (Xen.org)
> Subject: Re: [PATCH v6 09/16] x86/hvm: limit reps to avoid the need to
> handle retry
> 
> >>> On 03.07.15 at 18:25, <paul.durrant@xxxxxxxxxx> wrote:
> > @@ -287,17 +271,56 @@ 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);
> > +    unsigned int page_off = ram_gpa & (PAGE_SIZE - 1);
> > +    struct page_info *ram_page[2];
> > +    int nr_pages = 0;
> 
> unsigned int
> 

No, it's intentionally signed because the unwind code at the end of the 
function is:

    while ( --nr_pages >= 0 )
        hvmemul_release_page(ram_page[nr_pages]);

I.e. the loop terminates when nr_pages gets to -1.

> > +    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 */
> 
> "Determine" and missing stop.
> 
> > +    count = min_t(unsigned long,
> > +                  *reps,
> > +                  df ?
> > +                  (page_off + size - 1) / size :
> 
> Looks like this won't be correct for the page crossing case (where
> the code below really expects to see zero). I think you simply need
> to mask with (PAGE_SIZE - 1) again here before dividing.

Yep.

> 
> > @@ -468,6 +469,9 @@ void hvm_do_resume(struct vcpu *v)
> >          }
> >      }
> >
> > +    if ( vio->mmio_retry )
> > +        (void)handle_mmio();
> 
> Pointless cast.
> 

Ok.

  Paul

> Jan


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