[PATCH] softmmu/memory: use memcpy for multi-byte accesses

[PATCH] softmmu/memory: use memcpy for multi-byte accesses

[Patrick Venture]

Avoids unaligned pointer issues.

Reviewed-by: Chris Rauer <crauer@google.com>
Reviewed-by: Peter Foley <pefoley@google.com>
Signed-off-by: Patrick Venture <venture@google.com>
---
 system/memory.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/system/memory.c b/system/memory.c
index 304fa843ea..02c97d5187 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1343,16 +1343,16 @@ static uint64_t memory_region_ram_device_read(void *opaque,
 
     switch (size) {
     case 1:
-        data = *(uint8_t *)(mr->ram_block->host + addr);
+        memcpy(&data, mr->ram_block->host + addr, sizeof(uint8_t));
         break;
     case 2:
-        data = *(uint16_t *)(mr->ram_block->host + addr);
+        memcpy(&data, mr->ram_block->host + addr, sizeof(uint16_t));
         break;
     case 4:
-        data = *(uint32_t *)(mr->ram_block->host + addr);
+        memcpy(&data, mr->ram_block->host + addr, sizeof(uint32_t));
         break;
     case 8:
-        data = *(uint64_t *)(mr->ram_block->host + addr);
+        memcpy(&data, mr->ram_block->host + addr, sizeof(uint64_t));
         break;
     }
 
@@ -1370,16 +1370,16 @@ static void memory_region_ram_device_write(void *opaque, hwaddr addr,
 
     switch (size) {
     case 1:
-        *(uint8_t *)(mr->ram_block->host + addr) = (uint8_t)data;
+        memcpy(mr->ram_block->host + addr, &data, sizeof(uint8_t));
         break;
     case 2:
-        *(uint16_t *)(mr->ram_block->host + addr) = (uint16_t)data;
+        memcpy(mr->ram_block->host + addr, &data, sizeof(uint16_t));
         break;
     case 4:
-        *(uint32_t *)(mr->ram_block->host + addr) = (uint32_t)data;
+        memcpy(mr->ram_block->host + addr, &data, sizeof(uint32_t));
         break;
     case 8:
-        *(uint64_t *)(mr->ram_block->host + addr) = data;
+        memcpy(mr->ram_block->host + addr, &data, sizeof(uint64_t));
         break;
     }
 }
-- 
2.43.0.rc0.421.g78406f8d94-goog

Re: [PATCH] softmmu/memory: use memcpy for multi-byte accesses

[Richard Henderson]

On 11/14/23 12:55, Patrick Venture wrote:
> Avoids unaligned pointer issues.
> 
> Reviewed-by: Chris Rauer <crauer@google.com>
> Reviewed-by: Peter Foley <pefoley@google.com>
> Signed-off-by: Patrick Venture <venture@google.com>
> ---
>   system/memory.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/system/memory.c b/system/memory.c
> index 304fa843ea..02c97d5187 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -1343,16 +1343,16 @@ static uint64_t memory_region_ram_device_read(void *opaque,
>   
>       switch (size) {
>       case 1:
> -        data = *(uint8_t *)(mr->ram_block->host + addr);
> +        memcpy(&data, mr->ram_block->host + addr, sizeof(uint8_t));


This is incorrect, especially for big-endian hosts.

You want to use "qemu/bswap.h", ld*_he_p(), st*_he_p().


r~

Re: [PATCH] softmmu/memory: use memcpy for multi-byte accesses

[Patrick Venture]

[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]

On Tue, Nov 14, 2023 at 1:18 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 11/14/23 12:55, Patrick Venture wrote:
> > Avoids unaligned pointer issues.
> >
> > Reviewed-by: Chris Rauer <crauer@google.com>
> > Reviewed-by: Peter Foley <pefoley@google.com>
> > Signed-off-by: Patrick Venture <venture@google.com>
> > ---
> >   system/memory.c | 16 ++++++++--------
> >   1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/system/memory.c b/system/memory.c
> > index 304fa843ea..02c97d5187 100644
> > --- a/system/memory.c
> > +++ b/system/memory.c
> > @@ -1343,16 +1343,16 @@ static uint64_t
> memory_region_ram_device_read(void *opaque,
> >
> >       switch (size) {
> >       case 1:
> > -        data = *(uint8_t *)(mr->ram_block->host + addr);
> > +        memcpy(&data, mr->ram_block->host + addr, sizeof(uint8_t));
>
>
> This is incorrect, especially for big-endian hosts.
>
> You want to use "qemu/bswap.h", ld*_he_p(), st*_he_p().
>

Thanks, I'll take a look.


>
>
> r~
>

[-- Attachment #2: Type: text/html, Size: 1883 bytes --]

Re: [PATCH] softmmu/memory: use memcpy for multi-byte accesses

[Peter Maydell]

On Tue, 14 Nov 2023 at 21:18, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 11/14/23 12:55, Patrick Venture wrote:
> > Avoids unaligned pointer issues.
> >
> > Reviewed-by: Chris Rauer <crauer@google.com>
> > Reviewed-by: Peter Foley <pefoley@google.com>
> > Signed-off-by: Patrick Venture <venture@google.com>
> > ---
> >   system/memory.c | 16 ++++++++--------
> >   1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/system/memory.c b/system/memory.c
> > index 304fa843ea..02c97d5187 100644
> > --- a/system/memory.c
> > +++ b/system/memory.c
> > @@ -1343,16 +1343,16 @@ static uint64_t memory_region_ram_device_read(void *opaque,
> >
> >       switch (size) {
> >       case 1:
> > -        data = *(uint8_t *)(mr->ram_block->host + addr);
> > +        memcpy(&data, mr->ram_block->host + addr, sizeof(uint8_t));
>
>
> This is incorrect, especially for big-endian hosts.
>
> You want to use "qemu/bswap.h", ld*_he_p(), st*_he_p().

More specifically, we have a ldn_he_p() and stn_he_p() that
take the size in bytes of the data to read, so we should be
able to replace the switch-on-size in these functions with
a single call to the appropriate one of those.

thanks
-- PMM

Re: [PATCH] softmmu/memory: use memcpy for multi-byte accesses

[Peter Maydell]

On Tue, 14 Nov 2023 at 20:55, Patrick Venture <venture@google.com> wrote:
> Avoids unaligned pointer issues.
>

It would be nice to be more specific in the commit message here, by
describing what kind of guest behaviour or machine config runs into this
problem, and whether this happens in a situation users are likely to
run into. If the latter, we should consider tagging the commit
with "Cc: qemu-stable@nongnu.org" to have it backported to the
stable release branches.

thanks
-- PMM

Re: [PATCH] softmmu/memory: use memcpy for multi-byte accesses

[Patrick Venture]

[-- Attachment #1: Type: text/plain, Size: 1428 bytes --]

On Wed, Nov 15, 2023 at 2:30 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Tue, 14 Nov 2023 at 21:18, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> >
> > On 11/14/23 12:55, Patrick Venture wrote:
> > > Avoids unaligned pointer issues.
> > >
> > > Reviewed-by: Chris Rauer <crauer@google.com>
> > > Reviewed-by: Peter Foley <pefoley@google.com>
> > > Signed-off-by: Patrick Venture <venture@google.com>
> > > ---
> > >   system/memory.c | 16 ++++++++--------
> > >   1 file changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/system/memory.c b/system/memory.c
> > > index 304fa843ea..02c97d5187 100644
> > > --- a/system/memory.c
> > > +++ b/system/memory.c
> > > @@ -1343,16 +1343,16 @@ static uint64_t
> memory_region_ram_device_read(void *opaque,
> > >
> > >       switch (size) {
> > >       case 1:
> > > -        data = *(uint8_t *)(mr->ram_block->host + addr);
> > > +        memcpy(&data, mr->ram_block->host + addr, sizeof(uint8_t));
> >
> >
> > This is incorrect, especially for big-endian hosts.
> >
> > You want to use "qemu/bswap.h", ld*_he_p(), st*_he_p().
>
> More specifically, we have a ldn_he_p() and stn_he_p() that
> take the size in bytes of the data to read, so we should be
> able to replace the switch-on-size in these functions with
> a single call to the appropriate one of those.
>

Thanks!


>
> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 2404 bytes --]

Re: [PATCH] softmmu/memory: use memcpy for multi-byte accesses

[Patrick Venture]

[-- Attachment #1: Type: text/plain, Size: 835 bytes --]

On Wed, Nov 15, 2023 at 2:35 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Tue, 14 Nov 2023 at 20:55, Patrick Venture <venture@google.com> wrote:
> > Avoids unaligned pointer issues.
> >
>
> It would be nice to be more specific in the commit message here, by
> describing what kind of guest behaviour or machine config runs into this
> problem, and whether this happens in a situation users are likely to
> run into. If the latter, we should consider tagging the commit
> with "Cc: qemu-stable@nongnu.org" to have it backported to the
> stable release branches.
>

Thanks! I'll update the commit message with v2.  We were seeing this in our
infrastructure with unaligned accesses using the pointer dereference as
there are no guarantees on alignment of the incoming values.


>
> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 1472 bytes --]

Re: [PATCH] softmmu/memory: use memcpy for multi-byte accesses

[Richard Henderson]

On 11/15/23 08:58, Patrick Venture wrote:
> 
> 
> On Wed, Nov 15, 2023 at 2:35 AM Peter Maydell <peter.maydell@linaro.org 
> <mailto:peter.maydell@linaro.org>> wrote:
> 
>     On Tue, 14 Nov 2023 at 20:55, Patrick Venture <venture@google.com
>     <mailto:venture@google.com>> wrote:
>      > Avoids unaligned pointer issues.
>      >
> 
>     It would be nice to be more specific in the commit message here, by
>     describing what kind of guest behaviour or machine config runs into this
>     problem, and whether this happens in a situation users are likely to
>     run into. If the latter, we should consider tagging the commit
>     with "Cc: qemu-stable@nongnu.org <mailto:qemu-stable@nongnu.org>" to have it
>     backported to the
>     stable release branches.
> 
> 
> Thanks! I'll update the commit message with v2.  We were seeing this in our 
> infrastructure with unaligned accesses using the pointer dereference as there are no 
> guarantees on alignment of the incoming values.

Which host cpu, for reference?  There aren't many that generate unaligned traps these days...


r~

Re: [PATCH] softmmu/memory: use memcpy for multi-byte accesses

[Patrick Venture]

[-- Attachment #1: Type: text/plain, Size: 3921 bytes --]

On Wed, Nov 15, 2023 at 9:02 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 11/15/23 08:58, Patrick Venture wrote:
> >
> >
> > On Wed, Nov 15, 2023 at 2:35 AM Peter Maydell <peter.maydell@linaro.org
> > <mailto:peter.maydell@linaro.org>> wrote:
> >
> >     On Tue, 14 Nov 2023 at 20:55, Patrick Venture <venture@google.com
> >     <mailto:venture@google.com>> wrote:
> >      > Avoids unaligned pointer issues.
> >      >
> >
> >     It would be nice to be more specific in the commit message here, by
> >     describing what kind of guest behaviour or machine config runs into
> this
> >     problem, and whether this happens in a situation users are likely to
> >     run into. If the latter, we should consider tagging the commit
> >     with "Cc: qemu-stable@nongnu.org <mailto:qemu-stable@nongnu.org>"
> to have it
> >     backported to the
> >     stable release branches.
> >
> >
> > Thanks! I'll update the commit message with v2.  We were seeing this in
> our
> > infrastructure with unaligned accesses using the pointer dereference as
> there are no
> > guarantees on alignment of the incoming values.
>
> Which host cpu, for reference?  There aren't many that generate unaligned
> traps these days...
>
>
Here's the sanitizer log/qemu log, the host-cpu was an amd64.

qemu-kvm-system-x86_64: warning: host doesn't support requested feature:
CPUID.01H:ECX.pcid [bit 17]
qemu-kvm-system-x86_64: warning: host doesn't support requested feature:
CPUID.07H:EBX.erms [bit 9]
qemu-kvm-system-x86_64: warning: host doesn't support requested feature:
CPUID.07H:EBX.invpcid [bit 10]
qemu-kvm-system-x86_64: warning: host doesn't support requested feature:
CPUID.01H:ECX.pcid [bit 17]
qemu-kvm-system-x86_64: warning: host doesn't support requested feature:
CPUID.07H:EBX.erms [bit 9]
qemu-kvm-system-x86_64: warning: host doesn't support requested feature:
CPUID.07H:EBX.invpcid [bit 10]
third_party/qemu/softmmu/memory.c:1341:16: runtime error: load of
misaligned address 0x52500020b10d for type 'uint32_t' (aka 'unsigned int'),
which requires 4 byte alignment
0x52500020b10d: note: pointer points here
 ab ab ab ab ab ab ab  ab ab ab ab ab ab ab ab  ab ab ab ab ab ab ab ab  ab
ab ab ab ab ab ab ab  ab
             ^
    #0 0x55b34f8ef9d8 in memory_region_ram_device_read
third_party/qemu/softmmu/memory.c:1341:16
    #1 0x55b34f8ee8a8 in memory_region_read_accessor
third_party/qemu/softmmu/memory.c:441:11
    #2 0x55b34f8e06db in access_with_adjusted_size
third_party/qemu/softmmu/memory.c:569:18
    #3 0x55b34f8dfcb4 in memory_region_dispatch_read1
third_party/qemu/softmmu/memory.c
    #4 0x55b34f8dfcb4 in memory_region_dispatch_read
third_party/qemu/softmmu/memory.c:1476:9
    #5 0x55b34f8fa8b0 in flatview_read_continue
third_party/qemu/softmmu/physmem.c:2744:23
    #6 0x55b34f8fb0db in flatview_read
third_party/qemu/softmmu/physmem.c:2786:12
    #7 0x55b34f8faefa in address_space_read_full
third_party/qemu/softmmu/physmem.c:2799:18
    #8 0x55b34f8fb5b4 in address_space_rw
third_party/qemu/softmmu/physmem.c:2827:16
    #9 0x55b34f71eab5 in kvm_cpu_exec
third_party/qemu/accel/kvm/kvm-all.c:3062:13
    #10 0x55b34f7172e3 in kvm_vcpu_thread_fn
third_party/qemu/accel/kvm/kvm-accel-ops.c:51:17
    #11 0x55b350467044 in qemu_thread_start
third_party/qemu/util/qemu-thread-posix.c:541:9
    #12 0x55b34f6dba10 in asan_thread_start(void*)
third_party/llvm/llvm-project/compiler-rt/lib/asan/asan_interceptors.cpp:234:28
    #13 0x7f5e1c81a7d8 in start_thread
(/usr/grte/v5/lib64/libpthread.so.0+0xb7d8) (BuildId:
3ccc1600b9140e48da03ed16e0210354)
    #14 0x7f5e1c77169e in clone (/usr/grte/v5/lib64/libc.so.6+0x13969e)
(BuildId: 280088eab084c30a3992a9bce5c35b44)

SUMMARY: UndefinedBehaviorSanitizer: misaligned-pointer-use
third_party/qemu/softmmu/memory.c:1341:16 in




>
> r~
>
>

[-- Attachment #2: Type: text/html, Size: 5128 bytes --]

Re: [PATCH] softmmu/memory: use memcpy for multi-byte accesses

[Patrick Venture]

[-- Attachment #1: Type: text/plain, Size: 4180 bytes --]

On Wed, Nov 15, 2023 at 9:26 AM Patrick Venture <venture@google.com> wrote:

>
>
> On Wed, Nov 15, 2023 at 9:02 AM Richard Henderson <
> richard.henderson@linaro.org> wrote:
>
>> On 11/15/23 08:58, Patrick Venture wrote:
>> >
>> >
>> > On Wed, Nov 15, 2023 at 2:35 AM Peter Maydell <peter.maydell@linaro.org
>> > <mailto:peter.maydell@linaro.org>> wrote:
>> >
>> >     On Tue, 14 Nov 2023 at 20:55, Patrick Venture <venture@google.com
>> >     <mailto:venture@google.com>> wrote:
>> >      > Avoids unaligned pointer issues.
>> >      >
>> >
>> >     It would be nice to be more specific in the commit message here, by
>> >     describing what kind of guest behaviour or machine config runs into
>> this
>> >     problem, and whether this happens in a situation users are likely to
>> >     run into. If the latter, we should consider tagging the commit
>> >     with "Cc: qemu-stable@nongnu.org <mailto:qemu-stable@nongnu.org>"
>> to have it
>> >     backported to the
>> >     stable release branches.
>> >
>> >
>> > Thanks! I'll update the commit message with v2.  We were seeing this in
>> our
>> > infrastructure with unaligned accesses using the pointer dereference as
>> there are no
>> > guarantees on alignment of the incoming values.
>>
>> Which host cpu, for reference?  There aren't many that generate unaligned
>> traps these days...
>>
>>
> Here's the sanitizer log/qemu log, the host-cpu was an amd64.
>

AMD ROME


>
> qemu-kvm-system-x86_64: warning: host doesn't support requested feature:
> CPUID.01H:ECX.pcid [bit 17]
> qemu-kvm-system-x86_64: warning: host doesn't support requested feature:
> CPUID.07H:EBX.erms [bit 9]
> qemu-kvm-system-x86_64: warning: host doesn't support requested feature:
> CPUID.07H:EBX.invpcid [bit 10]
> qemu-kvm-system-x86_64: warning: host doesn't support requested feature:
> CPUID.01H:ECX.pcid [bit 17]
> qemu-kvm-system-x86_64: warning: host doesn't support requested feature:
> CPUID.07H:EBX.erms [bit 9]
> qemu-kvm-system-x86_64: warning: host doesn't support requested feature:
> CPUID.07H:EBX.invpcid [bit 10]
> third_party/qemu/softmmu/memory.c:1341:16: runtime error: load of
> misaligned address 0x52500020b10d for type 'uint32_t' (aka 'unsigned int'),
> which requires 4 byte alignment
> 0x52500020b10d: note: pointer points here
>  ab ab ab ab ab ab ab  ab ab ab ab ab ab ab ab  ab ab ab ab ab ab ab ab
>  ab ab ab ab ab ab ab ab  ab
>              ^
>     #0 0x55b34f8ef9d8 in memory_region_ram_device_read
> third_party/qemu/softmmu/memory.c:1341:16
>     #1 0x55b34f8ee8a8 in memory_region_read_accessor
> third_party/qemu/softmmu/memory.c:441:11
>     #2 0x55b34f8e06db in access_with_adjusted_size
> third_party/qemu/softmmu/memory.c:569:18
>     #3 0x55b34f8dfcb4 in memory_region_dispatch_read1
> third_party/qemu/softmmu/memory.c
>     #4 0x55b34f8dfcb4 in memory_region_dispatch_read
> third_party/qemu/softmmu/memory.c:1476:9
>     #5 0x55b34f8fa8b0 in flatview_read_continue
> third_party/qemu/softmmu/physmem.c:2744:23
>     #6 0x55b34f8fb0db in flatview_read
> third_party/qemu/softmmu/physmem.c:2786:12
>     #7 0x55b34f8faefa in address_space_read_full
> third_party/qemu/softmmu/physmem.c:2799:18
>     #8 0x55b34f8fb5b4 in address_space_rw
> third_party/qemu/softmmu/physmem.c:2827:16
>     #9 0x55b34f71eab5 in kvm_cpu_exec
> third_party/qemu/accel/kvm/kvm-all.c:3062:13
>     #10 0x55b34f7172e3 in kvm_vcpu_thread_fn
> third_party/qemu/accel/kvm/kvm-accel-ops.c:51:17
>     #11 0x55b350467044 in qemu_thread_start
> third_party/qemu/util/qemu-thread-posix.c:541:9
>     #12 0x55b34f6dba10 in asan_thread_start(void*)
> third_party/llvm/llvm-project/compiler-rt/lib/asan/asan_interceptors.cpp:234:28
>     #13 0x7f5e1c81a7d8 in start_thread
> (/usr/grte/v5/lib64/libpthread.so.0+0xb7d8) (BuildId:
> 3ccc1600b9140e48da03ed16e0210354)
>     #14 0x7f5e1c77169e in clone (/usr/grte/v5/lib64/libc.so.6+0x13969e)
> (BuildId: 280088eab084c30a3992a9bce5c35b44)
>
> SUMMARY: UndefinedBehaviorSanitizer: misaligned-pointer-use
> third_party/qemu/softmmu/memory.c:1341:16 in
>
>
>
>
>>
>> r~
>>
>>

[-- Attachment #2: Type: text/html, Size: 5761 bytes --]

Re: [PATCH] softmmu/memory: use memcpy for multi-byte accesses

[Peter Maydell]

On Wed, 15 Nov 2023 at 17:26, Patrick Venture <venture@google.com> wrote:
>
>
>
> On Wed, Nov 15, 2023 at 9:02 AM Richard Henderson <richard.henderson@linaro.org> wrote:
>>
>> On 11/15/23 08:58, Patrick Venture wrote:
>> >
>> >
>> > On Wed, Nov 15, 2023 at 2:35 AM Peter Maydell <peter.maydell@linaro.org
>> > <mailto:peter.maydell@linaro.org>> wrote:
>> >
>> >     On Tue, 14 Nov 2023 at 20:55, Patrick Venture <venture@google.com
>> >     <mailto:venture@google.com>> wrote:
>> >      > Avoids unaligned pointer issues.
>> >      >
>> >
>> >     It would be nice to be more specific in the commit message here, by
>> >     describing what kind of guest behaviour or machine config runs into this
>> >     problem, and whether this happens in a situation users are likely to
>> >     run into. If the latter, we should consider tagging the commit
>> >     with "Cc: qemu-stable@nongnu.org <mailto:qemu-stable@nongnu.org>" to have it
>> >     backported to the
>> >     stable release branches.
>> >
>> >
>> > Thanks! I'll update the commit message with v2.  We were seeing this in our
>> > infrastructure with unaligned accesses using the pointer dereference as there are no
>> > guarantees on alignment of the incoming values.
>>
>> Which host cpu, for reference?  There aren't many that generate unaligned traps these days...
>>
>
> Here's the sanitizer log/qemu log, the host-cpu was an amd64.
> third_party/qemu/softmmu/memory.c:1341:16: runtime error: load of misaligned address 0x52500020b10d for type 'uint32_t' (aka 'unsigned int'), which requires 4 byte alignment
> 0x52500020b10d: note: pointer points here
>  ab ab ab ab ab ab ab  ab ab ab ab ab ab ab ab  ab ab ab ab ab ab ab ab  ab ab ab ab ab ab ab ab  ab
>              ^

> SUMMARY: UndefinedBehaviorSanitizer: misaligned-pointer-use third_party/qemu/softmmu/memory.c:1341:16 in

Ah, right, so the clang/gcc undefined-behaviour sanitizers rather than
the actual host hardware barfing. (We definitely want to fix these
regardless.)

thanks
-- PMM