os: don't corrupt pre-existing memory-backend data with prealloc
PATCH v3 1/1 os: don't corrupt pre-existing memory-backend data with prealloc
When using a memory-backend object with prealloc turned on, QEMU will memset() the first byte in every memory page to zero. While this might have been acceptable for memory backends associated with RAM, this corrupts application data for NVDIMMs.
Instead of setting every page to zero, read the current byte value and then just write that same value back, so we are not corrupting the original data. Directly write the value instead of memset()ing it, since there's no benefit to memset for a single byte write.
---
Changed in v3:
- Mark the target of the write as volatile, instead of the intermedia variable (Andrea)
util/oslib-posix.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
util/oslib-posix.c +14 -1
@@ -355,7 +355,20 @@ void os_mem_prealloc(int fd, char *area, size_t memory, Error **errp)/* MAP_POPULATE silently ignores failures */ for (i =0 ; i < numpages; i++) { -memset (area + (hpagesize * i),0 ,1 ); +/* +* Read & write back the same value, so we don't +* corrupt existing user/app data that might be +* stored. +* +* 'volatile' to stop compiler optimizing this away +* to a no-op +* +* TODO: get a better solution from kernel so we +* don't need to write at all so we don't cause +* wear on the storage backing the region... +*/ +char *page = area + (hpagesize * i); + *(volatile char *)page = *page; } }
Signature · 1 line
2.9.3
This series seems to have some coding style problems. See output below for more information:
Hi,
This series seems to have some coding style problems. See output below for more information:
Message-id: [email protected]
Type: series
Subject: [Qemu-devel] [PATCH v3] os: don't corrupt pre-existing memory-backend data with prealloc
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
4210940 os: don't corrupt pre-existing memory-backend data with prealloc
=== OUTPUT BEGIN ===
Checking PATCH 1/1: os: don't corrupt pre-existing memory-backend data with prealloc...
ERROR: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt #43: FILE: util/oslib-posix.c:371:
+ *(volatile char *)page = *page;
total: 1 errors, 0 warnings, 21 lines checked
Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to [email protected]
Quoted 11 lines · Daniel P. Berrange
When using a memory-backend object with prealloc turned on, QEMU will
memset()the first byte in every memory page to zero. While this might have been acceptable for memory backends associated with RAM, this corrupts application data for NVDIMMs.Instead of setting every page to zero, read the current byte value and then just write that same value back, so we are not corrupting the original data. Directly write the value instead of memset()ing it, since there's no benefit to memset for a single byte write.
Quoted 4 lines
Signed-off-by Daniel P. Berrange <[email protected]>---
Changed in v3:
Thanks,
Andrea
Quoted 14 lines · Daniel P. Berrange
When using a memory-backend object with prealloc turned on, QEMU will
memset()the first byte in every memory page to zero. While this might have been acceptable for memory backends associated with RAM, this corrupts application data for NVDIMMs.Instead of setting every page to zero, read the current byte value and then just write that same value back, so we are not corrupting the original data. Directly write the value instead of memset()ing it, since there's no benefit to memset for a single byte write.
Signed-off-by Daniel P. Berrange <[email protected]>---
Quoted 7 lines
Changed in v3:
- Mark the target of the write as volatile, instead of the intermedia variable (Andrea)
util/oslib-posix.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
Stefan Hajnoczi
AppliedQuoted 14 lines · Daniel P. Berrange
When using a memory-backend object with prealloc turned on, QEMU will
memset()the first byte in every memory page to zero. While this might have been acceptable for memory backends associated with RAM, this corrupts application data for NVDIMMs.Instead of setting every page to zero, read the current byte value and then just write that same value back, so we are not corrupting the original data. Directly write the value instead of memset()ing it, since there's no benefit to memset for a single byte write.
Signed-off-by Daniel P. Berrange <[email protected]>---
Quoted 7 lines
Changed in v3:
- Mark the target of the write as volatile, instead of the intermedia variable (Andrea)
util/oslib-posix.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
There are no official maintainers for this source file. I want to make sure this patch goes into QEMU 2.9, so...
Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block
Stefan
Quoted 20 lines · Stefan Hajnoczi
Quoted 19 lines · Daniel P. Berrange
When using a memory-backend object with prealloc turned on, QEMU will
memset()the first byte in every memory page to zero. While this might have been acceptable for memory backends associated with RAM, this corrupts application data for NVDIMMs.Instead of setting every page to zero, read the current byte value and then just write that same value back, so we are not corrupting the original data. Directly write the value instead of memset()ing it, since there's no benefit to memset for a single byte write.
Signed-off-by Daniel P. Berrange <[email protected]>---
Changed in v3:
- Mark the target of the write as volatile, instead of the intermedia variable (Andrea)
Quoted 8 lines
Quoted 2 lines
util/oslib-posix.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)There are no official maintainers for this source file. I want to make sure this patch goes into QEMU 2.9, so...
Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block
Yeah, I was leaving it to you because it's related to NVDIMM. Thanks,
Paolo