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

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

[Alberto Faria]

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.

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);
 }
 
 #define ARG1_DECL                MemoryRegionCache *cache
-- 
2.37.2

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

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

[Philippe Mathieu-Daudé via]

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

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

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

[Philippe Mathieu-Daudé via]

On 30/8/22 18:18, Peter Xu wrote:
> 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.

Oh, I missed that call.

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

Ah yes, a helper is clever.

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

[Alberto Campinho Faria]

Hi Peter,

On Sat, Aug 27, 2022 at 7:59 PM Peter Xu <peterx@redhat.com> wrote:
> 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.

I came across this when performing unaligned 8-byte writes on
x86_64-softmmu. The write was aligned to 4 bytes but not 8, so the
first 4 bytes were written just fine by
address_space_translate_cached(), but flatview_write_continue() failed
to write the following 4 bytes as it was translating the wrong
address. I'll mention this in the commit description.

Thanks,
Alberto

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

[Alberto Campinho Faria]

On Mon, Sep 5, 2022 at 12:32 AM Alberto Campinho Faria
<afaria@redhat.com> wrote:
> Hi Peter,
>
> On Sat, Aug 27, 2022 at 7:59 PM Peter Xu <peterx@redhat.com> wrote:
> > 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.
>
> I came across this when performing unaligned 8-byte writes on
> x86_64-softmmu. The write was aligned to 4 bytes but not 8, so the
> first 4 bytes were written just fine by
> address_space_translate_cached(), but flatview_write_continue() failed
> to write the following 4 bytes as it was translating the wrong
> address. I'll mention this in the commit description.

Oops, I meant that the first 4 bytes were written just fine by
flatview_write_continue(), but since it couldn't write the 8 bytes in
one go, it then called flatview_translate(), which translated the
wrong address.