Message ID | 20171114225941.072707456B5@zero.eik.bme.hu |
---|---|
State | New |
Headers | show |
Series | exec: Skip mru section if it's a partial page and not resolving subpage | expand |
On 14/11/2017 23:42, BALATON Zoltan wrote: > 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 <balaton@eik.bme.hu> > --- > exec.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/exec.c b/exec.c > index 97a24a8..e5f2b9a 100644 > --- a/exec.c > +++ b/exec.c > @@ -413,6 +413,7 @@ static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d, > bool update; > > if (section && section != &d->map.sections[PHYS_SECTION_UNASSIGNED] && > + (resolve_subpage || !section->offset_within_region) && > section_covers_addr(section, addr)) { > update = false; > } else { > This is another possibility: diff --git a/exec.c b/exec.c index 97a24a875e..3bb9fcf257 100644 --- a/exec.c +++ b/exec.c @@ -410,22 +410,16 @@ static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d, { MemoryRegionSection *section = atomic_read(&d->mru_section); subpage_t *subpage; - bool update; - if (section && section != &d->map.sections[PHYS_SECTION_UNASSIGNED] && - section_covers_addr(section, addr)) { - update = false; - } else { + if (!section || section == &d->map.sections[PHYS_SECTION_UNASSIGNED] || + !section_covers_addr(section, addr)) { section = phys_page_find(d, addr); - update = true; + atomic_set(&d->mru_section, section); } if (resolve_subpage && section->mr->subpage) { subpage = container_of(section->mr, subpage_t, iomem); section = &d->map.sections[subpage->sub_section[SUBPAGE_IDX(addr)]]; } - if (update) { - atomic_set(&d->mru_section, section); - } return section; } It will skip the expensive phys_page_find but not the cheap subpage lookup. Does it work for you? Paolo
On Wed, 15 Nov 2017, Paolo Bonzini wrote: > This is another possibility: > > diff --git a/exec.c b/exec.c > index 97a24a875e..3bb9fcf257 100644 > --- a/exec.c > +++ b/exec.c > @@ -410,22 +410,16 @@ static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d, > { > MemoryRegionSection *section = atomic_read(&d->mru_section); > subpage_t *subpage; > - bool update; > > - if (section && section != &d->map.sections[PHYS_SECTION_UNASSIGNED] && > - section_covers_addr(section, addr)) { > - update = false; > - } else { > + if (!section || section == &d->map.sections[PHYS_SECTION_UNASSIGNED] || > + !section_covers_addr(section, addr)) { > section = phys_page_find(d, addr); > - update = true; > + atomic_set(&d->mru_section, section); > } > if (resolve_subpage && section->mr->subpage) { > subpage = container_of(section->mr, subpage_t, iomem); > section = &d->map.sections[subpage->sub_section[SUBPAGE_IDX(addr)]]; > } > - if (update) { > - atomic_set(&d->mru_section, section); > - } > return section; > } > > > It will skip the expensive phys_page_find but not the cheap subpage lookup. > Does it work for you? This also works but I can't understand why becuase the problematic call to this function which got the wrong result from section_covers_addr was with resolve_subpage=false and in that case this looks identical to the original which did not work. Unless this function is called multiple times and another call with resolve_subpage=true now somehow works better with this change or replacing the mru_section earlier before subpage lookup makes a difference which is not obvious from this function. Anyway, it fixes the problem I see so you can use this instead of my patch. Thank you, BALATON Zoltan
On 15/11/2017 20:00, BALATON Zoltan wrote: > On Wed, 15 Nov 2017, Paolo Bonzini wrote: >> This is another possibility: >> >> diff --git a/exec.c b/exec.c >> index 97a24a875e..3bb9fcf257 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -410,22 +410,16 @@ static MemoryRegionSection >> *address_space_lookup_region(AddressSpaceDispatch *d, >> { >> MemoryRegionSection *section = atomic_read(&d->mru_section); >> subpage_t *subpage; >> - bool update; >> >> - if (section && section != >> &d->map.sections[PHYS_SECTION_UNASSIGNED] && >> - section_covers_addr(section, addr)) { >> - update = false; >> - } else { >> + if (!section || section == >> &d->map.sections[PHYS_SECTION_UNASSIGNED] || >> + !section_covers_addr(section, addr)) { >> section = phys_page_find(d, addr); >> - update = true; >> + atomic_set(&d->mru_section, section); >> } >> if (resolve_subpage && section->mr->subpage) { >> subpage = container_of(section->mr, subpage_t, iomem); >> section = >> &d->map.sections[subpage->sub_section[SUBPAGE_IDX(addr)]]; >> } >> - if (update) { >> - atomic_set(&d->mru_section, section); >> - } >> return section; >> } >> >> >> It will skip the expensive phys_page_find but not the cheap subpage >> lookup. >> Does it work for you? > > This also works but I can't understand why becuase the problematic call > to this function which got the wrong result from section_covers_addr was > with resolve_subpage=false and in that case this looks identical to the > original which did not work. The problem was that a resolve_subpage=true mru_section was reused for a resolve_subpage=false mru_section. This is fixed by always making the mru_section the same as if resolve_subpage=false (mru_section is set before looking into the subpage_t). Thanks, Paolo > Unless this function is called multiple > times and another call with resolve_subpage=true now somehow works > better with this change or replacing the mru_section earlier before > subpage lookup makes a difference which is not obvious from this function. > > Anyway, it fixes the problem I see so you can use this instead of my patch. > > Thank you, > BALATON Zoltan
diff --git a/exec.c b/exec.c index 97a24a8..e5f2b9a 100644 --- a/exec.c +++ b/exec.c @@ -413,6 +413,7 @@ static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d, bool update; if (section && section != &d->map.sections[PHYS_SECTION_UNASSIGNED] && + (resolve_subpage || !section->offset_within_region) && section_covers_addr(section, addr)) { update = false; } else {
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 <balaton@eik.bme.hu> --- exec.c | 1 + 1 file changed, 1 insertion(+)