[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] exec: Fix MAP_RAM for cached access
From: |
Peter Xu |
Subject: |
Re: [Qemu-devel] [PATCH] exec: Fix MAP_RAM for cached access |
Date: |
Wed, 13 Jun 2018 14:53:28 +0800 |
User-agent: |
Mutt/1.9.5 (2018-04-13) |
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