| Message ID | 20220826160927.322797-1-afaria@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | softmmu/physmem: Fix address of FlatView access in address_space_(read|write)_cached_slow() | expand |
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. > > Fixes: 48564041a7 ("exec: reintroduce MemoryRegion caching") > Co-Developed-by: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: Alberto Faria <afaria@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, > + 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 the issue happens on vIOMMU+virtio on x86, then offset_within_region and offset_within_address_space should be all zeros for vt-d emulation since vt-d only has one huge vIOMMU window, then the outcome could be the same. However maybe there might be a difference with other vIOMMUs. Thanks,
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 <stefanha@redhat.com> > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > > Signed-off-by: Alberto Faria <afaria@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.
Hi Peter, On Sat, Aug 27, 2022 at 7:59 PM Peter Xu <peterx@redhat.com> wrote: > 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. I came across this when performing unaligned 8-byte writes on x86_64-softmmu. The write was aligned to 4 bytes but not 8, so the first 4 bytes were written just fine by address_space_translate_cached(), but flatview_write_continue() failed to write the following 4 bytes as it was translating the wrong address. I'll mention this in the commit description. Thanks, Alberto
On Mon, Sep 5, 2022 at 12:32 AM Alberto Campinho Faria <afaria@redhat.com> wrote: > Hi Peter, > > On Sat, Aug 27, 2022 at 7:59 PM Peter Xu <peterx@redhat.com> wrote: > > 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. > > I came across this when performing unaligned 8-byte writes on > x86_64-softmmu. The write was aligned to 4 bytes but not 8, so the > first 4 bytes were written just fine by > address_space_translate_cached(), but flatview_write_continue() failed > to write the following 4 bytes as it was translating the wrong > address. I'll mention this in the commit description. Oops, I meant that the first 4 bytes were written just fine by flatview_write_continue(), but since it couldn't write the 8 bytes in one go, it then called flatview_translate(), which translated the wrong address.
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, + 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); } #define ARG1_DECL MemoryRegionCache *cache
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. Fixes: 48564041a7 ("exec: reintroduce MemoryRegion caching") Co-Developed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Alberto Faria <afaria@redhat.com> --- softmmu/physmem.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)