| Message ID | 20231114205507.3792947-1-venture@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | softmmu/memory: use memcpy for multi-byte accesses | expand |
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~
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~ >
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
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
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 >
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 >
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~
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~ > >
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~ >> >>
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
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; } }