| Message ID | 20201215071948.23940-3-zhukeqian1@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | accel: kvm: Some bugfixes for kvm dirty log | expand |
On Tue, Dec 15, 2020 at 03:19:48PM +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 check to avoid future breaking. > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> > --- > accel/kvm/kvm-all.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index c5e06288eb..3d0e3aa872 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -701,6 +701,11 @@ 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 psize aligned */ > + if (!QEMU_IS_ALIGNED(start, psize) || !QEMU_IS_ALIGNED(size, psize)) { > + return -EINVAL; > + } > + > /* > * 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 > > It's not clear to me that this function has any restrictions on start and size. If it does, then please document those restrictions in the function's header and assert rather than return. Thanks, drew
On Tue, Dec 15, 2020 at 12:55:50PM +0100, Andrew Jones wrote: > On Tue, Dec 15, 2020 at 03:19:48PM +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 check to avoid future breaking. > > > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> > > --- > > accel/kvm/kvm-all.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > > index c5e06288eb..3d0e3aa872 100644 > > --- a/accel/kvm/kvm-all.c > > +++ b/accel/kvm/kvm-all.c > > @@ -701,6 +701,11 @@ 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 psize aligned */ > > + if (!QEMU_IS_ALIGNED(start, psize) || !QEMU_IS_ALIGNED(size, psize)) { > > + return -EINVAL; > > + } > > + > > /* > > * 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 > > > > > > It's not clear to me that this function has any restrictions on start > and size. If it does, then please document those restrictions in the > function's header and assert rather than return. > Also, I see this patch is on its way in https://patchwork.ozlabs.org/project/qemu-devel/patch/20201215175445.1272776-27-pbonzini@redhat.com/ Thanks, drew
On 2020/12/15 19:55, Andrew Jones wrote: > On Tue, Dec 15, 2020 at 03:19:48PM +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 check to avoid future breaking. >> >> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> >> --- >> accel/kvm/kvm-all.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >> index c5e06288eb..3d0e3aa872 100644 >> --- a/accel/kvm/kvm-all.c >> +++ b/accel/kvm/kvm-all.c >> @@ -701,6 +701,11 @@ 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 psize aligned */ >> + if (!QEMU_IS_ALIGNED(start, psize) || !QEMU_IS_ALIGNED(size, psize)) { >> + return -EINVAL; >> + } >> + >> /* >> * 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 >> >> > > It's not clear to me that this function has any restrictions on start > and size. If it does, then please document those restrictions in the > function's header and assert rather than return. Hi drew, The following code implicitly expands the clear area when start_delta is not psize aligned. start_delta /= psize; Thanks, Keqian > > Thanks, > drew > > . >
On 2020/12/16 15:23, Andrew Jones wrote: > On Tue, Dec 15, 2020 at 12:55:50PM +0100, Andrew Jones wrote: >> On Tue, Dec 15, 2020 at 03:19:48PM +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 check to avoid future breaking. >>> >>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> >>> --- >>> accel/kvm/kvm-all.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >>> index c5e06288eb..3d0e3aa872 100644 >>> --- a/accel/kvm/kvm-all.c >>> +++ b/accel/kvm/kvm-all.c >>> @@ -701,6 +701,11 @@ 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 psize aligned */ >>> + if (!QEMU_IS_ALIGNED(start, psize) || !QEMU_IS_ALIGNED(size, psize)) { >>> + return -EINVAL; >>> + } >>> + >>> /* >>> * 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 >>> >>> >> >> It's not clear to me that this function has any restrictions on start >> and size. If it does, then please document those restrictions in the >> function's header and assert rather than return. >> > > Also, I see this patch is on its way in > > https://patchwork.ozlabs.org/project/qemu-devel/patch/20201215175445.1272776-27-pbonzini@redhat.com/ Hi drew, Thanks for informing me. I see that they are not the same patch. The above patch fixes 64-pages-alignment problem, but this patch handles page-alignment problem. Thanks, Keqian > > Thanks, > drew > > . >
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index c5e06288eb..3d0e3aa872 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -701,6 +701,11 @@ 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 psize aligned */ + if (!QEMU_IS_ALIGNED(start, psize) || !QEMU_IS_ALIGNED(size, psize)) { + return -EINVAL; + } + /* * We need to extend either the start or the size or both to * satisfy the KVM interface requirement. Firstly, do the start
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 check to avoid future breaking. Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> --- accel/kvm/kvm-all.c | 5 +++++ 1 file changed, 5 insertions(+)