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

[Peter Xu]

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.

-- 
Peter Xu