On 21/10/2017 13:24, BALATON Zoltan wrote: > When a section with non-0 offset_within_region field is tested to > cover an address the offset should be taken into account as well. > > This fixes a crash caused by picking the wrong memory region in > address_space_lookup_region seen with client code accessing a device > model that uses alias memory regions. > > Signed-off-by: BALATON Zoltan <bala...@eik.bme.hu> > --- > This seems to fix the problem described in > http://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03356.html > but I'm not completely sure about it. This seems to be introduced in > 729633c exec: Introduce AddressSpaceDispatch.mru_section and the patch > before that which split off section_covers_addr from phys_page_find so > this patch also changes that caller. Is that OK to do? It appears to > work but I don't know this part of QEMU. > > Also the bug seems to be caused by section_covers_addr accepting > sii3112.bar5 when that's the mru_section instead of picking > sii3112.bar0 (which it picks when going through phys_page_find) when > client code is accessing 0xc08001006 from this address map (full > address map is at above URL): > > address-space: memory > 0000000c08000000-0000000c0800ffff (prio 0, i/o): alias isa_mmio @io > 0000000000000000-000000000000ffff > > address-space: I/O > 0000000000000000-000000000000ffff (prio 0, i/o): io > 0000000000001000-0000000000001007 (prio 1, i/o): alias sii3112.bar0 > @sii3112.bar5 0000000000000080-0000000000000087 > 0000000000001008-000000000000100b (prio 1, i/o): alias sii3112.bar1 > @sii3112.bar5 0000000000000088-000000000000008b > 0000000000001010-0000000000001017 (prio 1, i/o): alias sii3112.bar2 > @sii3112.bar5 00000000000000c0-00000000000000c7 > 0000000000001018-000000000000101b (prio 1, i/o): alias sii3112.bar3 > @sii3112.bar5 00000000000000c8-00000000000000cb > 0000000000001020-000000000000102f (prio 1, i/o): alias sii3112.bar4 > @sii3112.bar5 0000000000000000-000000000000000f > > which this patch fixes but would the same problem happen if the > mru_section is bar5 but bar4 is accessed? I could not reproduce that > case but then the offset is 0 but in this case the address would be > above 0xc08001020 and size is 0xf so they probably won't match. But > this is only because of the size of the region. Could that mean the > bug is caused by something else and should be fixed elsewhere? > > --- > exec.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/exec.c b/exec.c > index db5ae23..a915817 100644 > --- a/exec.c > +++ b/exec.c > @@ -370,7 +370,8 @@ static inline bool section_covers_addr(const > MemoryRegionSection *section, > * the section must cover the entire address space. > */ > return int128_gethi(section->size) || > - range_covers_byte(section->offset_within_address_space, > + range_covers_byte(section->offset_within_address_space + > + section->offset_within_region, > int128_getlo(section->size), addr); > }
Sorry, this is incorrect. addr is an address in the address space, and range_covers_byte checks if it is between section->offset_within_address_space and section->offset_within_address_space + section->size. I am not sure how things don't explode completely by adding section->offset_within_region (probably it's just because section->offset_within_region is usually 0). Paolo
