Re: [PATCH 1/2] tests/tcg/multiarch: fix code style in function main of test-mmap.c
On Mon, Nov 11, 2019 at 10:25:43AM +, Alex Benn??e wrote:
>
>Wei Yang writes:
>
>> This file uses quite a different code style and changing just one line
>> would leads to some awkward appearance.
>>
>> This is a preparation for the following replacement of
>> sysconf(_SC_PAGESIZE).
>>
>> BTW, to depress ERROR message from checkpatch.pl, this patch replaces
>> strtoul with qemu_strtoul.
>
>
>NACK I'm afraid.
>
>The tests/tcg directory all build against glibc only to make them easier
>to cross-compile for the various targets. If you run check-tcg and have
>a non-native cross compiler setup you'll notice this fails to build:
>
>BUILD aarch64-linux-user guest-tests with aarch64-linux-gnu-gcc
> /home/alex/lsrc/qemu.git/tests/tcg/multiarch/test-mmap.c: In function
> ???main???:
> /home/alex/lsrc/qemu.git/tests/tcg/multiarch/test-mmap.c:467:9: warning:
> implicit declaration of function ???qemu_strtoul???; did you mean
> ???strtoul [-Wimplicit-function-declaration]
> qemu_strtoul(argv[1], NULL, 0, );
> ^~~~
> strtoul
> /home/alex/lsrc/qemu.git/tests/tcg/multiarch/test-mmap.c:469:20: error:
> ???qemu_real_host_page_size??? undeclared (first use in this function)
> pagesize = qemu_real_host_page_size;
> ^~~~
> /home/alex/lsrc/qemu.git/tests/tcg/multiarch/test-mmap.c:469:20: note: each
> undeclared identifier is reported only once for each function it appears in
> make[2]: *** [../Makefile.target:103: test-mmap] Error 1
> make[1]: *** [/home/alex/lsrc/qemu.git/tests/tcg/Makefile.qemu:33:
> cross-build-guest-tests] Error 2
> make: *** [/home/alex/lsrc/qemu.git/tests/Makefile.include:1094:
> build-tcg-tests-aarch64-linux-user] Error 2
> make: *** Waiting for unfinished jobs
>
This output is from "make test" ?
>>
>> Signed-off-by: Wei Yang
>> ---
>> tests/tcg/multiarch/test-mmap.c | 67 ++---
>> 1 file changed, 36 insertions(+), 31 deletions(-)
>>
>> diff --git a/tests/tcg/multiarch/test-mmap.c
>> b/tests/tcg/multiarch/test-mmap.c
>> index 11d0e777b1..9ea49e2307 100644
>> --- a/tests/tcg/multiarch/test-mmap.c
>> +++ b/tests/tcg/multiarch/test-mmap.c
>> @@ -456,49 +456,54 @@ void check_invalid_mmaps(void)
>>
>> int main(int argc, char **argv)
>> {
>> -char tempname[] = "/tmp/.cmmapXX";
>> -unsigned int i;
>> -
>> -/* Trust the first argument, otherwise probe the system for our
>> - pagesize. */
>> -if (argc > 1)
>> -pagesize = strtoul(argv[1], NULL, 0);
>> -else
>> -pagesize = sysconf(_SC_PAGESIZE);
>> +char tempname[] = "/tmp/.cmmapXX";
>> +unsigned int i;
>> +
>> +/*
>> + * Trust the first argument, otherwise probe the system for our
>> + * pagesize.
>> + */
>> +if (argc > 1) {
>> +qemu_strtoul(argv[1], NULL, 0, );
>> +} else {
>> +pagesize = sysconf(_SC_PAGESIZE);
>> +}
>>
>> -/* Assume pagesize is a power of two. */
>> -pagemask = pagesize - 1;
>> -dummybuf = malloc (pagesize);
>> -printf ("pagesize=%u pagemask=%x\n", pagesize, pagemask);
>> +/* Assume pagesize is a power of two. */
>> +pagemask = pagesize - 1;
>> +dummybuf = malloc(pagesize);
>> +printf("pagesize=%u pagemask=%x\n", pagesize, pagemask);
>>
>> -test_fd = mkstemp(tempname);
>> -unlink(tempname);
>> +test_fd = mkstemp(tempname);
>> +unlink(tempname);
>>
>> -/* Fill the file with int's counting from zero and up. */
>> +/* Fill the file with int's counting from zero and up. */
>> for (i = 0; i < (pagesize * 4) / sizeof i; i++) {
>> checked_write(test_fd, , sizeof i);
>> }
>>
>> -/* Append a few extra writes to make the file end at non
>> - page boundary. */
>> +/*
>> + * Append a few extra writes to make the file end at non
>> + * page boundary.
>> + */
>> checked_write(test_fd, , sizeof i); i++;
>> checked_write(test_fd, , sizeof i); i++;
>> checked_write(test_fd, , sizeof i); i++;
>>
>> -test_fsize = lseek(test_fd, 0, SEEK_CUR);
>> +test_fsize = lseek(test_fd, 0, SEEK_CUR);
>>
>> -/* Run the tests. */
>> -check_aligned_anonymous_unfixed_mmaps();
>> -check_aligned_anonymous_unfixe
Re: [PATCH 2/2] core: replace sysconf(_SC_PAGESIZE) with qemu_real_host_page_size
On Mon, Nov 11, 2019 at 11:06:41AM +0100, Stefano Garzarella wrote:
>Why "core:" in the commit title?
>
>Perhaps to indicate that the patch concerns different subsystems,
>I'd use "qemu: ", but I'm not sure :-)
>
I didn't find a better one. Maybe "qemu" is better :-)
>On Tue, Oct 15, 2019 at 11:13:50AM +0800, Wei Yang wrote:
>> Signed-off-by: Wei Yang
>> Suggested-by: "Dr. David Alan Gilbert"
>> CC: Richard Henderson
>> ---
>> block/file-posix.c | 2 +-
>> net/l2tpv3.c| 2 +-
>> tests/tcg/multiarch/test-mmap.c | 2 +-
>> 3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 5d1995a07c..853ed42134 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -2562,7 +2562,7 @@ static void check_cache_dropped(BlockDriverState *bs,
>> Error **errp)
>> off_t end;
>>
>> /* mincore(2) page status information requires 1 byte per page */
>> -page_size = sysconf(_SC_PAGESIZE);
>> +page_size = qemu_real_host_page_size;
>> vec = g_malloc(DIV_ROUND_UP(window_size, page_size));
>>
>> end = raw_getlength(bs);
>> diff --git a/net/l2tpv3.c b/net/l2tpv3.c
>> index 55fea17c0f..5f843240de 100644
>> --- a/net/l2tpv3.c
>> +++ b/net/l2tpv3.c
>> @@ -41,7 +41,7 @@
>> * chosen to be sufficient to accommodate one packet with some headers
>> */
>>
>> -#define BUFFER_ALIGN sysconf(_SC_PAGESIZE)
>> +#define BUFFER_ALIGN qemu_real_host_page_size
>> #define BUFFER_SIZE 2048
>> #define IOVSIZE 2
>> #define MAX_L2TPV3_MSGCNT 64
>> diff --git a/tests/tcg/multiarch/test-mmap.c
>> b/tests/tcg/multiarch/test-mmap.c
>> index 9ea49e2307..370842e5c2 100644
>> --- a/tests/tcg/multiarch/test-mmap.c
>> +++ b/tests/tcg/multiarch/test-mmap.c
>> @@ -466,7 +466,7 @@ int main(int argc, char **argv)
>> if (argc > 1) {
>> qemu_strtoul(argv[1], NULL, 0, );
>> } else {
>> -pagesize = sysconf(_SC_PAGESIZE);
>> +pagesize = qemu_real_host_page_size;
>> }
>>
>> /* Assume pagesize is a power of two. */
>
>The patch LGTM:
>Reviewed-by: Stefano Garzarella
>
>Thanks,
>Stefano
>
--
Wei Yang
Help you, Help me
Re: [PATCH 0/2] replace sysconf(_SC_PAGESIZE) with qemu_real_host_page_size
On Tue, Oct 15, 2019 at 11:13:48AM +0800, Wei Yang wrote: >This is a following up patch to cleanup page size, suggested by >"Dr. David Alan Gilbert" . > >Patch 2 does the job, while during the cleanup I found test-mmap.c has quite a >lot code style problem. To make the code looks good, patch 1 is introduced to >make checkpatch.pl happy a little. Does this patch set look good? > >Wei Yang (2): > tests/tcg/multiarch: fix code style in function main of test-mmap.c > core: replace sysconf(_SC_PAGESIZE) with qemu_real_host_page_size > > block/file-posix.c | 2 +- > net/l2tpv3.c| 2 +- > tests/tcg/multiarch/test-mmap.c | 67 ++--- > 3 files changed, 38 insertions(+), 33 deletions(-) > >-- >2.17.1 > -- Wei Yang Help you, Help me
Re: [PATCH 2/2] core: replace getpagesize() with qemu_real_host_page_size
On Sun, Oct 13, 2019 at 08:28:41PM +1100, David Gibson wrote: >On Sun, Oct 13, 2019 at 10:11:45AM +0800, Wei Yang wrote: >> There are three page size in qemu: >> >> real host page size >> host page size >> target page size >> >> All of them have dedicate variable to represent. For the last two, we >> use the same form in the whole qemu project, while for the first one we >> use two forms: qemu_real_host_page_size and getpagesize(). >> >> qemu_real_host_page_size is defined to be a replacement of >> getpagesize(), so let it serve the role. >> >> [Note] Not fully tested for some arch or device. >> >> Signed-off-by: Wei Yang > >Reviewed-by: David Gibson > >Although the chances of someone messing this up again are almost 100%. > Hi, David I found put a check in checkpatch.pl may be a good way to prevent it. Just draft a patch, hope you would like it. >-- >David Gibson | I'll have my music baroque, and my code >david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! >http://www.ozlabs.org/~dgibson -- Wei Yang Help you, Help me
Re: [PATCH 2/2] core: replace getpagesize() with qemu_real_host_page_size
On Tue, Oct 15, 2019 at 02:45:15PM +0300, Yuval Shaia wrote:
>On Sun, Oct 13, 2019 at 10:11:45AM +0800, Wei Yang wrote:
>> There are three page size in qemu:
>>
>> real host page size
>> host page size
>> target page size
>>
>> All of them have dedicate variable to represent. For the last two, we
>> use the same form in the whole qemu project, while for the first one we
>> use two forms: qemu_real_host_page_size and getpagesize().
>>
>> qemu_real_host_page_size is defined to be a replacement of
>> getpagesize(), so let it serve the role.
>>
>> [Note] Not fully tested for some arch or device.
>>
>> Signed-off-by: Wei Yang
>> ---
>> accel/kvm/kvm-all.c| 6 +++---
>> backends/hostmem.c | 2 +-
>> block.c| 4 ++--
>> block/file-posix.c | 9 +
>> block/io.c | 2 +-
>> block/parallels.c | 2 +-
>> block/qcow2-cache.c| 2 +-
>> contrib/vhost-user-gpu/vugbm.c | 2 +-
>> exec.c | 6 +++---
>> hw/intc/s390_flic_kvm.c| 2 +-
>> hw/ppc/mac_newworld.c | 2 +-
>> hw/ppc/spapr_pci.c | 2 +-
>> hw/rdma/vmw/pvrdma_main.c | 2 +-
>
>for pvrdma stuff:
>
>Reviewed-by: Yuval Shaia
>Tested-by: Yuval Shaia
Thanks
>
>> hw/vfio/spapr.c| 7 ---
>> include/exec/ram_addr.h| 2 +-
>> include/qemu/osdep.h | 4 ++--
>> migration/migration.c | 2 +-
>> migration/postcopy-ram.c | 4 ++--
>> monitor/misc.c | 2 +-
>> target/ppc/kvm.c | 2 +-
>> tests/vhost-user-bridge.c | 8
>> util/mmap-alloc.c | 10 +-
>> util/oslib-posix.c | 4 ++--
>> util/oslib-win32.c | 2 +-
>> util/vfio-helpers.c| 12 ++--
>> 25 files changed, 52 insertions(+), 50 deletions(-)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index d2d96d73e8..140b0bd8f6 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -52,7 +52,7 @@
>> /* KVM uses PAGE_SIZE in its definition of KVM_COALESCED_MMIO_MAX. We
>> * need to use the real host PAGE_SIZE, as that's what KVM will use.
>> */
>> -#define PAGE_SIZE getpagesize()
>> +#define PAGE_SIZE qemu_real_host_page_size
>>
>> //#define DEBUG_KVM
>>
>> @@ -507,7 +507,7 @@ static int
>> kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
>> {
>> ram_addr_t start = section->offset_within_region +
>> memory_region_get_ram_addr(section->mr);
>> -ram_addr_t pages = int128_get64(section->size) / getpagesize();
>> +ram_addr_t pages = int128_get64(section->size) /
>> qemu_real_host_page_size;
>>
>> cpu_physical_memory_set_dirty_lebitmap(bitmap, start, pages);
>> return 0;
>> @@ -1841,7 +1841,7 @@ static int kvm_init(MachineState *ms)
>> * even with KVM. TARGET_PAGE_SIZE is assumed to be the minimum
>> * page size for the system though.
>> */
>> -assert(TARGET_PAGE_SIZE <= getpagesize());
>> +assert(TARGET_PAGE_SIZE <= qemu_real_host_page_size);
>>
>> s->sigmask_len = 8;
>>
>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>> index 6d333dc23c..e773bdfa6e 100644
>> --- a/backends/hostmem.c
>> +++ b/backends/hostmem.c
>> @@ -304,7 +304,7 @@ size_t host_memory_backend_pagesize(HostMemoryBackend
>> *memdev)
>> #else
>> size_t host_memory_backend_pagesize(HostMemoryBackend *memdev)
>> {
>> -return getpagesize();
>> +return qemu_real_host_page_size;
>> }
>> #endif
>>
>> diff --git a/block.c b/block.c
>> index 5944124845..98f47e2902 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -106,7 +106,7 @@ size_t bdrv_opt_mem_align(BlockDriverState *bs)
>> {
>> if (!bs || !bs->drv) {
>> /* page size or 4k (hdd sector size) should be on the safe side */
>> -return MAX(4096, getpagesize());
>> +return MAX(4096, qemu_real_host_page_size);
>> }
>>
>> return bs->bl.opt_mem_alignment;
>> @@ -116,7 +116,7 @@ size_t bdrv_min_mem_align(BlockDriverState *bs)
>> {
>> if (!bs || !bs->drv) {
>> /* page size or 4k (hdd sector size) should be on the safe side */
>> -return MAX(4096, getpag
[PATCH 1/2] tests/tcg/multiarch: fix code style in function main of test-mmap.c
This file uses quite a different code style and changing just one line
would leads to some awkward appearance.
This is a preparation for the following replacement of
sysconf(_SC_PAGESIZE).
BTW, to depress ERROR message from checkpatch.pl, this patch replaces
strtoul with qemu_strtoul.
Signed-off-by: Wei Yang
---
tests/tcg/multiarch/test-mmap.c | 67 ++---
1 file changed, 36 insertions(+), 31 deletions(-)
diff --git a/tests/tcg/multiarch/test-mmap.c b/tests/tcg/multiarch/test-mmap.c
index 11d0e777b1..9ea49e2307 100644
--- a/tests/tcg/multiarch/test-mmap.c
+++ b/tests/tcg/multiarch/test-mmap.c
@@ -456,49 +456,54 @@ void check_invalid_mmaps(void)
int main(int argc, char **argv)
{
- char tempname[] = "/tmp/.cmmapXX";
- unsigned int i;
-
- /* Trust the first argument, otherwise probe the system for our
- pagesize. */
- if (argc > 1)
- pagesize = strtoul(argv[1], NULL, 0);
- else
- pagesize = sysconf(_SC_PAGESIZE);
+char tempname[] = "/tmp/.cmmapXX";
+unsigned int i;
+
+/*
+ * Trust the first argument, otherwise probe the system for our
+ * pagesize.
+ */
+if (argc > 1) {
+qemu_strtoul(argv[1], NULL, 0, );
+} else {
+pagesize = sysconf(_SC_PAGESIZE);
+}
- /* Assume pagesize is a power of two. */
- pagemask = pagesize - 1;
- dummybuf = malloc (pagesize);
- printf ("pagesize=%u pagemask=%x\n", pagesize, pagemask);
+/* Assume pagesize is a power of two. */
+pagemask = pagesize - 1;
+dummybuf = malloc(pagesize);
+printf("pagesize=%u pagemask=%x\n", pagesize, pagemask);
- test_fd = mkstemp(tempname);
- unlink(tempname);
+test_fd = mkstemp(tempname);
+unlink(tempname);
- /* Fill the file with int's counting from zero and up. */
+/* Fill the file with int's counting from zero and up. */
for (i = 0; i < (pagesize * 4) / sizeof i; i++) {
checked_write(test_fd, , sizeof i);
}
- /* Append a few extra writes to make the file end at non
- page boundary. */
+/*
+ * Append a few extra writes to make the file end at non
+ * page boundary.
+ */
checked_write(test_fd, , sizeof i); i++;
checked_write(test_fd, , sizeof i); i++;
checked_write(test_fd, , sizeof i); i++;
- test_fsize = lseek(test_fd, 0, SEEK_CUR);
+test_fsize = lseek(test_fd, 0, SEEK_CUR);
- /* Run the tests. */
- check_aligned_anonymous_unfixed_mmaps();
- check_aligned_anonymous_unfixed_colliding_mmaps();
- check_aligned_anonymous_fixed_mmaps();
- check_file_unfixed_mmaps();
- check_file_fixed_mmaps();
- check_file_fixed_eof_mmaps();
- check_file_unfixed_eof_mmaps();
- check_invalid_mmaps();
+/* Run the tests. */
+check_aligned_anonymous_unfixed_mmaps();
+check_aligned_anonymous_unfixed_colliding_mmaps();
+check_aligned_anonymous_fixed_mmaps();
+check_file_unfixed_mmaps();
+check_file_fixed_mmaps();
+check_file_fixed_eof_mmaps();
+check_file_unfixed_eof_mmaps();
+check_invalid_mmaps();
- /* Fails at the moment. */
- /* check_aligned_anonymous_fixed_mmaps_collide_with_host(); */
+/* Fails at the moment. */
+/* check_aligned_anonymous_fixed_mmaps_collide_with_host(); */
- return EXIT_SUCCESS;
+return EXIT_SUCCESS;
}
--
2.17.1
[PATCH 2/2] core: replace sysconf(_SC_PAGESIZE) with qemu_real_host_page_size
Signed-off-by: Wei Yang
Suggested-by: "Dr. David Alan Gilbert"
CC: Richard Henderson
---
block/file-posix.c | 2 +-
net/l2tpv3.c| 2 +-
tests/tcg/multiarch/test-mmap.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 5d1995a07c..853ed42134 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2562,7 +2562,7 @@ static void check_cache_dropped(BlockDriverState *bs,
Error **errp)
off_t end;
/* mincore(2) page status information requires 1 byte per page */
-page_size = sysconf(_SC_PAGESIZE);
+page_size = qemu_real_host_page_size;
vec = g_malloc(DIV_ROUND_UP(window_size, page_size));
end = raw_getlength(bs);
diff --git a/net/l2tpv3.c b/net/l2tpv3.c
index 55fea17c0f..5f843240de 100644
--- a/net/l2tpv3.c
+++ b/net/l2tpv3.c
@@ -41,7 +41,7 @@
* chosen to be sufficient to accommodate one packet with some headers
*/
-#define BUFFER_ALIGN sysconf(_SC_PAGESIZE)
+#define BUFFER_ALIGN qemu_real_host_page_size
#define BUFFER_SIZE 2048
#define IOVSIZE 2
#define MAX_L2TPV3_MSGCNT 64
diff --git a/tests/tcg/multiarch/test-mmap.c b/tests/tcg/multiarch/test-mmap.c
index 9ea49e2307..370842e5c2 100644
--- a/tests/tcg/multiarch/test-mmap.c
+++ b/tests/tcg/multiarch/test-mmap.c
@@ -466,7 +466,7 @@ int main(int argc, char **argv)
if (argc > 1) {
qemu_strtoul(argv[1], NULL, 0, );
} else {
-pagesize = sysconf(_SC_PAGESIZE);
+pagesize = qemu_real_host_page_size;
}
/* Assume pagesize is a power of two. */
--
2.17.1
[PATCH 0/2] replace sysconf(_SC_PAGESIZE) with qemu_real_host_page_size
This is a following up patch to cleanup page size, suggested by "Dr. David Alan Gilbert" . Patch 2 does the job, while during the cleanup I found test-mmap.c has quite a lot code style problem. To make the code looks good, patch 1 is introduced to make checkpatch.pl happy a little. Wei Yang (2): tests/tcg/multiarch: fix code style in function main of test-mmap.c core: replace sysconf(_SC_PAGESIZE) with qemu_real_host_page_size block/file-posix.c | 2 +- net/l2tpv3.c| 2 +- tests/tcg/multiarch/test-mmap.c | 67 ++--- 3 files changed, 38 insertions(+), 33 deletions(-) -- 2.17.1
Re: [PATCH 0/2] cleanup on page size
Hi, All, I got one page size related question, hope to get some hint. There is one comment in page_size_init(). /* NOTE: we can always suppose that qemu_host_page_size >= TARGET_PAGE_SIZE */ The final result is true, since we compare qemu_host_page_size with TARGET_PAGE_SIZE and if not qemu_host_page_size will be assigned to TARGET_PAGE_SIZE. Generally, there is no problem, but one corner case for migration. In function ram_save_host_page(), it tries to save a whole host page. Or to be specific, it tries to save a whole REAL host page. The potential problem is when qemu_real_host_page_size < TARGET_PAGE_SIZE, this whole host page would be a wrong range. So I am wondering why we have the assumption written in page_size_init()? I tried to dig out who and why we have this comment, but found the first commit is commit 54936004fddc52c321cb3f9a9a51140e782bed5d Author: bellard Date: Tue May 13 00:25:15 2003 + mmap emulation git-svn-id: svn://svn.savannah.nongnu.org/qemu/trunk@158 c046a42c-6fe2-441c-8c8c-71466251a162 There is no reason logged. On Sun, Oct 13, 2019 at 10:11:43AM +0800, Wei Yang wrote: >Patch 1 simplify the definition of xxx_PAGE_ALIGN. >Patch 2 replaces getpagesize() with qemu_real_host_page_size. This one touch a >volume of code. If some point is not correct, I'd appreciate your >notification. > >Wei Yang (2): > cpu: use ROUND_UP() to define xxx_PAGE_ALIGN > core: replace getpagesize() with qemu_real_host_page_size > > accel/kvm/kvm-all.c| 6 +++--- > backends/hostmem.c | 2 +- > block.c| 4 ++-- > block/file-posix.c | 9 + > block/io.c | 2 +- > block/parallels.c | 2 +- > block/qcow2-cache.c| 2 +- > contrib/vhost-user-gpu/vugbm.c | 2 +- > exec.c | 6 +++--- > hw/intc/s390_flic_kvm.c| 2 +- > hw/ppc/mac_newworld.c | 2 +- > hw/ppc/spapr_pci.c | 2 +- > hw/rdma/vmw/pvrdma_main.c | 2 +- > hw/vfio/spapr.c| 7 --- > include/exec/cpu-all.h | 7 +++ > include/exec/ram_addr.h| 2 +- > include/qemu/osdep.h | 4 ++-- > migration/migration.c | 2 +- > migration/postcopy-ram.c | 4 ++-- > monitor/misc.c | 2 +- > target/ppc/kvm.c | 2 +- > tests/vhost-user-bridge.c | 8 > util/mmap-alloc.c | 10 +- > util/oslib-posix.c | 4 ++-- > util/oslib-win32.c | 2 +- > util/vfio-helpers.c | 12 ++-- > 26 files changed, 55 insertions(+), 54 deletions(-) > >-- >2.17.1 -- Wei Yang Help you, Help me
Re: [PATCH 2/2] core: replace getpagesize() with qemu_real_host_page_size
On Mon, Oct 14, 2019 at 10:15:02AM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.y...@linux.intel.com) wrote:
>> There are three page size in qemu:
>>
>> real host page size
>> host page size
>> target page size
>>
>> All of them have dedicate variable to represent. For the last two, we
>> use the same form in the whole qemu project, while for the first one we
>> use two forms: qemu_real_host_page_size and getpagesize().
>>
>> qemu_real_host_page_size is defined to be a replacement of
>> getpagesize(), so let it serve the role.
>
>We also use sysconf(_SC_PAGESIZE) in a few places.
You mean need to replace them too?
>
>Dave
>
>>
>> [Note] Not fully tested for some arch or device.
>>
>> Signed-off-by: Wei Yang
>> ---
>> accel/kvm/kvm-all.c| 6 +++---
>> backends/hostmem.c | 2 +-
>> block.c| 4 ++--
>> block/file-posix.c | 9 +
>> block/io.c | 2 +-
>> block/parallels.c | 2 +-
>> block/qcow2-cache.c| 2 +-
>> contrib/vhost-user-gpu/vugbm.c | 2 +-
>> exec.c | 6 +++---
>> hw/intc/s390_flic_kvm.c| 2 +-
>> hw/ppc/mac_newworld.c | 2 +-
>> hw/ppc/spapr_pci.c | 2 +-
>> hw/rdma/vmw/pvrdma_main.c | 2 +-
>> hw/vfio/spapr.c| 7 ---
>> include/exec/ram_addr.h| 2 +-
>> include/qemu/osdep.h | 4 ++--
>> migration/migration.c | 2 +-
>> migration/postcopy-ram.c | 4 ++--
>> monitor/misc.c | 2 +-
>> target/ppc/kvm.c | 2 +-
>> tests/vhost-user-bridge.c | 8
>> util/mmap-alloc.c | 10 +-
>> util/oslib-posix.c | 4 ++--
>> util/oslib-win32.c | 2 +-
>> util/vfio-helpers.c| 12 ++--
>> 25 files changed, 52 insertions(+), 50 deletions(-)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index d2d96d73e8..140b0bd8f6 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -52,7 +52,7 @@
>> /* KVM uses PAGE_SIZE in its definition of KVM_COALESCED_MMIO_MAX. We
>> * need to use the real host PAGE_SIZE, as that's what KVM will use.
>> */
>> -#define PAGE_SIZE getpagesize()
>> +#define PAGE_SIZE qemu_real_host_page_size
>>
>> //#define DEBUG_KVM
>>
>> @@ -507,7 +507,7 @@ static int
>> kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
>> {
>> ram_addr_t start = section->offset_within_region +
>> memory_region_get_ram_addr(section->mr);
>> -ram_addr_t pages = int128_get64(section->size) / getpagesize();
>> +ram_addr_t pages = int128_get64(section->size) /
>> qemu_real_host_page_size;
>>
>> cpu_physical_memory_set_dirty_lebitmap(bitmap, start, pages);
>> return 0;
>> @@ -1841,7 +1841,7 @@ static int kvm_init(MachineState *ms)
>> * even with KVM. TARGET_PAGE_SIZE is assumed to be the minimum
>> * page size for the system though.
>> */
>> -assert(TARGET_PAGE_SIZE <= getpagesize());
>> +assert(TARGET_PAGE_SIZE <= qemu_real_host_page_size);
>>
>> s->sigmask_len = 8;
>>
>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>> index 6d333dc23c..e773bdfa6e 100644
>> --- a/backends/hostmem.c
>> +++ b/backends/hostmem.c
>> @@ -304,7 +304,7 @@ size_t host_memory_backend_pagesize(HostMemoryBackend
>> *memdev)
>> #else
>> size_t host_memory_backend_pagesize(HostMemoryBackend *memdev)
>> {
>> -return getpagesize();
>> +return qemu_real_host_page_size;
>> }
>> #endif
>>
>> diff --git a/block.c b/block.c
>> index 5944124845..98f47e2902 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -106,7 +106,7 @@ size_t bdrv_opt_mem_align(BlockDriverState *bs)
>> {
>> if (!bs || !bs->drv) {
>> /* page size or 4k (hdd sector size) should be on the safe side */
>> -return MAX(4096, getpagesize());
>> +return MAX(4096, qemu_real_host_page_size);
>> }
>>
>> return bs->bl.opt_mem_alignment;
>> @@ -116,7 +116,7 @@ size_t bdrv_min_mem_align(BlockDriverState *bs)
>> {
>> if (!bs || !bs->drv) {
>> /* page size or 4k (hdd sector size) should be on the safe side */
>> -return
Re: [PATCH 1/2] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN
On Sun, Oct 13, 2019 at 07:38:04PM -0700, Richard Henderson wrote: >On 10/13/19 6:01 PM, Wei Yang wrote: >>> No, please. >>> >>> (1) The compiler does not know that qemu_*host_page_size is a power of 2, >>> and >>> will generate a real division at runtime. The same is true for >>> TARGET_PAGE_SIZE when TARGET_PAGE_BITS_VARY. >>> >> >> Confused >> >> The definition of ROUND_UP is: >> >> #define ROUND_UP(n, d) (((n) + (d) - 1) & -(0 ? (n) : (d))) > >Ah, my bad, I did confuse this with QEMU_ALIGN_UP. > >Hmm. > > lea -1(n, size), t > neg size > and size, t > >vs > > mov mask, t > not t > add n, t > and mask, t > >which is what I proposed here > >>> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04526.html > >I'm ok with your version. > >Reviewed-by: Richard Henderson > Thanks for your clarification. Have a nice day > >r~ > -- Wei Yang Help you, Help me
Re: [PATCH 1/2] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN
On Sun, Oct 13, 2019 at 11:56:35AM -0400, Richard Henderson wrote: >On 10/12/19 10:11 PM, Wei Yang wrote: >> Use ROUND_UP() to define, which is a little bit easy to read. >> >> Signed-off-by: Wei Yang >> --- >> include/exec/cpu-all.h | 7 +++ >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h >> index ad9ab85eb3..255bb186ac 100644 >> --- a/include/exec/cpu-all.h >> +++ b/include/exec/cpu-all.h >> @@ -220,7 +220,7 @@ extern int target_page_bits; >> >> #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS) >> #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1) >> -#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & >> TARGET_PAGE_MASK) >> +#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE) >> >> /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even >> * when intptr_t is 32-bit and we are aligning a long long. >> @@ -228,9 +228,8 @@ extern int target_page_bits; >> extern uintptr_t qemu_host_page_size; >> extern intptr_t qemu_host_page_mask; >> >> -#define HOST_PAGE_ALIGN(addr) (((addr) + qemu_host_page_size - 1) & >> qemu_host_page_mask) >> -#define REAL_HOST_PAGE_ALIGN(addr) (((addr) + qemu_real_host_page_size - 1) >> & \ >> -qemu_real_host_page_mask) >> +#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size) >> +#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr), >> qemu_real_host_page_size) > > >No, please. > >(1) The compiler does not know that qemu_*host_page_size is a power of 2, and >will generate a real division at runtime. The same is true for >TARGET_PAGE_SIZE when TARGET_PAGE_BITS_VARY. > Confused The definition of ROUND_UP is: #define ROUND_UP(n, d) (((n) + (d) - 1) & -(0 ? (n) : (d))) Why it will do division? This will be expanded to the same form as the original code, if my understanding is correct. Would you mind telling me more? >(2) The first hunk conflicts with an in-flight patch of mine: > >https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04526.html > > >r~ -- Wei Yang Help you, Help me
[PATCH 2/2] core: replace getpagesize() with qemu_real_host_page_size
There are three page size in qemu:
real host page size
host page size
target page size
All of them have dedicate variable to represent. For the last two, we
use the same form in the whole qemu project, while for the first one we
use two forms: qemu_real_host_page_size and getpagesize().
qemu_real_host_page_size is defined to be a replacement of
getpagesize(), so let it serve the role.
[Note] Not fully tested for some arch or device.
Signed-off-by: Wei Yang
---
accel/kvm/kvm-all.c| 6 +++---
backends/hostmem.c | 2 +-
block.c| 4 ++--
block/file-posix.c | 9 +
block/io.c | 2 +-
block/parallels.c | 2 +-
block/qcow2-cache.c| 2 +-
contrib/vhost-user-gpu/vugbm.c | 2 +-
exec.c | 6 +++---
hw/intc/s390_flic_kvm.c| 2 +-
hw/ppc/mac_newworld.c | 2 +-
hw/ppc/spapr_pci.c | 2 +-
hw/rdma/vmw/pvrdma_main.c | 2 +-
hw/vfio/spapr.c| 7 ---
include/exec/ram_addr.h| 2 +-
include/qemu/osdep.h | 4 ++--
migration/migration.c | 2 +-
migration/postcopy-ram.c | 4 ++--
monitor/misc.c | 2 +-
target/ppc/kvm.c | 2 +-
tests/vhost-user-bridge.c | 8
util/mmap-alloc.c | 10 +-
util/oslib-posix.c | 4 ++--
util/oslib-win32.c | 2 +-
util/vfio-helpers.c| 12 ++--
25 files changed, 52 insertions(+), 50 deletions(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index d2d96d73e8..140b0bd8f6 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -52,7 +52,7 @@
/* KVM uses PAGE_SIZE in its definition of KVM_COALESCED_MMIO_MAX. We
* need to use the real host PAGE_SIZE, as that's what KVM will use.
*/
-#define PAGE_SIZE getpagesize()
+#define PAGE_SIZE qemu_real_host_page_size
//#define DEBUG_KVM
@@ -507,7 +507,7 @@ static int
kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
{
ram_addr_t start = section->offset_within_region +
memory_region_get_ram_addr(section->mr);
-ram_addr_t pages = int128_get64(section->size) / getpagesize();
+ram_addr_t pages = int128_get64(section->size) / qemu_real_host_page_size;
cpu_physical_memory_set_dirty_lebitmap(bitmap, start, pages);
return 0;
@@ -1841,7 +1841,7 @@ static int kvm_init(MachineState *ms)
* even with KVM. TARGET_PAGE_SIZE is assumed to be the minimum
* page size for the system though.
*/
-assert(TARGET_PAGE_SIZE <= getpagesize());
+assert(TARGET_PAGE_SIZE <= qemu_real_host_page_size);
s->sigmask_len = 8;
diff --git a/backends/hostmem.c b/backends/hostmem.c
index 6d333dc23c..e773bdfa6e 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -304,7 +304,7 @@ size_t host_memory_backend_pagesize(HostMemoryBackend
*memdev)
#else
size_t host_memory_backend_pagesize(HostMemoryBackend *memdev)
{
-return getpagesize();
+return qemu_real_host_page_size;
}
#endif
diff --git a/block.c b/block.c
index 5944124845..98f47e2902 100644
--- a/block.c
+++ b/block.c
@@ -106,7 +106,7 @@ size_t bdrv_opt_mem_align(BlockDriverState *bs)
{
if (!bs || !bs->drv) {
/* page size or 4k (hdd sector size) should be on the safe side */
-return MAX(4096, getpagesize());
+return MAX(4096, qemu_real_host_page_size);
}
return bs->bl.opt_mem_alignment;
@@ -116,7 +116,7 @@ size_t bdrv_min_mem_align(BlockDriverState *bs)
{
if (!bs || !bs->drv) {
/* page size or 4k (hdd sector size) should be on the safe side */
-return MAX(4096, getpagesize());
+return MAX(4096, qemu_real_host_page_size);
}
return bs->bl.min_mem_alignment;
diff --git a/block/file-posix.c b/block/file-posix.c
index f12c06de2d..f60ac3f93f 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -322,7 +322,7 @@ static void raw_probe_alignment(BlockDriverState *bs, int
fd, Error **errp)
{
BDRVRawState *s = bs->opaque;
char *buf;
-size_t max_align = MAX(MAX_BLOCKSIZE, getpagesize());
+size_t max_align = MAX(MAX_BLOCKSIZE, qemu_real_host_page_size);
size_t alignments[] = {1, 512, 1024, 2048, 4096};
/* For SCSI generic devices the alignment is not really used.
@@ -1131,13 +1131,14 @@ static void raw_refresh_limits(BlockDriverState *bs,
Error **errp)
ret = sg_get_max_segments(s->fd);
if (ret > 0) {
-bs->bl.max_transfer = MIN(bs->bl.max_transfer, ret *
getpagesize());
+bs->bl.max_transfer = MIN(bs->bl.max_transfer,
+ ret * qemu_real_host_page_size);
}
}
raw_probe_alignment(bs, s->fd, errp);
bs->bl.min_mem_alignment = s->buf_align;
-bs->bl.opt_mem_alignment = MAX(s
[PATCH 0/2] cleanup on page size
Patch 1 simplify the definition of xxx_PAGE_ALIGN. Patch 2 replaces getpagesize() with qemu_real_host_page_size. This one touch a volume of code. If some point is not correct, I'd appreciate your notification. Wei Yang (2): cpu: use ROUND_UP() to define xxx_PAGE_ALIGN core: replace getpagesize() with qemu_real_host_page_size accel/kvm/kvm-all.c| 6 +++--- backends/hostmem.c | 2 +- block.c| 4 ++-- block/file-posix.c | 9 + block/io.c | 2 +- block/parallels.c | 2 +- block/qcow2-cache.c| 2 +- contrib/vhost-user-gpu/vugbm.c | 2 +- exec.c | 6 +++--- hw/intc/s390_flic_kvm.c| 2 +- hw/ppc/mac_newworld.c | 2 +- hw/ppc/spapr_pci.c | 2 +- hw/rdma/vmw/pvrdma_main.c | 2 +- hw/vfio/spapr.c| 7 --- include/exec/cpu-all.h | 7 +++ include/exec/ram_addr.h| 2 +- include/qemu/osdep.h | 4 ++-- migration/migration.c | 2 +- migration/postcopy-ram.c | 4 ++-- monitor/misc.c | 2 +- target/ppc/kvm.c | 2 +- tests/vhost-user-bridge.c | 8 util/mmap-alloc.c | 10 +- util/oslib-posix.c | 4 ++-- util/oslib-win32.c | 2 +- util/vfio-helpers.c| 12 ++-- 26 files changed, 55 insertions(+), 54 deletions(-) -- 2.17.1
[PATCH 1/2] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN
Use ROUND_UP() to define, which is a little bit easy to read. Signed-off-by: Wei Yang --- include/exec/cpu-all.h | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h index ad9ab85eb3..255bb186ac 100644 --- a/include/exec/cpu-all.h +++ b/include/exec/cpu-all.h @@ -220,7 +220,7 @@ extern int target_page_bits; #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS) #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1) -#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK) +#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE) /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even * when intptr_t is 32-bit and we are aligning a long long. @@ -228,9 +228,8 @@ extern int target_page_bits; extern uintptr_t qemu_host_page_size; extern intptr_t qemu_host_page_mask; -#define HOST_PAGE_ALIGN(addr) (((addr) + qemu_host_page_size - 1) & qemu_host_page_mask) -#define REAL_HOST_PAGE_ALIGN(addr) (((addr) + qemu_real_host_page_size - 1) & \ -qemu_real_host_page_mask) +#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size) +#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_real_host_page_size) /* same as PROT_xxx */ #define PAGE_READ 0x0001 -- 2.17.1
Re: [PULL 5/5] multifd: Use number of channels as listen backlog
On Fri, Oct 11, 2019 at 12:40:03PM +0200, Juan Quintela wrote:
>Wei Yang wrote:
>> On Wed, Sep 04, 2019 at 08:29:15AM +0200, Juan Quintela wrote:
>>>Reviewed-by: Daniel P. Berrang??
>>>Signed-off-by: Juan Quintela
>>>---
>>> migration/socket.c | 7 ++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>>diff --git a/migration/socket.c b/migration/socket.c
>>>index e63f5e1612..97c9efde59 100644
>>>--- a/migration/socket.c
>>>+++ b/migration/socket.c
>>>@@ -178,10 +178,15 @@ static void
>>>socket_start_incoming_migration(SocketAddress *saddr,
>>> {
>>> QIONetListener *listener = qio_net_listener_new();
>>> size_t i;
>>>+int num = 1;
>>>
>>> qio_net_listener_set_name(listener, "migration-socket-listener");
>>>
>>>-if (qio_net_listener_open_sync(listener, saddr, 1, errp) < 0) {
>>>+if (migrate_use_multifd()) {
>>>+num = migrate_multifd_channels();
>>>+}
>>>+
>>>+if (qio_net_listener_open_sync(listener, saddr, num, errp) < 0) {
>>> object_unref(OBJECT(listener));
>>> return;
>>> }
>>
>> My confusion is this function is called at the beginning of the program,
>> which
>> means we didn't set multifd on or change the multifd channel parameter.
>>
>> They are the default value at this point.
>>
>> Am I right?
>
>Hi
>
>good catch!
>
>You are right. The fix worked for me because I always use on the
>command line:
>
>--global migration.multifd-channels=10
>
>or whatever number I want to avoid typing. I can only see two
>solutions:
>- increase the number always
You mean change default value? Then which one should we choose?
>- require "defer" when using multifd to be able to setup parameters.
>
>Any other good ideas?
Would you mind explaining more about "defer"? How this works?
>
>Thanks, Juan.
>
>PD. I was having problem reproducing this issue because I use the
>command line for the parameter.
--
Wei Yang
Help you, Help me
Re: [Qemu-devel] [PULL 5/5] multifd: Use number of channels as listen backlog
On Wed, Sep 04, 2019 at 08:29:15AM +0200, Juan Quintela wrote:
>Reviewed-by: Daniel P. Berrangé
>Signed-off-by: Juan Quintela
>---
> migration/socket.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
>diff --git a/migration/socket.c b/migration/socket.c
>index e63f5e1612..97c9efde59 100644
>--- a/migration/socket.c
>+++ b/migration/socket.c
>@@ -178,10 +178,15 @@ static void
>socket_start_incoming_migration(SocketAddress *saddr,
> {
> QIONetListener *listener = qio_net_listener_new();
> size_t i;
>+int num = 1;
>
> qio_net_listener_set_name(listener, "migration-socket-listener");
>
>-if (qio_net_listener_open_sync(listener, saddr, 1, errp) < 0) {
>+if (migrate_use_multifd()) {
>+num = migrate_multifd_channels();
>+}
>+
>+if (qio_net_listener_open_sync(listener, saddr, num, errp) < 0) {
> object_unref(OBJECT(listener));
> return;
> }
My confusion is this function is called at the beginning of the program, which
means we didn't set multifd on or change the multifd channel parameter.
They are the default value at this point.
Am I right?
>--
>2.21.0
>
--
Wei Yang
Help you, Help me
Re: [Qemu-block] [PATCH 5/5] hw/block/pflash_cfi02: Add the DeviceReset() handler
On Sun, May 05, 2019 at 10:06:02PM +0200, Philippe Mathieu-Daudé wrote: >The pflash device is a child of TYPE_DEVICE, so it can implement >the DeviceReset handler. Actually it has to implement it, else >on machine reset it might stay in an incoherent state, as it has >been reported in the buglink listed below. > >Add the DeviceReset handler and remove its call from the realize() >function. > >Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713 >Reported-by: Laszlo Ersek >Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Wei Yang -- Wei Yang Help you, Help me
Re: [Qemu-block] [PATCH 4/5] hw/block/pflash_cfi02: Extract the pflash_reset() code
On Sun, May 05, 2019 at 10:06:01PM +0200, Philippe Mathieu-Daudé wrote:
>The reset() code is used in various places, refactor it.
>
>Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Wei Yang
>---
> hw/block/pflash_cfi02.c | 25 +++--
> 1 file changed, 15 insertions(+), 10 deletions(-)
>
>diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>index f2c6201f813..f321b74433c 100644
>--- a/hw/block/pflash_cfi02.c
>+++ b/hw/block/pflash_cfi02.c
>@@ -120,6 +120,17 @@ static void pflash_register_memory(PFlashCFI02 *pfl, int
>rom_mode)
> pfl->rom_mode = rom_mode;
> }
>
>+static void pflash_reset(PFlashCFI02 *pfl)
>+{
>+trace_pflash_reset();
>+timer_del(>timer);
>+pfl->bypass = 0;
>+pfl->wcycle = 0;
>+pfl->cmd = 0;
>+pfl->status = 0;
>+pflash_register_memory(pfl, 1);
>+}
>+
> static void pflash_timer (void *opaque)
> {
> PFlashCFI02 *pfl = opaque;
>@@ -129,11 +140,10 @@ static void pflash_timer (void *opaque)
> pfl->status ^= 0x80;
> if (pfl->bypass) {
> pfl->wcycle = 2;
>+pfl->cmd = 0;
> } else {
>-pflash_register_memory(pfl, 1);
>-pfl->wcycle = 0;
>+pflash_reset(pfl);
> }
>-pfl->cmd = 0;
> }
>
> static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr offset,
>@@ -481,10 +491,7 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
>
> /* Reset flash */
> reset_flash:
>-trace_pflash_reset();
>-pfl->bypass = 0;
>-pfl->wcycle = 0;
>-pfl->cmd = 0;
>+pflash_reset(pfl);
> return;
>
> do_bypass:
>@@ -588,9 +595,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error
>**errp)
> sysbus_init_mmio(SYS_BUS_DEVICE(dev), >mem);
>
> timer_init_ns(>timer, QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
>-pfl->wcycle = 0;
>-pfl->cmd = 0;
>-pfl->status = 0;
>+pflash_reset(pfl);
> /* Hardcoded CFI table (mostly from SG29 Spansion flash) */
> /* Standard "QRY" string */
> pfl->cfi_table[0x10] = 'Q';
>--
>2.20.1
--
Wei Yang
Help you, Help me
Re: [Qemu-block] [PATCH 3/5] hw/block/pflash_cfi01: Add the DeviceReset() handler
On Sun, May 05, 2019 at 10:06:00PM +0200, Philippe Mathieu-Daudé wrote: >The pflash device is a child of TYPE_DEVICE, so it can implement >the DeviceReset handler. Actually it has to implement it, else >on machine reset it might stay in an incoherent state, as it has >been reported in the buglink listed below. > >Add the DeviceReset handler and remove its call from the realize() >function. > >Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713 >Reported-by: Laszlo Ersek >Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Wei Yang -- Wei Yang Help you, Help me
Re: [Qemu-block] [PATCH 2/5] hw/block/pflash_cfi01: Extract the pflash_reset() code
On Sun, May 05, 2019 at 10:05:59PM +0200, Philippe Mathieu-Daudé wrote:
>The reset() code is used in various places, refactor it.
>
>Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Wei Yang
>---
> hw/block/pflash_cfi01.c | 21 -
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
>diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>index 6dc04f156a7..073cd14978f 100644
>--- a/hw/block/pflash_cfi01.c
>+++ b/hw/block/pflash_cfi01.c
>@@ -108,6 +108,15 @@ static const VMStateDescription vmstate_pflash = {
> }
> };
>
>+static void pflash_reset(PFlashCFI01 *pfl)
>+{
>+trace_pflash_reset();
>+pfl->wcycle = 0;
>+pfl->cmd = 0;
>+pfl->status = 0;
>+memory_region_rom_device_set_romd(>mem, true);
>+}
>+
> /* Perform a CFI query based on the bank width of the flash.
> * If this code is called we know we have a device_width set for
> * this flash.
>@@ -275,8 +284,7 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr
>offset,
> default:
> /* This should never happen : reset state & treat it as a read */
> DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
>-pfl->wcycle = 0;
>-pfl->cmd = 0;
>+pflash_reset(pfl);
> /* fall through to read code */
> case 0x00:
> /* Flash area read */
>@@ -639,10 +647,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
> "\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
>
> reset_flash:
>-trace_pflash_reset();
>-memory_region_rom_device_set_romd(>mem, true);
>-pfl->wcycle = 0;
>-pfl->cmd = 0;
>+pflash_reset(pfl);
> }
>
>
>@@ -757,9 +762,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error
>**errp)
> pfl->max_device_width = pfl->device_width;
> }
>
>-pfl->wcycle = 0;
>-pfl->cmd = 0;
>-pfl->status = 0;
>+pflash_reset(pfl);
> /* Hardcoded CFI table */
> /* Standard "QRY" string */
> pfl->cfi_table[0x10] = 'Q';
>--
>2.20.1
--
Wei Yang
Help you, Help me
Re: [Qemu-block] [PATCH 1/5] hw/block/pflash_cfi01: Removed an unused timer
On Sun, May 05, 2019 at 10:05:58PM +0200, Philippe Mathieu-Daudé wrote:
>The 'CFI01' NOR flash was introduced in commit 29133e9a0fff, with
>timing modelled. One year later, the CFI02 model was introduced
>(commit 05ee37ebf630) based on the CFI01 model. As noted in the
>header, "It does not support timings". 12 years later, we never
>had to model the device timings. Time to remove the unused timer,
>we can still add it back if required.
>
>Suggested-by: Laszlo Ersek
>Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Wei Yang
>---
>Yes, I plan to model those timings later. Actually I have a series
>working, but I'd rather first
>1/ refactor common code between the both CFI implementations,
>2/ discuss on list whether or not use timings for the Virt flash.
>---
> hw/block/pflash_cfi01.c | 15 ---
> 1 file changed, 15 deletions(-)
>
>diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>index 16dfae14b80..6dc04f156a7 100644
>--- a/hw/block/pflash_cfi01.c
>+++ b/hw/block/pflash_cfi01.c
>@@ -42,7 +42,6 @@
> #include "hw/block/flash.h"
> #include "sysemu/block-backend.h"
> #include "qapi/error.h"
>-#include "qemu/timer.h"
> #include "qemu/bitops.h"
> #include "qemu/host-utils.h"
> #include "qemu/log.h"
>@@ -86,7 +85,6 @@ struct PFlashCFI01 {
> uint8_t cfi_table[0x52];
> uint64_t counter;
> unsigned int writeblock_size;
>-QEMUTimer *timer;
> MemoryRegion mem;
> char *name;
> void *storage;
>@@ -110,18 +108,6 @@ static const VMStateDescription vmstate_pflash = {
> }
> };
>
>-static void pflash_timer (void *opaque)
>-{
>-PFlashCFI01 *pfl = opaque;
>-
>-trace_pflash_timer_expired(pfl->cmd);
>-/* Reset flash */
>-pfl->status ^= 0x80;
>-memory_region_rom_device_set_romd(>mem, true);
>-pfl->wcycle = 0;
>-pfl->cmd = 0;
>-}
>-
> /* Perform a CFI query based on the bank width of the flash.
> * If this code is called we know we have a device_width set for
> * this flash.
>@@ -771,7 +757,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error
>**errp)
> pfl->max_device_width = pfl->device_width;
> }
>
>-pfl->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
> pfl->wcycle = 0;
> pfl->cmd = 0;
> pfl->status = 0;
>--
>2.20.1
--
Wei Yang
Help you, Help me
Re: [Qemu-block] [Qemu-devel] [PATCH] block/pflash_cfi02: Fix memory leak and potential use-after-free
On Tue, Feb 19, 2019 at 10:37:27AM -0500, Stephen Checkoway wrote:
>Don't dynamically allocate the pflash's timer. But do use timer_del in
>an unrealize function to make sure that the timer can't fire after the
>pflash_t has been freed.
>
>Signed-off-by: Stephen Checkoway
Reviewed-by: Wei Yang
>---
> hw/block/pflash_cfi02.c | 15 +++
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
>diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>index 0f8b7b8c7b..1588aeff5a 100644
>--- a/hw/block/pflash_cfi02.c
>+++ b/hw/block/pflash_cfi02.c
>@@ -84,7 +84,7 @@ struct pflash_t {
> uint16_t unlock_addr0;
> uint16_t unlock_addr1;
> uint8_t cfi_table[0x52];
>-QEMUTimer *timer;
>+QEMUTimer timer;
> /* The device replicates the flash memory across its memory space.
> Emulate
> * that by having a container (.mem) filled with an array of aliases
> * (.mem_mappings) pointing to the flash memory (.orig_mem).
>@@ -429,7 +429,7 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
> }
> pfl->status = 0x00;
> /* Let's wait 5 seconds before chip erase is done */
>-timer_mod(pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>+timer_mod(>timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> (NANOSECONDS_PER_SECOND * 5));
> break;
> case 0x30:
>@@ -444,7 +444,7 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
> }
> pfl->status = 0x00;
> /* Let's wait 1/2 second before sector erase is done */
>-timer_mod(pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>+timer_mod(>timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> (NANOSECONDS_PER_SECOND / 2));
> break;
> default:
>@@ -596,7 +596,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error
>**errp)
> pfl->rom_mode = 1;
> sysbus_init_mmio(SYS_BUS_DEVICE(dev), >mem);
>
>-pfl->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
>+timer_init_ns(>timer, QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
> pfl->wcycle = 0;
> pfl->cmd = 0;
> pfl->status = 0;
>@@ -695,11 +695,18 @@ static Property pflash_cfi02_properties[] = {
> DEFINE_PROP_END_OF_LIST(),
> };
>
>+static void pflash_cfi02_unrealize(DeviceState *dev, Error **errp)
>+{
>+pflash_t *pfl = CFI_PFLASH02(dev);
>+timer_del(>timer);
>+}
>+
> static void pflash_cfi02_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
>
> dc->realize = pflash_cfi02_realize;
>+dc->unrealize = pflash_cfi02_unrealize;
> dc->props = pflash_cfi02_properties;
> set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> }
>--
>2.17.2 (Apple Git-113)
>
--
Wei Yang
Help you, Help me
Re: [Qemu-block] [Qemu-devel] [PATCH] hw/ide/ich: Compile ich.c only if CONFIG_PCI is also set
On Tue, Feb 19, 2019 at 04:55:57PM +0100, Thomas Huth wrote: >With the upcoming Kconfig-like build system, it will be easy to >build also version of QEMU that only contain a single machine. Some Sorry for my poor English. What is also version? >of these machines (like the ARM cubieboard) use CONFIG_AHCI for an >AHCI sysbus device, but do not use CONFIG_PCI since they do not feature >a PCI bus. In this case linking fails: > >../hw/ide/ich.o: In function `pci_ich9_ahci_realize': >hw/ide/ich.c:124: undefined reference to `pci_allocate_irq' >hw/ide/ich.c:126: undefined reference to `pci_register_bar' >hw/ide/ich.c:128: undefined reference to `pci_register_bar' >hw/ide/ich.c:131: undefined reference to `pci_add_capability' >hw/ide/ich.c:147: undefined reference to `msi_init' >../hw/ide/ich.o: In function `pci_ich9_uninit': >hw/ide/ich.c:158: undefined reference to `msi_uninit' >../hw/ide/ich.o:(.data.rel+0x50): undefined reference to `vmstate_pci_device' > >Thus we must only compile ich.c if CONFIG_PCI is also set. > >Signed-off-by: Thomas Huth >--- > hw/ide/Makefile.objs | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/hw/ide/Makefile.objs b/hw/ide/Makefile.objs >index a142add..dfe53af 100644 >--- a/hw/ide/Makefile.objs >+++ b/hw/ide/Makefile.objs >@@ -9,6 +9,6 @@ common-obj-$(CONFIG_IDE_MMIO) += mmio.o > common-obj-$(CONFIG_IDE_VIA) += via.o > common-obj-$(CONFIG_MICRODRIVE) += microdrive.o > common-obj-$(CONFIG_AHCI) += ahci.o >-common-obj-$(CONFIG_AHCI) += ich.o >+common-obj-$(call land,$(CONFIG_AHCI),$(CONFIG_PCI)) += ich.o > common-obj-$(CONFIG_ALLWINNER_A10) += ahci-allwinner.o > common-obj-$(CONFIG_IDE_SII3112) += sii3112.o >-- >1.8.3.1 > -- Wei Yang Help you, Help me
