Re: [PATCH] softmmu/physmem: Fix address of FlatView access in address_space_(read|write)_cached_slow()

[Philippe Mathieu-Daudé]

On 30/8/22 18:18, Peter Xu wrote:
> 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.

Oh, I missed that call.

> > > > +                                  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.

Ah yes, a helper is clever.