* [PATCH v2 0/2] accel: kvm: Some bugfixes for kvm dirty log
@ 2020-12-17 1:49 Keqian Zhu
2020-12-17 1:49 ` [PATCH v2 1/2] accel: kvm: Fix memory waste under mismatch page size Keqian Zhu
` (4 more replies)
0 siblings, 5 replies; 22+ messages in thread
From: Keqian Zhu @ 2020-12-17 1:49 UTC (permalink / raw)
To: Peter Maydell, Paolo Bonzini, Dr . David Alan Gilbert,
Andrew Jones, Peter Xu
Cc: jiangkunkun, qemu-devel, qemu-arm, Zenghui Yu, wanghaibin.wang,
Keqian Zhu
Hi all,
This series fixes memory waste and adds alignment check for unmatched
qemu_real_host_page_size and TARGET_PAGE_SIZE.
Thanks.
Keqian Zhu (2):
accel: kvm: Fix memory waste under mismatch page size
accel: kvm: Add aligment assert for kvm_log_clear_one_slot
accel/kvm/kvm-all.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
--
2.23.0
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 1/2] accel: kvm: Fix memory waste under mismatch page size
2020-12-17 1:49 [PATCH v2 0/2] accel: kvm: Some bugfixes for kvm dirty log Keqian Zhu
@ 2020-12-17 1:49 ` Keqian Zhu
2020-12-17 1:49 ` [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot Keqian Zhu
` (3 subsequent siblings)
4 siblings, 0 replies; 22+ messages in thread
From: Keqian Zhu @ 2020-12-17 1:49 UTC (permalink / raw)
To: Peter Maydell, Paolo Bonzini, Dr . David Alan Gilbert,
Andrew Jones, Peter Xu
Cc: jiangkunkun, qemu-devel, qemu-arm, Zenghui Yu, wanghaibin.wang,
Keqian Zhu
When handle dirty log, we face qemu_real_host_page_size and
TARGET_PAGE_SIZE. The first one is the granule of KVM dirty
bitmap, and the second one is the granule of QEMU dirty bitmap.
As qemu_real_host_page_size >= TARGET_PAGE_SIZE (kvm_init()
enforced it), misuse TARGET_PAGE_SIZE to init kvmslot dirty_bmap
may waste memory. For example, when qemu_real_host_page_size is
64K and TARGET_PAGE_SIZE is 4K, it wastes 93.75% (15/16) memory.
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
accel/kvm/kvm-all.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
---
v2
- Address Andrew's comment (qemu_real_host_page_size >= TARGET_PAGE_SIZE
is a rule).
- Add Andrew and Peter's R-b.
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 389eaace72..f6b16a8df8 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -620,8 +620,12 @@ static void kvm_memslot_init_dirty_bitmap(KVMSlot *mem)
* too, in most cases).
* So for now, let's align to 64 instead of HOST_LONG_BITS here, in
* a hope that sizeof(long) won't become >8 any time soon.
+ *
+ * Note: the granule of kvm dirty log is qemu_real_host_page_size.
+ * And mem->memory_size is aligned to it (otherwise this mem can't
+ * be registered to KVM).
*/
- hwaddr bitmap_size = ALIGN(((mem->memory_size) >> TARGET_PAGE_BITS),
+ hwaddr bitmap_size = ALIGN(mem->memory_size / qemu_real_host_page_size,
/*HOST_LONG_BITS*/ 64) / 8;
mem->dirty_bmap = g_malloc0(bitmap_size);
}
--
2.23.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot
2020-12-17 1:49 [PATCH v2 0/2] accel: kvm: Some bugfixes for kvm dirty log Keqian Zhu
2020-12-17 1:49 ` [PATCH v2 1/2] accel: kvm: Fix memory waste under mismatch page size Keqian Zhu
@ 2020-12-17 1:49 ` Keqian Zhu
2020-12-17 12:18 ` Andrew Jones
` (3 more replies)
2021-01-06 7:07 ` [PATCH v2 0/2] accel: kvm: Some bugfixes for kvm dirty log Keqian Zhu
` (2 subsequent siblings)
4 siblings, 4 replies; 22+ messages in thread
From: Keqian Zhu @ 2020-12-17 1:49 UTC (permalink / raw)
To: Peter Maydell, Paolo Bonzini, Dr . David Alan Gilbert,
Andrew Jones, Peter Xu
Cc: jiangkunkun, qemu-devel, qemu-arm, Zenghui Yu, wanghaibin.wang,
Keqian Zhu
The parameters start and size are transfered from QEMU memory
emulation layer. It can promise that they are TARGET_PAGE_SIZE
aligned. However, KVM needs they are qemu_real_page_size aligned.
Though no caller breaks this aligned requirement currently, we'd
better add an explicit assert to avoid future breaking.
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
---
accel/kvm/kvm-all.c | 7 +++++++
1 file changed, 7 insertions(+)
---
v2
- Address Andrew's commment (Use assert instead of return err).
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index f6b16a8df8..73b195cc41 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -692,6 +692,10 @@ out:
#define KVM_CLEAR_LOG_ALIGN (qemu_real_host_page_size << KVM_CLEAR_LOG_SHIFT)
#define KVM_CLEAR_LOG_MASK (-KVM_CLEAR_LOG_ALIGN)
+/*
+ * As the granule of kvm dirty log is qemu_real_host_page_size,
+ * @start and @size are expected and restricted to align to it.
+ */
static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
uint64_t size)
{
@@ -701,6 +705,9 @@ static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
unsigned long *bmap_clear = NULL, psize = qemu_real_host_page_size;
int ret;
+ /* Make sure start and size are qemu_real_host_page_size aligned */
+ assert(QEMU_IS_ALIGNED(start | size, psize));
+
/*
* We need to extend either the start or the size or both to
* satisfy the KVM interface requirement. Firstly, do the start
--
2.23.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot
2020-12-17 1:49 ` [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot Keqian Zhu
@ 2020-12-17 12:18 ` Andrew Jones
2020-12-17 14:36 ` Peter Xu
` (2 subsequent siblings)
3 siblings, 0 replies; 22+ messages in thread
From: Andrew Jones @ 2020-12-17 12:18 UTC (permalink / raw)
To: Keqian Zhu
Cc: Peter Maydell, jiangkunkun, qemu-devel, Peter Xu,
Dr . David Alan Gilbert, qemu-arm, wanghaibin.wang, Zenghui Yu,
Paolo Bonzini
On Thu, Dec 17, 2020 at 09:49:41AM +0800, Keqian Zhu wrote:
> The parameters start and size are transfered from QEMU memory
> emulation layer. It can promise that they are TARGET_PAGE_SIZE
> aligned. However, KVM needs they are qemu_real_page_size aligned.
>
> Though no caller breaks this aligned requirement currently, we'd
> better add an explicit assert to avoid future breaking.
>
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> ---
> accel/kvm/kvm-all.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> ---
> v2
> - Address Andrew's commment (Use assert instead of return err).
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index f6b16a8df8..73b195cc41 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -692,6 +692,10 @@ out:
> #define KVM_CLEAR_LOG_ALIGN (qemu_real_host_page_size << KVM_CLEAR_LOG_SHIFT)
> #define KVM_CLEAR_LOG_MASK (-KVM_CLEAR_LOG_ALIGN)
>
> +/*
> + * As the granule of kvm dirty log is qemu_real_host_page_size,
> + * @start and @size are expected and restricted to align to it.
> + */
> static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
> uint64_t size)
> {
> @@ -701,6 +705,9 @@ static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
> unsigned long *bmap_clear = NULL, psize = qemu_real_host_page_size;
> int ret;
>
> + /* Make sure start and size are qemu_real_host_page_size aligned */
> + assert(QEMU_IS_ALIGNED(start | size, psize));
> +
> /*
> * We need to extend either the start or the size or both to
> * satisfy the KVM interface requirement. Firstly, do the start
> --
> 2.23.0
>
>
Reviewed-by: Andrew Jones <drjones@redhat.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot
2020-12-17 1:49 ` [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot Keqian Zhu
2020-12-17 12:18 ` Andrew Jones
@ 2020-12-17 14:36 ` Peter Xu
2021-02-01 15:14 ` Philippe Mathieu-Daudé
2021-03-09 13:48 ` Thomas Huth
3 siblings, 0 replies; 22+ messages in thread
From: Peter Xu @ 2020-12-17 14:36 UTC (permalink / raw)
To: Keqian Zhu
Cc: Peter Maydell, Andrew Jones, jiangkunkun, qemu-devel,
Dr . David Alan Gilbert, qemu-arm, wanghaibin.wang, Zenghui Yu,
Paolo Bonzini
On Thu, Dec 17, 2020 at 09:49:41AM +0800, Keqian Zhu wrote:
> The parameters start and size are transfered from QEMU memory
> emulation layer. It can promise that they are TARGET_PAGE_SIZE
> aligned. However, KVM needs they are qemu_real_page_size aligned.
>
> Though no caller breaks this aligned requirement currently, we'd
> better add an explicit assert to avoid future breaking.
>
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
Acked-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 0/2] accel: kvm: Some bugfixes for kvm dirty log
2020-12-17 1:49 [PATCH v2 0/2] accel: kvm: Some bugfixes for kvm dirty log Keqian Zhu
2020-12-17 1:49 ` [PATCH v2 1/2] accel: kvm: Fix memory waste under mismatch page size Keqian Zhu
2020-12-17 1:49 ` [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot Keqian Zhu
@ 2021-01-06 7:07 ` Keqian Zhu
2021-01-25 7:51 ` Keqian Zhu
2021-03-02 11:43 ` [PING] " Keqian Zhu
4 siblings, 0 replies; 22+ messages in thread
From: Keqian Zhu @ 2021-01-06 7:07 UTC (permalink / raw)
To: Peter Maydell, Paolo Bonzini, Dr . David Alan Gilbert
Cc: Andrew Jones, jiangkunkun, qemu-devel, Peter Xu, qemu-arm,
Zenghui Yu, wanghaibin.wang
Friendly ping ...
Hi, please queue this well reviewed series, Thanks :-)
Keqian
On 2020/12/17 9:49, Keqian Zhu wrote:
> Hi all,
>
> This series fixes memory waste and adds alignment check for unmatched
> qemu_real_host_page_size and TARGET_PAGE_SIZE.
>
> Thanks.
>
> Keqian Zhu (2):
> accel: kvm: Fix memory waste under mismatch page size
> accel: kvm: Add aligment assert for kvm_log_clear_one_slot
>
> accel/kvm/kvm-all.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 0/2] accel: kvm: Some bugfixes for kvm dirty log
2020-12-17 1:49 [PATCH v2 0/2] accel: kvm: Some bugfixes for kvm dirty log Keqian Zhu
` (2 preceding siblings ...)
2021-01-06 7:07 ` [PATCH v2 0/2] accel: kvm: Some bugfixes for kvm dirty log Keqian Zhu
@ 2021-01-25 7:51 ` Keqian Zhu
2021-02-01 13:07 ` Keqian Zhu
2021-03-02 11:43 ` [PING] " Keqian Zhu
4 siblings, 1 reply; 22+ messages in thread
From: Keqian Zhu @ 2021-01-25 7:51 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Peter Maydell, Andrew Jones, jiangkunkun,
Dr . David Alan Gilbert, Peter Xu, qemu-devel, qemu-arm,
Zenghui Yu, wanghaibin.wang
Hi Paolo,
Any suggestions on this patch series :-) ? Thanks,
Keqian
On 2020/12/17 9:49, Keqian Zhu wrote:
> Hi all,
>
> This series fixes memory waste and adds alignment check for unmatched
> qemu_real_host_page_size and TARGET_PAGE_SIZE.
>
> Thanks.
>
> Keqian Zhu (2):
> accel: kvm: Fix memory waste under mismatch page size
> accel: kvm: Add aligment assert for kvm_log_clear_one_slot
>
> accel/kvm/kvm-all.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 0/2] accel: kvm: Some bugfixes for kvm dirty log
2021-01-25 7:51 ` Keqian Zhu
@ 2021-02-01 13:07 ` Keqian Zhu
0 siblings, 0 replies; 22+ messages in thread
From: Keqian Zhu @ 2021-02-01 13:07 UTC (permalink / raw)
To: Paolo Bonzini, Peter Maydell, Dr . David Alan Gilbert
Cc: Andrew Jones, jiangkunkun, qemu-arm, qemu-devel, Peter Xu
Hi All,
Kindly Ping ;-) ...
On 2021/1/25 15:51, Keqian Zhu wrote:
> Hi Paolo,
>
> Any suggestions on this patch series :-) ? Thanks,
>
> Keqian
>
> On 2020/12/17 9:49, Keqian Zhu wrote:
>> Hi all,
>>
>> This series fixes memory waste and adds alignment check for unmatched
>> qemu_real_host_page_size and TARGET_PAGE_SIZE.
>>
>> Thanks.
>>
>> Keqian Zhu (2):
>> accel: kvm: Fix memory waste under mismatch page size
>> accel: kvm: Add aligment assert for kvm_log_clear_one_slot
>>
>> accel/kvm/kvm-all.c | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>
> .
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot
2020-12-17 1:49 ` [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot Keqian Zhu
2020-12-17 12:18 ` Andrew Jones
2020-12-17 14:36 ` Peter Xu
@ 2021-02-01 15:14 ` Philippe Mathieu-Daudé
2021-02-02 1:17 ` Keqian Zhu
2021-03-09 13:48 ` Thomas Huth
3 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-02-01 15:14 UTC (permalink / raw)
To: Keqian Zhu, Peter Maydell, Paolo Bonzini,
Dr . David Alan Gilbert, Andrew Jones, Peter Xu
Cc: Zenghui Yu, wanghaibin.wang, jiangkunkun, qemu-devel, qemu-arm
Hi,
On 12/17/20 2:49 AM, Keqian Zhu wrote:
> The parameters start and size are transfered from QEMU memory
> emulation layer. It can promise that they are TARGET_PAGE_SIZE
> aligned. However, KVM needs they are qemu_real_page_size aligned.
>
> Though no caller breaks this aligned requirement currently, we'd
> better add an explicit assert to avoid future breaking.
>
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> ---
> accel/kvm/kvm-all.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> ---
> v2
> - Address Andrew's commment (Use assert instead of return err).
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index f6b16a8df8..73b195cc41 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -692,6 +692,10 @@ out:
> #define KVM_CLEAR_LOG_ALIGN (qemu_real_host_page_size << KVM_CLEAR_LOG_SHIFT)
> #define KVM_CLEAR_LOG_MASK (-KVM_CLEAR_LOG_ALIGN)
>
> +/*
> + * As the granule of kvm dirty log is qemu_real_host_page_size,
> + * @start and @size are expected and restricted to align to it.
> + */
> static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
> uint64_t size)
> {
> @@ -701,6 +705,9 @@ static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
> unsigned long *bmap_clear = NULL, psize = qemu_real_host_page_size;
> int ret;
>
> + /* Make sure start and size are qemu_real_host_page_size aligned */
> + assert(QEMU_IS_ALIGNED(start | size, psize));
Why not return an error instead of aborting the VM?
> /*
> * We need to extend either the start or the size or both to
> * satisfy the KVM interface requirement. Firstly, do the start
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot
2021-02-01 15:14 ` Philippe Mathieu-Daudé
@ 2021-02-02 1:17 ` Keqian Zhu
0 siblings, 0 replies; 22+ messages in thread
From: Keqian Zhu @ 2021-02-02 1:17 UTC (permalink / raw)
To: Philippe Mathieu-Daudé,
Peter Maydell, Paolo Bonzini, Dr . David Alan Gilbert,
Andrew Jones, Peter Xu
Cc: Zenghui Yu, wanghaibin.wang, jiangkunkun, qemu-devel, qemu-arm
Hi Philippe,
On 2021/2/1 23:14, Philippe Mathieu-Daudé wrote:
> Hi,
>
> On 12/17/20 2:49 AM, Keqian Zhu wrote:
>> The parameters start and size are transfered from QEMU memory
>> emulation layer. It can promise that they are TARGET_PAGE_SIZE
>> aligned. However, KVM needs they are qemu_real_page_size aligned.
>>
>> Though no caller breaks this aligned requirement currently, we'd
>> better add an explicit assert to avoid future breaking.
>>
>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>> ---
>> accel/kvm/kvm-all.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> ---
>> v2
>> - Address Andrew's commment (Use assert instead of return err).
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index f6b16a8df8..73b195cc41 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -692,6 +692,10 @@ out:
>> #define KVM_CLEAR_LOG_ALIGN (qemu_real_host_page_size << KVM_CLEAR_LOG_SHIFT)
>> #define KVM_CLEAR_LOG_MASK (-KVM_CLEAR_LOG_ALIGN)
>>
>> +/*
>> + * As the granule of kvm dirty log is qemu_real_host_page_size,
>> + * @start and @size are expected and restricted to align to it.
>> + */
>> static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
>> uint64_t size)
>> {
>> @@ -701,6 +705,9 @@ static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
>> unsigned long *bmap_clear = NULL, psize = qemu_real_host_page_size;
>> int ret;
>>
>> + /* Make sure start and size are qemu_real_host_page_size aligned */
>> + assert(QEMU_IS_ALIGNED(start | size, psize));
>
> Why not return an error instead of aborting the VM?
Yep, I return an error in v1. As suggested by Peter Xu: "Returning -EINVAL is the same as abort() currently - it'll just abort() at
kvm_log_clear() instead."
>
>> /*
>> * We need to extend either the start or the size or both to
>> * satisfy the KVM interface requirement. Firstly, do the start
>>
>
> .
>
Thanks for review.
Keqian.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PING] accel: kvm: Some bugfixes for kvm dirty log
2020-12-17 1:49 [PATCH v2 0/2] accel: kvm: Some bugfixes for kvm dirty log Keqian Zhu
` (3 preceding siblings ...)
2021-01-25 7:51 ` Keqian Zhu
@ 2021-03-02 11:43 ` Keqian Zhu
2021-03-02 13:14 ` Paolo Bonzini
4 siblings, 1 reply; 22+ messages in thread
From: Keqian Zhu @ 2021-03-02 11:43 UTC (permalink / raw)
To: Peter Maydell, Paolo Bonzini, Dr . David Alan Gilbert,
Andrew Jones, Peter Xu
Cc: Zenghui Yu, wanghaibin.wang, qemu-arm, jiangkunkun, qemu-devel
Hi,
This patch is still not queued. Who can help to do this? Thanks :)
Keqian
On 2020/12/17 9:49, Keqian Zhu wrote:
> Hi all,
>
> This series fixes memory waste and adds alignment check for unmatched
> qemu_real_host_page_size and TARGET_PAGE_SIZE.
>
> Thanks.
>
> Keqian Zhu (2):
> accel: kvm: Fix memory waste under mismatch page size
> accel: kvm: Add aligment assert for kvm_log_clear_one_slot
>
> accel/kvm/kvm-all.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PING] accel: kvm: Some bugfixes for kvm dirty log
2021-03-02 11:43 ` [PING] " Keqian Zhu
@ 2021-03-02 13:14 ` Paolo Bonzini
0 siblings, 0 replies; 22+ messages in thread
From: Paolo Bonzini @ 2021-03-02 13:14 UTC (permalink / raw)
To: Keqian Zhu, Peter Maydell, Dr . David Alan Gilbert, Andrew Jones,
Peter Xu
Cc: Zenghui Yu, wanghaibin.wang, qemu-arm, jiangkunkun, qemu-devel
On 02/03/21 12:43, Keqian Zhu wrote:
> Hi,
>
> This patch is still not queued. Who can help to do this? Thanks :)
>
> Keqian
>
> On 2020/12/17 9:49, Keqian Zhu wrote:
>> Hi all,
>>
>> This series fixes memory waste and adds alignment check for unmatched
>> qemu_real_host_page_size and TARGET_PAGE_SIZE.
>>
>> Thanks.
>>
>> Keqian Zhu (2):
>> accel: kvm: Fix memory waste under mismatch page size
>> accel: kvm: Add aligment assert for kvm_log_clear_one_slot
>>
>> accel/kvm/kvm-all.c | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>
Queued, thanks.
Paolo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot
2020-12-17 1:49 ` [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot Keqian Zhu
` (2 preceding siblings ...)
2021-02-01 15:14 ` Philippe Mathieu-Daudé
@ 2021-03-09 13:48 ` Thomas Huth
2021-03-09 14:05 ` Keqian Zhu
3 siblings, 1 reply; 22+ messages in thread
From: Thomas Huth @ 2021-03-09 13:48 UTC (permalink / raw)
To: Keqian Zhu, Peter Maydell, Paolo Bonzini,
Dr . David Alan Gilbert, Andrew Jones, Peter Xu
Cc: Zenghui Yu, wanghaibin.wang, jiangkunkun, qemu-devel, qemu-arm
On 17/12/2020 02.49, Keqian Zhu wrote:
> The parameters start and size are transfered from QEMU memory
> emulation layer. It can promise that they are TARGET_PAGE_SIZE
> aligned. However, KVM needs they are qemu_real_page_size aligned.
>
> Though no caller breaks this aligned requirement currently, we'd
> better add an explicit assert to avoid future breaking.
>
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> ---
> accel/kvm/kvm-all.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> ---
> v2
> - Address Andrew's commment (Use assert instead of return err).
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index f6b16a8df8..73b195cc41 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -692,6 +692,10 @@ out:
> #define KVM_CLEAR_LOG_ALIGN (qemu_real_host_page_size << KVM_CLEAR_LOG_SHIFT)
> #define KVM_CLEAR_LOG_MASK (-KVM_CLEAR_LOG_ALIGN)
>
> +/*
> + * As the granule of kvm dirty log is qemu_real_host_page_size,
> + * @start and @size are expected and restricted to align to it.
> + */
> static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
> uint64_t size)
> {
> @@ -701,6 +705,9 @@ static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
> unsigned long *bmap_clear = NULL, psize = qemu_real_host_page_size;
> int ret;
>
> + /* Make sure start and size are qemu_real_host_page_size aligned */
> + assert(QEMU_IS_ALIGNED(start | size, psize));
Sorry, but that was a bad idea: It triggers and kills my Centos 6 VM:
$ qemu-system-x86_64 -accel kvm -hda ~/virt/images/centos6.qcow2 -m 1G
qemu-system-x86_64: ../../devel/qemu/accel/kvm/kvm-all.c:690:
kvm_log_clear_one_slot: Assertion `QEMU_IS_ALIGNED(start | size, psize)' failed.
Aborted (core dumped)
Can we please revert this patch?
Thomas
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot
2021-03-09 13:48 ` Thomas Huth
@ 2021-03-09 14:05 ` Keqian Zhu
2021-03-09 14:45 ` Thomas Huth
0 siblings, 1 reply; 22+ messages in thread
From: Keqian Zhu @ 2021-03-09 14:05 UTC (permalink / raw)
To: Thomas Huth, Peter Maydell, Paolo Bonzini,
Dr . David Alan Gilbert, Andrew Jones, Peter Xu
Cc: Zenghui Yu, wanghaibin.wang, jiangkunkun, qemu-devel, qemu-arm
On 2021/3/9 21:48, Thomas Huth wrote:
> On 17/12/2020 02.49, Keqian Zhu wrote:
>> The parameters start and size are transfered from QEMU memory
>> emulation layer. It can promise that they are TARGET_PAGE_SIZE
>> aligned. However, KVM needs they are qemu_real_page_size aligned.
>>
>> Though no caller breaks this aligned requirement currently, we'd
>> better add an explicit assert to avoid future breaking.
>>
>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>> ---
>> accel/kvm/kvm-all.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> ---
>> v2
>> - Address Andrew's commment (Use assert instead of return err).
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index f6b16a8df8..73b195cc41 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -692,6 +692,10 @@ out:
>> #define KVM_CLEAR_LOG_ALIGN (qemu_real_host_page_size << KVM_CLEAR_LOG_SHIFT)
>> #define KVM_CLEAR_LOG_MASK (-KVM_CLEAR_LOG_ALIGN)
>> +/*
>> + * As the granule of kvm dirty log is qemu_real_host_page_size,
>> + * @start and @size are expected and restricted to align to it.
>> + */
>> static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
>> uint64_t size)
>> {
>> @@ -701,6 +705,9 @@ static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
>> unsigned long *bmap_clear = NULL, psize = qemu_real_host_page_size;
>> int ret;
>> + /* Make sure start and size are qemu_real_host_page_size aligned */
>> + assert(QEMU_IS_ALIGNED(start | size, psize));
>
> Sorry, but that was a bad idea: It triggers and kills my Centos 6 VM:
>
> $ qemu-system-x86_64 -accel kvm -hda ~/virt/images/centos6.qcow2 -m 1G
> qemu-system-x86_64: ../../devel/qemu/accel/kvm/kvm-all.c:690: kvm_log_clear_one_slot: Assertion `QEMU_IS_ALIGNED(start | size, psize)' failed.
> Aborted (core dumped)
Hi Thomas,
I think this patch is ok, maybe it trigger a potential bug?
Thanks,
Keqian
>
> Can we please revert this patch?
>
> Thomas
>
> .
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot
2021-03-09 14:05 ` Keqian Zhu
@ 2021-03-09 14:45 ` Thomas Huth
2021-03-09 14:57 ` Dr. David Alan Gilbert
2021-03-09 15:11 ` zhukeqian
0 siblings, 2 replies; 22+ messages in thread
From: Thomas Huth @ 2021-03-09 14:45 UTC (permalink / raw)
To: Keqian Zhu, Peter Maydell, Paolo Bonzini,
Dr . David Alan Gilbert, Andrew Jones, Peter Xu, Gerd Hoffmann
Cc: Zenghui Yu, wanghaibin.wang, jiangkunkun, qemu-devel, qemu-arm
On 09/03/2021 15.05, Keqian Zhu wrote:
>
>
> On 2021/3/9 21:48, Thomas Huth wrote:
>> On 17/12/2020 02.49, Keqian Zhu wrote:
>>> The parameters start and size are transfered from QEMU memory
>>> emulation layer. It can promise that they are TARGET_PAGE_SIZE
>>> aligned. However, KVM needs they are qemu_real_page_size aligned.
>>>
>>> Though no caller breaks this aligned requirement currently, we'd
>>> better add an explicit assert to avoid future breaking.
>>>
>>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>>> ---
>>> accel/kvm/kvm-all.c | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>>
>>> ---
>>> v2
>>> - Address Andrew's commment (Use assert instead of return err).
>>>
>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>> index f6b16a8df8..73b195cc41 100644
>>> --- a/accel/kvm/kvm-all.c
>>> +++ b/accel/kvm/kvm-all.c
>>> @@ -692,6 +692,10 @@ out:
>>> #define KVM_CLEAR_LOG_ALIGN (qemu_real_host_page_size << KVM_CLEAR_LOG_SHIFT)
>>> #define KVM_CLEAR_LOG_MASK (-KVM_CLEAR_LOG_ALIGN)
>>> +/*
>>> + * As the granule of kvm dirty log is qemu_real_host_page_size,
>>> + * @start and @size are expected and restricted to align to it.
>>> + */
>>> static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
>>> uint64_t size)
>>> {
>>> @@ -701,6 +705,9 @@ static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
>>> unsigned long *bmap_clear = NULL, psize = qemu_real_host_page_size;
>>> int ret;
>>> + /* Make sure start and size are qemu_real_host_page_size aligned */
>>> + assert(QEMU_IS_ALIGNED(start | size, psize));
>>
>> Sorry, but that was a bad idea: It triggers and kills my Centos 6 VM:
>>
>> $ qemu-system-x86_64 -accel kvm -hda ~/virt/images/centos6.qcow2 -m 1G
>> qemu-system-x86_64: ../../devel/qemu/accel/kvm/kvm-all.c:690: kvm_log_clear_one_slot: Assertion `QEMU_IS_ALIGNED(start | size, psize)' failed.
>> Aborted (core dumped)
> Hi Thomas,
>
> I think this patch is ok, maybe it trigger a potential bug?
Well, sure, there is either a bug somewhere else or in this new code. But it's certainly not normal that the assert() triggers, is it?
FWIW, here's a backtrace:
#0 0x00007ffff2c1584f in raise () at /lib64/libc.so.6
#1 0x00007ffff2bffc45 in abort () at /lib64/libc.so.6
#2 0x00007ffff2bffb19 in _nl_load_domain.cold.0 () at /lib64/libc.so.6
#3 0x00007ffff2c0de36 in .annobin_assert.c_end () at /lib64/libc.so.6
#4 0x0000555555ba25f3 in kvm_log_clear_one_slot
(size=6910080, start=0, as_id=0, mem=0x555556e1ee00)
at ../../devel/qemu/accel/kvm/kvm-all.c:691
#5 0x0000555555ba25f3 in kvm_physical_log_clear
(section=0x7fffffffd0b0, section=0x7fffffffd0b0, kml=0x555556dbaac0)
at ../../devel/qemu/accel/kvm/kvm-all.c:843
#6 0x0000555555ba25f3 in kvm_log_clear (listener=0x555556dbaac0, section=0x7fffffffd0b0)
at ../../devel/qemu/accel/kvm/kvm-all.c:1253
#7 0x0000555555b023d8 in memory_region_clear_dirty_bitmap
(mr=mr@entry=0x5555573394c0, start=start@entry=0, len=len@entry=6910080)
at ../../devel/qemu/softmmu/memory.c:2132
#8 0x0000555555b313d9 in cpu_physical_memory_snapshot_and_clear_dirty
(mr=mr@entry=0x5555573394c0, offset=offset@entry=0, length=length@entry=6910080, client=client@entry=0) at ../../devel/qemu/softmmu/physmem.c:1109
#9 0x0000555555b02483 in memory_region_snapshot_and_clear_dirty
(mr=mr@entry=0x5555573394c0, addr=addr@entry=0, size=size@entry=6910080, client=client@entry=0)
at ../../devel/qemu/softmmu/memory.c:2146
#10 0x0000555555babe99 in vga_draw_graphic (full_update=0, s=0x5555573394b0)
at ../../devel/qemu/hw/display/vga.c:1661
#11 0x0000555555babe99 in vga_update_display (opaque=0x5555573394b0)
at ../../devel/qemu/hw/display/vga.c:1784
#12 0x0000555555babe99 in vga_update_display (opaque=0x5555573394b0)
at ../../devel/qemu/hw/display/vga.c:1757
#13 0x00005555558ddd32 in graphic_hw_update (con=0x555556a11800)
at ../../devel/qemu/ui/console.c:279
#14 0x00005555558dccd2 in dpy_refresh (s=0x555556c17da0) at ../../devel/qemu/ui/console.c:1742
#15 0x00005555558dccd2 in gui_update (opaque=opaque@entry=0x555556c17da0)
at ../../devel/qemu/ui/console.c:209
#16 0x0000555555dbd520 in timerlist_run_timers (timer_list=0x555556937c50)
at ../../devel/qemu/util/qemu-timer.c:574
#17 0x0000555555dbd520 in timerlist_run_timers (timer_list=0x555556937c50)
at ../../devel/qemu/util/qemu-timer.c:499
#18 0x0000555555dbd74a in qemu_clock_run_timers (type=<optimized out>)
at ../../devel/qemu/util/qemu-timer.c:670
#19 0x0000555555dbd74a in qemu_clock_run_all_timers () at ../../devel/qemu/util/qemu-timer.c:670
Looks like something in the vga code calls this with size=6910080
and thus triggers the alignment assertion?
Thomas
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot
2021-03-09 14:45 ` Thomas Huth
@ 2021-03-09 14:57 ` Dr. David Alan Gilbert
2021-03-09 16:08 ` Peter Xu
2021-03-09 16:20 ` Thomas Huth
2021-03-09 15:11 ` zhukeqian
1 sibling, 2 replies; 22+ messages in thread
From: Dr. David Alan Gilbert @ 2021-03-09 14:57 UTC (permalink / raw)
To: Thomas Huth
Cc: Peter Maydell, Andrew Jones, jiangkunkun, qemu-devel, Peter Xu,
qemu-arm, Gerd Hoffmann, wanghaibin.wang, Zenghui Yu,
Paolo Bonzini, Keqian Zhu
* Thomas Huth (thuth@redhat.com) wrote:
> On 09/03/2021 15.05, Keqian Zhu wrote:
> >
> >
> > On 2021/3/9 21:48, Thomas Huth wrote:
> > > On 17/12/2020 02.49, Keqian Zhu wrote:
> > > > The parameters start and size are transfered from QEMU memory
> > > > emulation layer. It can promise that they are TARGET_PAGE_SIZE
> > > > aligned. However, KVM needs they are qemu_real_page_size aligned.
> > > >
> > > > Though no caller breaks this aligned requirement currently, we'd
> > > > better add an explicit assert to avoid future breaking.
> > > >
> > > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> > > > ---
> > > > accel/kvm/kvm-all.c | 7 +++++++
> > > > 1 file changed, 7 insertions(+)
> > > >
> > > > ---
> > > > v2
> > > > - Address Andrew's commment (Use assert instead of return err).
> > > >
> > > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > > > index f6b16a8df8..73b195cc41 100644
> > > > --- a/accel/kvm/kvm-all.c
> > > > +++ b/accel/kvm/kvm-all.c
> > > > @@ -692,6 +692,10 @@ out:
> > > > #define KVM_CLEAR_LOG_ALIGN (qemu_real_host_page_size << KVM_CLEAR_LOG_SHIFT)
> > > > #define KVM_CLEAR_LOG_MASK (-KVM_CLEAR_LOG_ALIGN)
> > > > +/*
> > > > + * As the granule of kvm dirty log is qemu_real_host_page_size,
> > > > + * @start and @size are expected and restricted to align to it.
> > > > + */
> > > > static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
> > > > uint64_t size)
> > > > {
> > > > @@ -701,6 +705,9 @@ static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
> > > > unsigned long *bmap_clear = NULL, psize = qemu_real_host_page_size;
> > > > int ret;
> > > > + /* Make sure start and size are qemu_real_host_page_size aligned */
> > > > + assert(QEMU_IS_ALIGNED(start | size, psize));
> > >
> > > Sorry, but that was a bad idea: It triggers and kills my Centos 6 VM:
> > >
> > > $ qemu-system-x86_64 -accel kvm -hda ~/virt/images/centos6.qcow2 -m 1G
> > > qemu-system-x86_64: ../../devel/qemu/accel/kvm/kvm-all.c:690: kvm_log_clear_one_slot: Assertion `QEMU_IS_ALIGNED(start | size, psize)' failed.
> > > Aborted (core dumped)
> > Hi Thomas,
> >
> > I think this patch is ok, maybe it trigger a potential bug?
>
> Well, sure, there is either a bug somewhere else or in this new code. But it's certainly not normal that the assert() triggers, is it?
>
> FWIW, here's a backtrace:
>
> #0 0x00007ffff2c1584f in raise () at /lib64/libc.so.6
> #1 0x00007ffff2bffc45 in abort () at /lib64/libc.so.6
> #2 0x00007ffff2bffb19 in _nl_load_domain.cold.0 () at /lib64/libc.so.6
> #3 0x00007ffff2c0de36 in .annobin_assert.c_end () at /lib64/libc.so.6
> #4 0x0000555555ba25f3 in kvm_log_clear_one_slot
> (size=6910080, start=0, as_id=0, mem=0x555556e1ee00)
> at ../../devel/qemu/accel/kvm/kvm-all.c:691
> #5 0x0000555555ba25f3 in kvm_physical_log_clear
> (section=0x7fffffffd0b0, section=0x7fffffffd0b0, kml=0x555556dbaac0)
> at ../../devel/qemu/accel/kvm/kvm-all.c:843
> #6 0x0000555555ba25f3 in kvm_log_clear (listener=0x555556dbaac0, section=0x7fffffffd0b0)
> at ../../devel/qemu/accel/kvm/kvm-all.c:1253
> #7 0x0000555555b023d8 in memory_region_clear_dirty_bitmap
> (mr=mr@entry=0x5555573394c0, start=start@entry=0, len=len@entry=6910080)
> at ../../devel/qemu/softmmu/memory.c:2132
> #8 0x0000555555b313d9 in cpu_physical_memory_snapshot_and_clear_dirty
> (mr=mr@entry=0x5555573394c0, offset=offset@entry=0, length=length@entry=6910080, client=client@entry=0) at ../../devel/qemu/softmmu/physmem.c:1109
> #9 0x0000555555b02483 in memory_region_snapshot_and_clear_dirty
> (mr=mr@entry=0x5555573394c0, addr=addr@entry=0, size=size@entry=6910080, client=client@entry=0)
> at ../../devel/qemu/softmmu/memory.c:2146
Could you please figure out which memory region this is?
WTH is that size? Is that really the problem that the size is just
crazy?
Dave
> #10 0x0000555555babe99 in vga_draw_graphic (full_update=0, s=0x5555573394b0)
> at ../../devel/qemu/hw/display/vga.c:1661
> #11 0x0000555555babe99 in vga_update_display (opaque=0x5555573394b0)
> at ../../devel/qemu/hw/display/vga.c:1784
> #12 0x0000555555babe99 in vga_update_display (opaque=0x5555573394b0)
> at ../../devel/qemu/hw/display/vga.c:1757
> #13 0x00005555558ddd32 in graphic_hw_update (con=0x555556a11800)
> at ../../devel/qemu/ui/console.c:279
> #14 0x00005555558dccd2 in dpy_refresh (s=0x555556c17da0) at ../../devel/qemu/ui/console.c:1742
> #15 0x00005555558dccd2 in gui_update (opaque=opaque@entry=0x555556c17da0)
> at ../../devel/qemu/ui/console.c:209
> #16 0x0000555555dbd520 in timerlist_run_timers (timer_list=0x555556937c50)
> at ../../devel/qemu/util/qemu-timer.c:574
> #17 0x0000555555dbd520 in timerlist_run_timers (timer_list=0x555556937c50)
> at ../../devel/qemu/util/qemu-timer.c:499
> #18 0x0000555555dbd74a in qemu_clock_run_timers (type=<optimized out>)
> at ../../devel/qemu/util/qemu-timer.c:670
> #19 0x0000555555dbd74a in qemu_clock_run_all_timers () at ../../devel/qemu/util/qemu-timer.c:670
>
> Looks like something in the vga code calls this with size=6910080
> and thus triggers the alignment assertion?
>
> Thomas
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot
2021-03-09 14:45 ` Thomas Huth
2021-03-09 14:57 ` Dr. David Alan Gilbert
@ 2021-03-09 15:11 ` zhukeqian
1 sibling, 0 replies; 22+ messages in thread
From: zhukeqian @ 2021-03-09 15:11 UTC (permalink / raw)
To: Thomas Huth, Peter Maydell, Paolo Bonzini,
Dr . David Alan Gilbert, Andrew Jones, Peter Xu, Gerd Hoffmann
Cc: yuzenghui, Wanghaibin (D), jiangkunkun, qemu-devel, qemu-arm
[-- Attachment #1: Type: text/plain, Size: 5056 bytes --]
Thanks for your bug report. I was just off work, will dig into it tomorrow. thanks :)
Keqian
On 09/03/2021 15.05, Keqian Zhu wrote:
>
>
> On 2021/3/9 21:48, Thomas Huth wrote:
>> On 17/12/2020 02.49, Keqian Zhu wrote:
>>> The parameters start and size are transfered from QEMU memory
>>> emulation layer. It can promise that they are TARGET_PAGE_SIZE
>>> aligned. However, KVM needs they are qemu_real_page_size aligned.
>>>
>>> Though no caller breaks this aligned requirement currently, we'd
>>> better add an explicit assert to avoid future breaking.
>>>
>>> Signed-off-by: Keqian Zhu < zhukeqian1@huawei.com<mailto:zhukeqian1@huawei.com>>
>>> ---
>>> accel/kvm/kvm-all.c | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>>
>>> ---
>>> v2
>>> - Address Andrew's commment (Use assert instead of return err).
>>>
>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>> index f6b16a8df8..73b195cc41 100644
>>> --- a/accel/kvm/kvm-all.c
>>> +++ b/accel/kvm/kvm-all.c
>>> @@ -692,6 +692,10 @@ out:
>>> #define KVM_CLEAR_LOG_ALIGN (qemu_real_host_page_size << KVM_CLEAR_LOG_SHIFT)
>>> #define KVM_CLEAR_LOG_MASK (-KVM_CLEAR_LOG_ALIGN)
>>> +/*
>>> + * As the granule of kvm dirty log is qemu_real_host_page_size,
>>> + * @start and @size are expected and restricted to align to it.
>>> + */
>>> static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
>>> uint64_t size)
>>> {
>>> @@ -701,6 +705,9 @@ static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
>>> unsigned long *bmap_clear = NULL, psize = qemu_real_host_page_size;
>>> int ret;
>>> + /* Make sure start and size are qemu_real_host_page_size aligned */
>>> + assert(QEMU_IS_ALIGNED(start | size, psize));
>>
>> Sorry, but that was a bad idea: It triggers and kills my Centos 6 VM:
>>
>> $ qemu-system-x86_64 -accel kvm -hda ~/virt/images/centos6.qcow2 -m 1G
>> qemu-system-x86_64: ../../devel/qemu/accel/kvm/kvm-all.c:690: kvm_log_clear_one_slot: Assertion `QEMU_IS_ALIGNED(start | size, psize)' failed.
>> Aborted (core dumped)
> Hi Thomas,
>
> I think this patch is ok, maybe it trigger a potential bug?
Well, sure, there is either a bug somewhere else or in this new code. But it's certainly not normal that the assert() triggers, is it?
FWIW, here's a backtrace:
#0 0x00007ffff2c1584f in raise () at /lib64/libc.so.6
#1 0x00007ffff2bffc45 in abort () at /lib64/libc.so.6
#2 0x00007ffff2bffb19 in _nl_load_domain.cold.0 () at /lib64/libc.so.6
#3 0x00007ffff2c0de36 in .annobin_assert.c_end () at /lib64/libc.so.6
#4 0x0000555555ba25f3 in kvm_log_clear_one_slot
(size=6910080, start=0, as_id=0, mem=0x555556e1ee00)
at ../../devel/qemu/accel/kvm/kvm-all.c:691
#5 0x0000555555ba25f3 in kvm_physical_log_clear
(section=0x7fffffffd0b0, section=0x7fffffffd0b0, kml=0x555556dbaac0)
at ../../devel/qemu/accel/kvm/kvm-all.c:843
#6 0x0000555555ba25f3 in kvm_log_clear (listener=0x555556dbaac0, section=0x7fffffffd0b0)
at ../../devel/qemu/accel/kvm/kvm-all.c:1253
#7 0x0000555555b023d8 in memory_region_clear_dirty_bitmap
(mr=mr@entry=0x5555573394c0, start=start@entry=0, len=len@entry=6910080)
at ../../devel/qemu/softmmu/memory.c:2132
#8 0x0000555555b313d9 in cpu_physical_memory_snapshot_and_clear_dirty
(mr=mr@entry=0x5555573394c0, offset=offset@entry=0, length=length@entry=6910080, client=client@entry=0) at ../../devel/qemu/softmmu/physmem.c:1109
#9 0x0000555555b02483 in memory_region_snapshot_and_clear_dirty
(mr=mr@entry=0x5555573394c0, addr=addr@entry=0, size=size@entry=6910080, client=client@entry=0)
at ../../devel/qemu/softmmu/memory.c:2146
#10 0x0000555555babe99 in vga_draw_graphic (full_update=0, s=0x5555573394b0)
at ../../devel/qemu/hw/display/vga.c:1661
#11 0x0000555555babe99 in vga_update_display (opaque=0x5555573394b0)
at ../../devel/qemu/hw/display/vga.c:1784
#12 0x0000555555babe99 in vga_update_display (opaque=0x5555573394b0)
at ../../devel/qemu/hw/display/vga.c:1757
#13 0x00005555558ddd32 in graphic_hw_update (con=0x555556a11800)
at ../../devel/qemu/ui/console.c:279
#14 0x00005555558dccd2 in dpy_refresh (s=0x555556c17da0) at ../../devel/qemu/ui/console.c:1742
#15 0x00005555558dccd2 in gui_update (opaque=opaque@entry=0x555556c17da0)
at ../../devel/qemu/ui/console.c:209
#16 0x0000555555dbd520 in timerlist_run_timers (timer_list=0x555556937c50)
at ../../devel/qemu/util/qemu-timer.c:574
#17 0x0000555555dbd520 in timerlist_run_timers (timer_list=0x555556937c50)
at ../../devel/qemu/util/qemu-timer.c:499
#18 0x0000555555dbd74a in qemu_clock_run_timers (type=<optimized out>)
at ../../devel/qemu/util/qemu-timer.c:670
#19 0x0000555555dbd74a in qemu_clock_run_all_timers () at ../../devel/qemu/util/qemu-timer.c:670
Looks like something in the vga code calls this with size=6910080
and thus triggers the alignment assertion?
Thomas
[-- Attachment #2: Type: text/html, Size: 7199 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot
2021-03-09 14:57 ` Dr. David Alan Gilbert
@ 2021-03-09 16:08 ` Peter Xu
2021-03-10 1:57 ` Keqian Zhu
2021-03-09 16:20 ` Thomas Huth
1 sibling, 1 reply; 22+ messages in thread
From: Peter Xu @ 2021-03-09 16:08 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Peter Maydell, Thomas Huth, jiangkunkun, Andrew Jones,
qemu-devel, qemu-arm, Gerd Hoffmann, wanghaibin.wang, Zenghui Yu,
Paolo Bonzini, Keqian Zhu
On Tue, Mar 09, 2021 at 02:57:53PM +0000, Dr. David Alan Gilbert wrote:
> * Thomas Huth (thuth@redhat.com) wrote:
> > On 09/03/2021 15.05, Keqian Zhu wrote:
> > >
> > >
> > > On 2021/3/9 21:48, Thomas Huth wrote:
> > > > On 17/12/2020 02.49, Keqian Zhu wrote:
> > > > > The parameters start and size are transfered from QEMU memory
> > > > > emulation layer. It can promise that they are TARGET_PAGE_SIZE
> > > > > aligned. However, KVM needs they are qemu_real_page_size aligned.
> > > > >
> > > > > Though no caller breaks this aligned requirement currently, we'd
> > > > > better add an explicit assert to avoid future breaking.
> > > > >
> > > > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> > > > > ---
> > > > > accel/kvm/kvm-all.c | 7 +++++++
> > > > > 1 file changed, 7 insertions(+)
> > > > >
> > > > > ---
> > > > > v2
> > > > > - Address Andrew's commment (Use assert instead of return err).
> > > > >
> > > > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > > > > index f6b16a8df8..73b195cc41 100644
> > > > > --- a/accel/kvm/kvm-all.c
> > > > > +++ b/accel/kvm/kvm-all.c
> > > > > @@ -692,6 +692,10 @@ out:
> > > > > #define KVM_CLEAR_LOG_ALIGN (qemu_real_host_page_size << KVM_CLEAR_LOG_SHIFT)
> > > > > #define KVM_CLEAR_LOG_MASK (-KVM_CLEAR_LOG_ALIGN)
> > > > > +/*
> > > > > + * As the granule of kvm dirty log is qemu_real_host_page_size,
> > > > > + * @start and @size are expected and restricted to align to it.
> > > > > + */
> > > > > static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
> > > > > uint64_t size)
> > > > > {
> > > > > @@ -701,6 +705,9 @@ static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
> > > > > unsigned long *bmap_clear = NULL, psize = qemu_real_host_page_size;
> > > > > int ret;
> > > > > + /* Make sure start and size are qemu_real_host_page_size aligned */
> > > > > + assert(QEMU_IS_ALIGNED(start | size, psize));
> > > >
> > > > Sorry, but that was a bad idea: It triggers and kills my Centos 6 VM:
> > > >
> > > > $ qemu-system-x86_64 -accel kvm -hda ~/virt/images/centos6.qcow2 -m 1G
> > > > qemu-system-x86_64: ../../devel/qemu/accel/kvm/kvm-all.c:690: kvm_log_clear_one_slot: Assertion `QEMU_IS_ALIGNED(start | size, psize)' failed.
> > > > Aborted (core dumped)
> > > Hi Thomas,
> > >
> > > I think this patch is ok, maybe it trigger a potential bug?
> >
> > Well, sure, there is either a bug somewhere else or in this new code. But it's certainly not normal that the assert() triggers, is it?
> >
> > FWIW, here's a backtrace:
> >
> > #0 0x00007ffff2c1584f in raise () at /lib64/libc.so.6
> > #1 0x00007ffff2bffc45 in abort () at /lib64/libc.so.6
> > #2 0x00007ffff2bffb19 in _nl_load_domain.cold.0 () at /lib64/libc.so.6
> > #3 0x00007ffff2c0de36 in .annobin_assert.c_end () at /lib64/libc.so.6
> > #4 0x0000555555ba25f3 in kvm_log_clear_one_slot
> > (size=6910080, start=0, as_id=0, mem=0x555556e1ee00)
> > at ../../devel/qemu/accel/kvm/kvm-all.c:691
> > #5 0x0000555555ba25f3 in kvm_physical_log_clear
> > (section=0x7fffffffd0b0, section=0x7fffffffd0b0, kml=0x555556dbaac0)
> > at ../../devel/qemu/accel/kvm/kvm-all.c:843
> > #6 0x0000555555ba25f3 in kvm_log_clear (listener=0x555556dbaac0, section=0x7fffffffd0b0)
> > at ../../devel/qemu/accel/kvm/kvm-all.c:1253
> > #7 0x0000555555b023d8 in memory_region_clear_dirty_bitmap
> > (mr=mr@entry=0x5555573394c0, start=start@entry=0, len=len@entry=6910080)
> > at ../../devel/qemu/softmmu/memory.c:2132
> > #8 0x0000555555b313d9 in cpu_physical_memory_snapshot_and_clear_dirty
> > (mr=mr@entry=0x5555573394c0, offset=offset@entry=0, length=length@entry=6910080, client=client@entry=0) at ../../devel/qemu/softmmu/physmem.c:1109
> > #9 0x0000555555b02483 in memory_region_snapshot_and_clear_dirty
> > (mr=mr@entry=0x5555573394c0, addr=addr@entry=0, size=size@entry=6910080, client=client@entry=0)
> > at ../../devel/qemu/softmmu/memory.c:2146
>
> Could you please figure out which memory region this is?
> WTH is that size? Is that really the problem that the size is just
> crazy?
It seems vga_draw_graphic() could call memory_region_snapshot_and_clear_dirty()
with not-page-aligned size. cpu_physical_memory_snapshot_and_clear_dirty()
actually took care of most of it on alignment, however still the "length"
parameter got passed in without alignment check or so.
Cc Gerd too.
I'm not sure how many use cases are there like this.. if there're a lot maybe
we can indeed drop this assert patch, but instead in kvm_log_clear_one_slot()
we should ALIGN_DOWN the size to smallest host page size. Say, if we need to
clear dirty bit for range (0, 0x1020), we should only clean (0, 0x1000) since
there can still be dirty data on range (0x1020, 0x2000).
Thanks,
>
> Dave
>
> > #10 0x0000555555babe99 in vga_draw_graphic (full_update=0, s=0x5555573394b0)
> > at ../../devel/qemu/hw/display/vga.c:1661
> > #11 0x0000555555babe99 in vga_update_display (opaque=0x5555573394b0)
> > at ../../devel/qemu/hw/display/vga.c:1784
> > #12 0x0000555555babe99 in vga_update_display (opaque=0x5555573394b0)
> > at ../../devel/qemu/hw/display/vga.c:1757
> > #13 0x00005555558ddd32 in graphic_hw_update (con=0x555556a11800)
> > at ../../devel/qemu/ui/console.c:279
> > #14 0x00005555558dccd2 in dpy_refresh (s=0x555556c17da0) at ../../devel/qemu/ui/console.c:1742
> > #15 0x00005555558dccd2 in gui_update (opaque=opaque@entry=0x555556c17da0)
> > at ../../devel/qemu/ui/console.c:209
> > #16 0x0000555555dbd520 in timerlist_run_timers (timer_list=0x555556937c50)
> > at ../../devel/qemu/util/qemu-timer.c:574
> > #17 0x0000555555dbd520 in timerlist_run_timers (timer_list=0x555556937c50)
> > at ../../devel/qemu/util/qemu-timer.c:499
> > #18 0x0000555555dbd74a in qemu_clock_run_timers (type=<optimized out>)
> > at ../../devel/qemu/util/qemu-timer.c:670
> > #19 0x0000555555dbd74a in qemu_clock_run_all_timers () at ../../devel/qemu/util/qemu-timer.c:670
> >
> > Looks like something in the vga code calls this with size=6910080
> > and thus triggers the alignment assertion?
> >
> > Thomas
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
--
Peter Xu
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot
2021-03-09 14:57 ` Dr. David Alan Gilbert
2021-03-09 16:08 ` Peter Xu
@ 2021-03-09 16:20 ` Thomas Huth
2021-03-09 16:26 ` Peter Maydell
1 sibling, 1 reply; 22+ messages in thread
From: Thomas Huth @ 2021-03-09 16:20 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Peter Maydell, Andrew Jones, jiangkunkun, qemu-devel, Peter Xu,
qemu-arm, Gerd Hoffmann, wanghaibin.wang, Zenghui Yu,
Paolo Bonzini, Keqian Zhu
On 09/03/2021 15.57, Dr. David Alan Gilbert wrote:
> * Thomas Huth (thuth@redhat.com) wrote:
>> On 09/03/2021 15.05, Keqian Zhu wrote:
>>>
>>>
>>> On 2021/3/9 21:48, Thomas Huth wrote:
>>>> On 17/12/2020 02.49, Keqian Zhu wrote:
>>>>> The parameters start and size are transfered from QEMU memory
>>>>> emulation layer. It can promise that they are TARGET_PAGE_SIZE
>>>>> aligned. However, KVM needs they are qemu_real_page_size aligned.
>>>>>
>>>>> Though no caller breaks this aligned requirement currently, we'd
>>>>> better add an explicit assert to avoid future breaking.
>>>>>
>>>>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>>>>> ---
>>>>> accel/kvm/kvm-all.c | 7 +++++++
>>>>> 1 file changed, 7 insertions(+)
>>>>>
>>>>> ---
>>>>> v2
>>>>> - Address Andrew's commment (Use assert instead of return err).
>>>>>
>>>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>>>> index f6b16a8df8..73b195cc41 100644
>>>>> --- a/accel/kvm/kvm-all.c
>>>>> +++ b/accel/kvm/kvm-all.c
>>>>> @@ -692,6 +692,10 @@ out:
>>>>> #define KVM_CLEAR_LOG_ALIGN (qemu_real_host_page_size << KVM_CLEAR_LOG_SHIFT)
>>>>> #define KVM_CLEAR_LOG_MASK (-KVM_CLEAR_LOG_ALIGN)
>>>>> +/*
>>>>> + * As the granule of kvm dirty log is qemu_real_host_page_size,
>>>>> + * @start and @size are expected and restricted to align to it.
>>>>> + */
>>>>> static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
>>>>> uint64_t size)
>>>>> {
>>>>> @@ -701,6 +705,9 @@ static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
>>>>> unsigned long *bmap_clear = NULL, psize = qemu_real_host_page_size;
>>>>> int ret;
>>>>> + /* Make sure start and size are qemu_real_host_page_size aligned */
>>>>> + assert(QEMU_IS_ALIGNED(start | size, psize));
>>>>
>>>> Sorry, but that was a bad idea: It triggers and kills my Centos 6 VM:
>>>>
>>>> $ qemu-system-x86_64 -accel kvm -hda ~/virt/images/centos6.qcow2 -m 1G
>>>> qemu-system-x86_64: ../../devel/qemu/accel/kvm/kvm-all.c:690: kvm_log_clear_one_slot: Assertion `QEMU_IS_ALIGNED(start | size, psize)' failed.
>>>> Aborted (core dumped)
>>> Hi Thomas,
>>>
>>> I think this patch is ok, maybe it trigger a potential bug?
>>
>> Well, sure, there is either a bug somewhere else or in this new code. But it's certainly not normal that the assert() triggers, is it?
>>
>> FWIW, here's a backtrace:
>>
>> #0 0x00007ffff2c1584f in raise () at /lib64/libc.so.6
>> #1 0x00007ffff2bffc45 in abort () at /lib64/libc.so.6
>> #2 0x00007ffff2bffb19 in _nl_load_domain.cold.0 () at /lib64/libc.so.6
>> #3 0x00007ffff2c0de36 in .annobin_assert.c_end () at /lib64/libc.so.6
>> #4 0x0000555555ba25f3 in kvm_log_clear_one_slot
>> (size=6910080, start=0, as_id=0, mem=0x555556e1ee00)
>> at ../../devel/qemu/accel/kvm/kvm-all.c:691
>> #5 0x0000555555ba25f3 in kvm_physical_log_clear
>> (section=0x7fffffffd0b0, section=0x7fffffffd0b0, kml=0x555556dbaac0)
>> at ../../devel/qemu/accel/kvm/kvm-all.c:843
>> #6 0x0000555555ba25f3 in kvm_log_clear (listener=0x555556dbaac0, section=0x7fffffffd0b0)
>> at ../../devel/qemu/accel/kvm/kvm-all.c:1253
>> #7 0x0000555555b023d8 in memory_region_clear_dirty_bitmap
>> (mr=mr@entry=0x5555573394c0, start=start@entry=0, len=len@entry=6910080)
>> at ../../devel/qemu/softmmu/memory.c:2132
>> #8 0x0000555555b313d9 in cpu_physical_memory_snapshot_and_clear_dirty
>> (mr=mr@entry=0x5555573394c0, offset=offset@entry=0, length=length@entry=6910080, client=client@entry=0) at ../../devel/qemu/softmmu/physmem.c:1109
>> #9 0x0000555555b02483 in memory_region_snapshot_and_clear_dirty
>> (mr=mr@entry=0x5555573394c0, addr=addr@entry=0, size=size@entry=6910080, client=client@entry=0)
>> at ../../devel/qemu/softmmu/memory.c:2146
>
> Could you please figure out which memory region this is?
> WTH is that size? Is that really the problem that the size is just
> crazy?
The answer is one stack frame below...
>> #10 0x0000555555babe99 in vga_draw_graphic (full_update=0, s=0x5555573394b0)
>> at ../../devel/qemu/hw/display/vga.c:1661
The vga code basically does this:
region_start = (s->start_addr * 4);
region_end = region_start + (ram_addr_t)s->line_offset * height;
region_end += width * depth / 8; /* scanline length */
region_end -= s->line_offset;
...
memory_region_snapshot_and_clear_dirty(... region_end - region_start...);
Thus it uses a size that is nowhere guaranteed to be a multiple
of the page size.
A similar usage can be seen in other devices, too (e.g. sm501.c),
so either there is a bug in the assert() statement, or we have
a problem with many devices...
Thomas
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot
2021-03-09 16:20 ` Thomas Huth
@ 2021-03-09 16:26 ` Peter Maydell
2021-03-09 19:03 ` Paolo Bonzini
0 siblings, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2021-03-09 16:26 UTC (permalink / raw)
To: Thomas Huth
Cc: Andrew Jones, Kunkun Jiang, Dr. David Alan Gilbert, Peter Xu,
QEMU Developers, qemu-arm, Gerd Hoffmann, wanghaibin.wang,
Zenghui Yu, Paolo Bonzini, Keqian Zhu
On Tue, 9 Mar 2021 at 16:20, Thomas Huth <thuth@redhat.com> wrote:
> The vga code basically does this:
>
> region_start = (s->start_addr * 4);
> region_end = region_start + (ram_addr_t)s->line_offset * height;
> region_end += width * depth / 8; /* scanline length */
> region_end -= s->line_offset;
> ...
> memory_region_snapshot_and_clear_dirty(... region_end - region_start...);
>
> Thus it uses a size that is nowhere guaranteed to be a multiple
> of the page size.
The documentation comment for memory_region_snapshot_and_clear_dirty()
says:
* The dirty bitmap region which gets copyed into the snapshot (and
* cleared afterwards) can be larger than requested. The boundaries
* are rounded up/down
That is, it is the job of memory_region_snapshot_and_clear_dirty()
to round the boundaries up/down to whatever extent it requires
internally.
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot
2021-03-09 16:26 ` Peter Maydell
@ 2021-03-09 19:03 ` Paolo Bonzini
0 siblings, 0 replies; 22+ messages in thread
From: Paolo Bonzini @ 2021-03-09 19:03 UTC (permalink / raw)
To: Peter Maydell, Thomas Huth
Cc: Andrew Jones, Kunkun Jiang, Dr. David Alan Gilbert, Peter Xu,
QEMU Developers, qemu-arm, Gerd Hoffmann, Zenghui Yu,
wanghaibin.wang, Keqian Zhu
On 09/03/21 17:26, Peter Maydell wrote:
> The documentation comment for memory_region_snapshot_and_clear_dirty()
> says:
> * The dirty bitmap region which gets copyed into the snapshot (and
> * cleared afterwards) can be larger than requested. The boundaries
> * are rounded up/down
>
> That is, it is the job of memory_region_snapshot_and_clear_dirty()
> to round the boundaries up/down to whatever extent it requires
> internally.
Or alternatively of the log_sync/log_clear callbacks. I'll queue a
revert for now, anyway.
Paolo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot
2021-03-09 16:08 ` Peter Xu
@ 2021-03-10 1:57 ` Keqian Zhu
0 siblings, 0 replies; 22+ messages in thread
From: Keqian Zhu @ 2021-03-10 1:57 UTC (permalink / raw)
To: Peter Xu, Dr. David Alan Gilbert
Cc: Peter Maydell, Thomas Huth, jiangkunkun, Andrew Jones,
qemu-devel, qemu-arm, Gerd Hoffmann, wanghaibin.wang, Zenghui Yu,
Paolo Bonzini
On 2021/3/10 0:08, Peter Xu wrote:
> On Tue, Mar 09, 2021 at 02:57:53PM +0000, Dr. David Alan Gilbert wrote:
>> * Thomas Huth (thuth@redhat.com) wrote:
>>> On 09/03/2021 15.05, Keqian Zhu wrote:
>>>>
>>>>
>>>> On 2021/3/9 21:48, Thomas Huth wrote:
>>>>> On 17/12/2020 02.49, Keqian Zhu wrote:
[...]
>>>
>>> #0 0x00007ffff2c1584f in raise () at /lib64/libc.so.6
>>> #1 0x00007ffff2bffc45 in abort () at /lib64/libc.so.6
>>> #2 0x00007ffff2bffb19 in _nl_load_domain.cold.0 () at /lib64/libc.so.6
>>> #3 0x00007ffff2c0de36 in .annobin_assert.c_end () at /lib64/libc.so.6
>>> #4 0x0000555555ba25f3 in kvm_log_clear_one_slot
>>> (size=6910080, start=0, as_id=0, mem=0x555556e1ee00)
>>> at ../../devel/qemu/accel/kvm/kvm-all.c:691
>>> #5 0x0000555555ba25f3 in kvm_physical_log_clear
>>> (section=0x7fffffffd0b0, section=0x7fffffffd0b0, kml=0x555556dbaac0)
>>> at ../../devel/qemu/accel/kvm/kvm-all.c:843
>>> #6 0x0000555555ba25f3 in kvm_log_clear (listener=0x555556dbaac0, section=0x7fffffffd0b0)
>>> at ../../devel/qemu/accel/kvm/kvm-all.c:1253
>>> #7 0x0000555555b023d8 in memory_region_clear_dirty_bitmap
>>> (mr=mr@entry=0x5555573394c0, start=start@entry=0, len=len@entry=6910080)
>>> at ../../devel/qemu/softmmu/memory.c:2132
>>> #8 0x0000555555b313d9 in cpu_physical_memory_snapshot_and_clear_dirty
>>> (mr=mr@entry=0x5555573394c0, offset=offset@entry=0, length=length@entry=6910080, client=client@entry=0) at ../../devel/qemu/softmmu/physmem.c:1109
>>> #9 0x0000555555b02483 in memory_region_snapshot_and_clear_dirty
>>> (mr=mr@entry=0x5555573394c0, addr=addr@entry=0, size=size@entry=6910080, client=client@entry=0)
>>> at ../../devel/qemu/softmmu/memory.c:2146
>>
>> Could you please figure out which memory region this is?
>> WTH is that size? Is that really the problem that the size is just
>> crazy?
>
> It seems vga_draw_graphic() could call memory_region_snapshot_and_clear_dirty()
> with not-page-aligned size. cpu_physical_memory_snapshot_and_clear_dirty()
> actually took care of most of it on alignment, however still the "length"
> parameter got passed in without alignment check or so.
>
> Cc Gerd too.
>
> I'm not sure how many use cases are there like this.. if there're a lot maybe
> we can indeed drop this assert patch, but instead in kvm_log_clear_one_slot()
> we should ALIGN_DOWN the size to smallest host page size. Say, if we need to
> clear dirty bit for range (0, 0x1020), we should only clean (0, 0x1000) since
> there can still be dirty data on range (0x1020, 0x2000).
Right, the @start and @size should be properly aligned by kvm_log_clear_one_slot().
We shouldn't clear areas that beyond what caller expected.
An assert here is not properly.
Thanks,
Keqian
>
> Thanks,
>
>>
>> Dave
>>
>>> #10 0x0000555555babe99 in vga_draw_graphic (full_update=0, s=0x5555573394b0)
>>> at ../../devel/qemu/hw/display/vga.c:1661
>>> #11 0x0000555555babe99 in vga_update_display (opaque=0x5555573394b0)
>>> at ../../devel/qemu/hw/display/vga.c:1784
>>> #12 0x0000555555babe99 in vga_update_display (opaque=0x5555573394b0)
>>> at ../../devel/qemu/hw/display/vga.c:1757
>>> #13 0x00005555558ddd32 in graphic_hw_update (con=0x555556a11800)
>>> at ../../devel/qemu/ui/console.c:279
>>> #14 0x00005555558dccd2 in dpy_refresh (s=0x555556c17da0) at ../../devel/qemu/ui/console.c:1742
>>> #15 0x00005555558dccd2 in gui_update (opaque=opaque@entry=0x555556c17da0)
>>> at ../../devel/qemu/ui/console.c:209
>>> #16 0x0000555555dbd520 in timerlist_run_timers (timer_list=0x555556937c50)
>>> at ../../devel/qemu/util/qemu-timer.c:574
>>> #17 0x0000555555dbd520 in timerlist_run_timers (timer_list=0x555556937c50)
>>> at ../../devel/qemu/util/qemu-timer.c:499
>>> #18 0x0000555555dbd74a in qemu_clock_run_timers (type=<optimized out>)
>>> at ../../devel/qemu/util/qemu-timer.c:670
>>> #19 0x0000555555dbd74a in qemu_clock_run_all_timers () at ../../devel/qemu/util/qemu-timer.c:670
>>>
>>> Looks like something in the vga code calls this with size=6910080
>>> and thus triggers the alignment assertion?
>>>
>>> Thomas
>> --
>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2021-03-10 1:58 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-17 1:49 [PATCH v2 0/2] accel: kvm: Some bugfixes for kvm dirty log Keqian Zhu
2020-12-17 1:49 ` [PATCH v2 1/2] accel: kvm: Fix memory waste under mismatch page size Keqian Zhu
2020-12-17 1:49 ` [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot Keqian Zhu
2020-12-17 12:18 ` Andrew Jones
2020-12-17 14:36 ` Peter Xu
2021-02-01 15:14 ` Philippe Mathieu-Daudé
2021-02-02 1:17 ` Keqian Zhu
2021-03-09 13:48 ` Thomas Huth
2021-03-09 14:05 ` Keqian Zhu
2021-03-09 14:45 ` Thomas Huth
2021-03-09 14:57 ` Dr. David Alan Gilbert
2021-03-09 16:08 ` Peter Xu
2021-03-10 1:57 ` Keqian Zhu
2021-03-09 16:20 ` Thomas Huth
2021-03-09 16:26 ` Peter Maydell
2021-03-09 19:03 ` Paolo Bonzini
2021-03-09 15:11 ` zhukeqian
2021-01-06 7:07 ` [PATCH v2 0/2] accel: kvm: Some bugfixes for kvm dirty log Keqian Zhu
2021-01-25 7:51 ` Keqian Zhu
2021-02-01 13:07 ` Keqian Zhu
2021-03-02 11:43 ` [PING] " Keqian Zhu
2021-03-02 13:14 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.