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

[Peter Xu]

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,

-- 
Peter Xu