Re: [Qemu-devel] [PATCH] exec-obsolete: fix length handling

[Avi Kivity]

On 01/29/2012 03:39 PM, Blue Swirl wrote:
> >> >> +++ b/exec-obsolete.h
> >> >> @@ -81,11 +81,10 @@ static inline void
> >> >> cpu_physical_memory_set_dirty_range(ram_addr_t start,
> >> >>                                                         int dirty_flags)
> >> >>  {
> >> >>      uint8_t *p;
> >> >> -    ram_addr_t addr, end;
> >> >> +    ram_addr_t cur;
> >> >>
> >> >> -    end = start + length;
> >> >>      p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS);
> >> >> -    for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) {
> >> >> +    for (cur = 0; cur < length; cur += TARGET_PAGE_SIZE) {
> >> >>          *p++ |= dirty_flags;
> >> >>      }
> >> >
> >> > I think this is still wrong - if length == 2 it will iterate once, but
> >> > we need two iterations if start == 0xfff.
> >>
> >> Yes, tricky. We could do something like
> >> for (cur = start & TARGET_PAGE_MASK; cur < length; cur += 
> >> TARGET_PAGE_SIZE) {
> >> but I'll send a new patch with just s/<=/</.
> >
> > That's broken too.
>
> Because length should be adjusted by -1?

0xfff/2 breaks.

More generally, you can't have a loop that just looks at length, since
0/2 wants one iteration, and 0xfff/2 wants two.

>
> > I have:
> >
> >     uint8_t *p;
> >     ram_addr_t addr, end;
> >
> > -    end = start + length;
> > +    end = (start + length - 1) | (TARGET_PAGE_SIZE - 1);
>
> Why  | (TARGET_PAGE_SIZE - 1), for length == 1? 

Yes.  More generally, I wanted something that is easily understood -
start/end addresses without trickery, given all the broken patches for
fixing this.

> TARGET_PAGE_ALIGN()
> could be useful here.

True, I'll respin.

-- 
error compiling committee.c: too many arguments to function