On Tue, Aug 30, 2022 at 02:06:32PM +0200, Philippe Mathieu-Daudé wrote: > On 27/8/22 20:59, Peter Xu wrote: > > Hi, Alberto, > > > > On Fri, Aug 26, 2022 at 05:09:27PM +0100, Alberto Faria wrote: > > > Apply cache->xlat to addr before passing it to > > > flatview_(read|write)_continue(), to convert it from the > > > MemoryRegionCache's address space to the FlatView's. > > > > Any bug encountered? It'll be great to add more information into the > > commit message if there is. We could also mention the issue was observed > > by code review or so. > > Agreed. > > > > > > > Fixes: 48564041a7 ("exec: reintroduce MemoryRegion caching") > > > Co-Developed-by: Stefan Hajnoczi <stefa...@redhat.com> > > > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com> > > > Signed-off-by: Alberto Faria <afa...@redhat.com> > > > --- > > > softmmu/physmem.c | 12 ++++++------ > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > diff --git a/softmmu/physmem.c b/softmmu/physmem.c > > > index dc3c3e5f2e..95d4c77cc3 100644 > > > --- a/softmmu/physmem.c > > > +++ b/softmmu/physmem.c > > > @@ -3450,9 +3450,9 @@ address_space_read_cached_slow(MemoryRegionCache > > > *cache, hwaddr addr, > > > l = len; > > > mr = address_space_translate_cached(cache, addr, &addr1, &l, false, > > > MEMTXATTRS_UNSPECIFIED); > > > - return flatview_read_continue(cache->fv, > > > - addr, MEMTXATTRS_UNSPECIFIED, buf, len, > > > - addr1, l, mr); > > > + return flatview_read_continue(cache->fv, cache->xlat + addr, > > So this is just addr1...
It may not; note that in address_space_translate_cached() the xlat pointer will also be passed over to address_space_translate_iommu(), so it can be further modified to point to the real MR address behind the IOMMU. > > > > + MEMTXATTRS_UNSPECIFIED, buf, len, > > > addr1, l, > > > + mr); > > > } > > > /* Called from RCU critical section. address_space_write_cached uses > > > this > > > @@ -3468,9 +3468,9 @@ address_space_write_cached_slow(MemoryRegionCache > > > *cache, hwaddr addr, > > > l = len; > > > mr = address_space_translate_cached(cache, addr, &addr1, &l, true, > > > MEMTXATTRS_UNSPECIFIED); > > > - return flatview_write_continue(cache->fv, > > > - addr, MEMTXATTRS_UNSPECIFIED, buf, > > > len, > > > - addr1, l, mr); > > > + return flatview_write_continue(cache->fv, cache->xlat + addr, > > > + MEMTXATTRS_UNSPECIFIED, buf, len, > > > addr1, l, > > > + mr); > > > } > > > > The issue looks correct, but I hesitate on the fix.. since afaict > > cache->xlat is per memory region not flat view, so the new calculation is > > offset within memory region but not flat view too. > > > > So I'm wondering whether it should become: > > > > cache->xlat + addr - cache.mrs.offset_within_region + > > cache.mrs.offset_within_address_space > > If so, shouldn't this be calculated [*] within > address_space_translate_cached() instead of the caller? > > [*] Maybe passed as another pointer to hwaddr No strong opinion, but that looks like a duplication of information we have. Maybe we can also have a helper to convert the cache address space to global address space. -- Peter Xu