Re: [Qemu-devel] [PATCH] exec: Fix MAP_RAM for cached access

[Peter Xu]

On Wed, Jun 13, 2018 at 08:31:31AM +0200, Auger Eric wrote:
> Hi Peter,
> 
> On 06/13/2018 05:15 AM, Peter Xu wrote:
> > On Tue, Jun 12, 2018 at 09:05:25PM +0200, Eric Auger wrote:
> >> When an IOMMUMemoryRegion is in front of a virtio device,
> >> address_space_cache_init does not set cache->ptr as the memory
> >> region is not RAM. However when the device performs an access,
> >> we end up in glue() which performs the translation and then uses
> >> MAP_RAM. This latter uses the unset ptr and returns a wrong value
> >> which leads to a SIGSEV in address_space_lduw_internal_cached_slow,
> >> for instance. Let's test whether the cache->ptr is set, and in
> >> the negative use the old macro definition. This fixes the
> >> use cases featuring vIOMMU (Intel and ARM SMMU) which lead to
> >> a SIGSEV.
> >>
> >> Fixes: 48564041a73a (exec: reintroduce MemoryRegion caching)
> >> Signed-off-by: Eric Auger <address@hidden>
> >>
> >> ---
> >>
> >> I am not sure whether it doesn't break any targeted optimization
> >> but at least it removes the SIGSEV.
> >>
> >> Signed-off-by: Eric Auger <address@hidden>
> >> ---
> >>  exec.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/exec.c b/exec.c
> >> index f6645ed..46fbd25 100644
> >> --- a/exec.c
> >> +++ b/exec.c
> >> @@ -3800,7 +3800,9 @@ address_space_write_cached_slow(MemoryRegionCache 
> >> *cache, hwaddr addr,
> >>  #define SUFFIX                   _cached_slow
> >>  #define TRANSLATE(...)           address_space_translate_cached(cache, 
> >> __VA_ARGS__)
> >>  #define IS_DIRECT(mr, is_write)  memory_access_is_direct(mr, is_write)
> >> -#define MAP_RAM(mr, ofs)         (cache->ptr + (ofs - cache->xlat))
> >> +#define MAP_RAM(mr, ofs)         (cache->ptr ? \
> >> +                                 (cache->ptr + (ofs - cache->xlat)) :  \
> >> +                                 qemu_map_ram_ptr((mr)->ram_block, ofs))
> > 
> > A pure question: if the MR is not a RAM (I think the only case for
> > virtio case should be an IOMMU MR), then why we'll call MAP_RAM()
> > after all?  An glue() example:
> > 
> > void glue(address_space_stb, SUFFIX)(ARG1_DECL,
> >     hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result)
> > {
> >     uint8_t *ptr;
> >     MemoryRegion *mr;
> >     hwaddr l = 1;
> >     hwaddr addr1;
> >     MemTxResult r;
> >     bool release_lock = false;
> > 
> >     RCU_READ_LOCK();
> >     mr = TRANSLATE(addr, &addr1, &l, true, attrs);
> >     if (!IS_DIRECT(mr, true)) {                <----------------- [1]
> after the translate, mr points to the actual RAM region, downstream to
> the IOMMU MR. And this one is direct. addr1 is the offset within the RAM
> region if I am not wrong.
> 
> Am i missing something?

I think you are right. Then the change seems reasonable to me.

Thanks,

-- 
Peter Xu