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

[Philippe Mathieu-Daudé]

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

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

> 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,