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 14:38
> 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 09.07.15 at 14:50, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 09 July 2015 13:04
> >> >>> On 09.07.15 at 13:11, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> >> Sent: 09 July 2015 11:05
> >> >> >>> 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.
> >>
> >>      while ( nr_pages-- )
> >>          hvmemul_release_page(ram_page[nr_pages]);
> >>
> >
> > But that wouldn't be correct, since the loop would try to release
> > ram_page[1] and ram_page[0] in the case where only ram_page[0] had
> been
> > acquired. It would have to be:
> >
> >       while ( nr_pages )
> >           hvmemul_release_page(ram_page[--nr_pages]);
> 
> Unless you need the value of nr_pages after the loop, I can't see
> the difference between your and my proposal - the decrement in
> both cases happens between determining whether to continue the
> loop and accessing the intended array element.

D'oh. Of course it does.

> 
> > which I'll do, since you so dislike signed ints.
> 
> Sounds like you're not understanding why. Just look at the
> generated code when using signed vs unsigned int as array
> index.

Ok, I'll take a look because I would not have thought it would make a huge 
difference.

  Paul

> 
> Jan


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