* [PATCH] kvm: don't lose the higher 32 bits of tlbs_dirty @ 2020-12-13 4:49 Lai Jiangshan 2020-12-14 17:20 ` Sean Christopherson 0 siblings, 1 reply; 10+ messages in thread From: Lai Jiangshan @ 2020-12-13 4:49 UTC (permalink / raw) To: linux-kernel, kvm; +Cc: Lai Jiangshan, Paolo Bonzini From: Lai Jiangshan <laijs@linux.alibaba.com> In kvm_mmu_notifier_invalidate_range_start(), tlbs_dirty is used as: need_tlb_flush |= kvm->tlbs_dirty; with need_tlb_flush's type being int and tlbs_dirty's type being long. It means that tlbs_dirty is always used as int and the higher 32 bits is useless. We can just change need_tlb_flush's type to long to make full use of tlbs_dirty. Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> --- virt/kvm/kvm_main.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 2541a17ff1c4..4e519a517e9f 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -470,7 +470,8 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, const struct mmu_notifier_range *range) { struct kvm *kvm = mmu_notifier_to_kvm(mn); - int need_tlb_flush = 0, idx; + long need_tlb_flush = 0; + int idx; idx = srcu_read_lock(&kvm->srcu); spin_lock(&kvm->mmu_lock); -- 2.19.1.6.gb485710b ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] kvm: don't lose the higher 32 bits of tlbs_dirty 2020-12-13 4:49 [PATCH] kvm: don't lose the higher 32 bits of tlbs_dirty Lai Jiangshan @ 2020-12-14 17:20 ` Sean Christopherson 2020-12-15 5:14 ` Lai Jiangshan 2020-12-15 10:32 ` Paolo Bonzini 0 siblings, 2 replies; 10+ messages in thread From: Sean Christopherson @ 2020-12-14 17:20 UTC (permalink / raw) To: Lai Jiangshan; +Cc: linux-kernel, kvm, Lai Jiangshan, Paolo Bonzini On Sun, Dec 13, 2020, Lai Jiangshan wrote: > From: Lai Jiangshan <laijs@linux.alibaba.com> > > In kvm_mmu_notifier_invalidate_range_start(), tlbs_dirty is used as: > need_tlb_flush |= kvm->tlbs_dirty; > with need_tlb_flush's type being int and tlbs_dirty's type being long. > > It means that tlbs_dirty is always used as int and the higher 32 bits > is useless. It's probably worth noting in the changelog that it's _extremely_ unlikely this bug can cause problems in practice. It would require encountering tlbs_dirty on a 4 billion count boundary, and KVM would need to be using shadow paging or be running a nested guest. > We can just change need_tlb_flush's type to long to > make full use of tlbs_dirty. Hrm, this does solve the problem, but I'm not a fan of continuing to use an integer variable as a boolean. Rather than propagate tlbs_dirty to need_tlb_flush, what if this bug fix patch checks tlbs_dirty directly, and then a follow up patch converts need_tlb_flush to a bool and removes the unnecessary initialization (see below). E.g. the net result of both patches would be: diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 3abcb2ce5b7d..93b6986d3dfc 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -473,7 +473,8 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, const struct mmu_notifier_range *range) { struct kvm *kvm = mmu_notifier_to_kvm(mn); - int need_tlb_flush = 0, idx; + bool need_tlb_flush; + int idx; idx = srcu_read_lock(&kvm->srcu); spin_lock(&kvm->mmu_lock); @@ -483,11 +484,10 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, * count is also read inside the mmu_lock critical section. */ kvm->mmu_notifier_count++; - need_tlb_flush = kvm_unmap_hva_range(kvm, range->start, range->end, - range->flags); - need_tlb_flush |= kvm->tlbs_dirty; + need_tlb_flush = !!kvm_unmap_hva_range(kvm, range->start, range->end, + range->flags); /* we've to flush the tlb before the pages can be freed */ - if (need_tlb_flush) + if (need_tlb_flush || kvm->tlbs_dirty) kvm_flush_remote_tlbs(kvm); spin_unlock(&kvm->mmu_lock); Cc: stable@vger.kernel.org Fixes: a4ee1ca4a36e ("KVM: MMU: delay flush all tlbs on sync_page path") > Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> > --- > virt/kvm/kvm_main.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 2541a17ff1c4..4e519a517e9f 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -470,7 +470,8 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, > const struct mmu_notifier_range *range) > { > struct kvm *kvm = mmu_notifier_to_kvm(mn); > - int need_tlb_flush = 0, idx; > + long need_tlb_flush = 0; need_tlb_flush doesn't need to be initialized here, it's explicitly set via the call to kvm_unmap_hva_range(). > + int idx; > > idx = srcu_read_lock(&kvm->srcu); > spin_lock(&kvm->mmu_lock); > -- > 2.19.1.6.gb485710b > ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] kvm: don't lose the higher 32 bits of tlbs_dirty 2020-12-14 17:20 ` Sean Christopherson @ 2020-12-15 5:14 ` Lai Jiangshan 2020-12-15 10:32 ` Paolo Bonzini 1 sibling, 0 replies; 10+ messages in thread From: Lai Jiangshan @ 2020-12-15 5:14 UTC (permalink / raw) To: Sean Christopherson; +Cc: LKML, kvm, Lai Jiangshan, Paolo Bonzini On Tue, Dec 15, 2020 at 1:20 AM Sean Christopherson <seanjc@google.com> wrote: > > On Sun, Dec 13, 2020, Lai Jiangshan wrote: > > From: Lai Jiangshan <laijs@linux.alibaba.com> > > > > In kvm_mmu_notifier_invalidate_range_start(), tlbs_dirty is used as: > > need_tlb_flush |= kvm->tlbs_dirty; > > with need_tlb_flush's type being int and tlbs_dirty's type being long. > > > > It means that tlbs_dirty is always used as int and the higher 32 bits > > is useless. > > It's probably worth noting in the changelog that it's _extremely_ unlikely this > bug can cause problems in practice. It would require encountering tlbs_dirty > on a 4 billion count boundary, and KVM would need to be using shadow paging or > be running a nested guest. You are right, I don't consider it would cause problems in practice, and I also tried to make tlbs_dirty as "int" and found that I have to change too many places. And you are right it is worth noting about it, I'm sorry for neglecting. > > > We can just change need_tlb_flush's type to long to > > make full use of tlbs_dirty. > > Hrm, this does solve the problem, but I'm not a fan of continuing to use an > integer variable as a boolean. Rather than propagate tlbs_dirty to > need_tlb_flush, what if this bug fix patch checks tlbs_dirty directly, and then > a follow up patch converts need_tlb_flush to a bool and removes the unnecessary > initialization (see below). > > E.g. the net result of both patches would be: > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 3abcb2ce5b7d..93b6986d3dfc 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -473,7 +473,8 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, > const struct mmu_notifier_range *range) > { > struct kvm *kvm = mmu_notifier_to_kvm(mn); > - int need_tlb_flush = 0, idx; > + bool need_tlb_flush; > + int idx; > > idx = srcu_read_lock(&kvm->srcu); > spin_lock(&kvm->mmu_lock); > @@ -483,11 +484,10 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, > * count is also read inside the mmu_lock critical section. > */ > kvm->mmu_notifier_count++; > - need_tlb_flush = kvm_unmap_hva_range(kvm, range->start, range->end, > - range->flags); > - need_tlb_flush |= kvm->tlbs_dirty; > + need_tlb_flush = !!kvm_unmap_hva_range(kvm, range->start, range->end, > + range->flags); > /* we've to flush the tlb before the pages can be freed */ > - if (need_tlb_flush) > + if (need_tlb_flush || kvm->tlbs_dirty) > kvm_flush_remote_tlbs(kvm); > > spin_unlock(&kvm->mmu_lock); > > Cc: stable@vger.kernel.org > Fixes: a4ee1ca4a36e ("KVM: MMU: delay flush all tlbs on sync_page path") I searched back, found it and considered adding this fixes tag, but I was afraid that this cleanup would be backported. Is it worth backporting such patch which is extremely hardly a problem in practice? > > > Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> > > --- > > virt/kvm/kvm_main.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index 2541a17ff1c4..4e519a517e9f 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -470,7 +470,8 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, > > const struct mmu_notifier_range *range) > > { > > struct kvm *kvm = mmu_notifier_to_kvm(mn); > > - int need_tlb_flush = 0, idx; > > + long need_tlb_flush = 0; > > need_tlb_flush doesn't need to be initialized here, it's explicitly set via the > call to kvm_unmap_hva_range(). > > > + int idx; > > > > idx = srcu_read_lock(&kvm->srcu); > > spin_lock(&kvm->mmu_lock); > > -- > > 2.19.1.6.gb485710b > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] kvm: don't lose the higher 32 bits of tlbs_dirty 2020-12-14 17:20 ` Sean Christopherson 2020-12-15 5:14 ` Lai Jiangshan @ 2020-12-15 10:32 ` Paolo Bonzini 2020-12-15 14:52 ` [PATCH V2] kvm: check tlbs_dirty directly Lai Jiangshan 1 sibling, 1 reply; 10+ messages in thread From: Paolo Bonzini @ 2020-12-15 10:32 UTC (permalink / raw) To: Sean Christopherson, Lai Jiangshan; +Cc: linux-kernel, kvm, Lai Jiangshan On 14/12/20 18:20, Sean Christopherson wrote: > On Sun, Dec 13, 2020, Lai Jiangshan wrote: >> From: Lai Jiangshan <laijs@linux.alibaba.com> >> >> In kvm_mmu_notifier_invalidate_range_start(), tlbs_dirty is used as: >> need_tlb_flush |= kvm->tlbs_dirty; >> with need_tlb_flush's type being int and tlbs_dirty's type being long. >> >> It means that tlbs_dirty is always used as int and the higher 32 bits >> is useless. > > It's probably worth noting in the changelog that it's _extremely_ unlikely this > bug can cause problems in practice. It would require encountering tlbs_dirty > on a 4 billion count boundary, and KVM would need to be using shadow paging or > be running a nested guest. > >> We can just change need_tlb_flush's type to long to >> make full use of tlbs_dirty. > > Hrm, this does solve the problem, but I'm not a fan of continuing to use an > integer variable as a boolean. Rather than propagate tlbs_dirty to > need_tlb_flush, what if this bug fix patch checks tlbs_dirty directly, and then > a follow up patch converts need_tlb_flush to a bool and removes the unnecessary > initialization (see below). Indeed, the compiler should be able to convert || to | if useful and valid (it may or may not do it depending on the sizes of types involved, but that's Someone Else's Problem and this is not really a path where every instruction matter). Paolo > E.g. the net result of both patches would be: > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 3abcb2ce5b7d..93b6986d3dfc 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -473,7 +473,8 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, > const struct mmu_notifier_range *range) > { > struct kvm *kvm = mmu_notifier_to_kvm(mn); > - int need_tlb_flush = 0, idx; > + bool need_tlb_flush; > + int idx; > > idx = srcu_read_lock(&kvm->srcu); > spin_lock(&kvm->mmu_lock); > @@ -483,11 +484,10 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, > * count is also read inside the mmu_lock critical section. > */ > kvm->mmu_notifier_count++; > - need_tlb_flush = kvm_unmap_hva_range(kvm, range->start, range->end, > - range->flags); > - need_tlb_flush |= kvm->tlbs_dirty; > + need_tlb_flush = !!kvm_unmap_hva_range(kvm, range->start, range->end, > + range->flags); > /* we've to flush the tlb before the pages can be freed */ > - if (need_tlb_flush) > + if (need_tlb_flush || kvm->tlbs_dirty) > kvm_flush_remote_tlbs(kvm); > > spin_unlock(&kvm->mmu_lock); > > Cc: stable@vger.kernel.org > Fixes: a4ee1ca4a36e ("KVM: MMU: delay flush all tlbs on sync_page path") > >> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> >> --- >> virt/kvm/kvm_main.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >> index 2541a17ff1c4..4e519a517e9f 100644 >> --- a/virt/kvm/kvm_main.c >> +++ b/virt/kvm/kvm_main.c >> @@ -470,7 +470,8 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, >> const struct mmu_notifier_range *range) >> { >> struct kvm *kvm = mmu_notifier_to_kvm(mn); >> - int need_tlb_flush = 0, idx; >> + long need_tlb_flush = 0; > > need_tlb_flush doesn't need to be initialized here, it's explicitly set via the > call to kvm_unmap_hva_range(). > >> + int idx; >> >> idx = srcu_read_lock(&kvm->srcu); >> spin_lock(&kvm->mmu_lock); >> -- >> 2.19.1.6.gb485710b >> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH V2] kvm: check tlbs_dirty directly 2020-12-15 10:32 ` Paolo Bonzini @ 2020-12-15 14:52 ` Lai Jiangshan 2020-12-15 18:44 ` Sean Christopherson 0 siblings, 1 reply; 10+ messages in thread From: Lai Jiangshan @ 2020-12-15 14:52 UTC (permalink / raw) To: linux-kernel, kvm Cc: Paolo Bonzini, Sean Christopherson, Lai Jiangshan, stable From: Lai Jiangshan <laijs@linux.alibaba.com> In kvm_mmu_notifier_invalidate_range_start(), tlbs_dirty is used as: need_tlb_flush |= kvm->tlbs_dirty; with need_tlb_flush's type being int and tlbs_dirty's type being long. It means that tlbs_dirty is always used as int and the higher 32 bits is useless. We need to check tlbs_dirty in a correct way and this change checks it directly without propagating it to need_tlb_flush. And need_tlb_flush is changed to boolean because it is used as a boolean and its name starts with "need". Note: it's _extremely_ unlikely this neglecting of higher 32 bits can cause problems in practice. It would require encountering tlbs_dirty on a 4 billion count boundary, and KVM would need to be using shadow paging or be running a nested guest. Cc: stable@vger.kernel.org Fixes: a4ee1ca4a36e ("KVM: MMU: delay flush all tlbs on sync_page path") Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> --- Changed from V1: Update the patch and the changelog as Sean Christopherson suggested. virt/kvm/kvm_main.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 2541a17ff1c4..1c17f3d073cb 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -470,7 +470,8 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, const struct mmu_notifier_range *range) { struct kvm *kvm = mmu_notifier_to_kvm(mn); - int need_tlb_flush = 0, idx; + int idx; + bool need_tlb_flush; idx = srcu_read_lock(&kvm->srcu); spin_lock(&kvm->mmu_lock); @@ -480,11 +481,10 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, * count is also read inside the mmu_lock critical section. */ kvm->mmu_notifier_count++; - need_tlb_flush = kvm_unmap_hva_range(kvm, range->start, range->end, - range->flags); - need_tlb_flush |= kvm->tlbs_dirty; + need_tlb_flush = !!kvm_unmap_hva_range(kvm, range->start, range->end, + range->flags); /* we've to flush the tlb before the pages can be freed */ - if (need_tlb_flush) + if (need_tlb_flush || kvm->tlbs_dirty) kvm_flush_remote_tlbs(kvm); spin_unlock(&kvm->mmu_lock); -- 2.19.1.6.gb485710b ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH V2] kvm: check tlbs_dirty directly 2020-12-15 14:52 ` [PATCH V2] kvm: check tlbs_dirty directly Lai Jiangshan @ 2020-12-15 18:44 ` Sean Christopherson 2020-12-16 9:46 ` Greg KH ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Sean Christopherson @ 2020-12-15 18:44 UTC (permalink / raw) To: Lai Jiangshan; +Cc: linux-kernel, kvm, Paolo Bonzini, Lai Jiangshan, stable Note, you don't actually need to Cc stable@vger.kernel.org when sending the patch. If/when the patch is merged to Linus' tree, the stable tree maintainers, or more accurately their scripts, will automatically pick up the patch and apply it to the relevant stable trees. You'll probably be getting the following letter from Greg KH any time now :-) <formletter> This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly. </formletter> On Tue, Dec 15, 2020, Lai Jiangshan wrote: > From: Lai Jiangshan <laijs@linux.alibaba.com> > > In kvm_mmu_notifier_invalidate_range_start(), tlbs_dirty is used as: > need_tlb_flush |= kvm->tlbs_dirty; > with need_tlb_flush's type being int and tlbs_dirty's type being long. > > It means that tlbs_dirty is always used as int and the higher 32 bits > is useless. We need to check tlbs_dirty in a correct way and this > change checks it directly without propagating it to need_tlb_flush. > > And need_tlb_flush is changed to boolean because it is used as a > boolean and its name starts with "need". > > Note: it's _extremely_ unlikely this neglecting of higher 32 bits can > cause problems in practice. It would require encountering tlbs_dirty > on a 4 billion count boundary, and KVM would need to be using shadow > paging or be running a nested guest. > > Cc: stable@vger.kernel.org > Fixes: a4ee1ca4a36e ("KVM: MMU: delay flush all tlbs on sync_page path") > Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> > --- > Changed from V1: > Update the patch and the changelog as Sean Christopherson suggested. > > virt/kvm/kvm_main.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 2541a17ff1c4..1c17f3d073cb 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -470,7 +470,8 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, > const struct mmu_notifier_range *range) > { > struct kvm *kvm = mmu_notifier_to_kvm(mn); > - int need_tlb_flush = 0, idx; > + int idx; > + bool need_tlb_flush; It's a bit silly given how small this patch already is, but I do think this should be split into two patches so that the kvm->tlbs_dirty bug fix is fully isolated for backporting. I.e. patch 1/2 would simply be: diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 3abcb2ce5b7d..19dae28904f7 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -485,9 +485,8 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, kvm->mmu_notifier_count++; need_tlb_flush = kvm_unmap_hva_range(kvm, range->start, range->end, range->flags); - need_tlb_flush |= kvm->tlbs_dirty; /* we've to flush the tlb before the pages can be freed */ - if (need_tlb_flush) + if (need_tlb_flush || kvm->tlbs_dirty) kvm_flush_remote_tlbs(kvm); spin_unlock(&kvm->mmu_lock); > > idx = srcu_read_lock(&kvm->srcu); > spin_lock(&kvm->mmu_lock); > @@ -480,11 +481,10 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, > * count is also read inside the mmu_lock critical section. > */ > kvm->mmu_notifier_count++; > - need_tlb_flush = kvm_unmap_hva_range(kvm, range->start, range->end, > - range->flags); > - need_tlb_flush |= kvm->tlbs_dirty; > + need_tlb_flush = !!kvm_unmap_hva_range(kvm, range->start, range->end, > + range->flags); > /* we've to flush the tlb before the pages can be freed */ > - if (need_tlb_flush) > + if (need_tlb_flush || kvm->tlbs_dirty) > kvm_flush_remote_tlbs(kvm); > > spin_unlock(&kvm->mmu_lock); > -- > 2.19.1.6.gb485710b > ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH V2] kvm: check tlbs_dirty directly 2020-12-15 18:44 ` Sean Christopherson @ 2020-12-16 9:46 ` Greg KH 2020-12-16 16:37 ` Paolo Bonzini 2020-12-17 15:41 ` [PATCH V3] " Lai Jiangshan 2 siblings, 0 replies; 10+ messages in thread From: Greg KH @ 2020-12-16 9:46 UTC (permalink / raw) To: Sean Christopherson Cc: Lai Jiangshan, linux-kernel, kvm, Paolo Bonzini, Lai Jiangshan, stable On Tue, Dec 15, 2020 at 10:44:18AM -0800, Sean Christopherson wrote: > Note, you don't actually need to Cc stable@vger.kernel.org when sending the > patch. If/when the patch is merged to Linus' tree, the stable tree maintainers, > or more accurately their scripts, will automatically pick up the patch and apply > it to the relevant stable trees. > > You'll probably be getting the following letter from Greg KH any time now :-) Nope, this was fine, I don't mind seeing the patch before it hits Linus's tree (or any tree) at all. It lets me know what to look out for, nothing was done incorrectly here at all from what I can tell. thanks, greg k-h ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] kvm: check tlbs_dirty directly 2020-12-15 18:44 ` Sean Christopherson 2020-12-16 9:46 ` Greg KH @ 2020-12-16 16:37 ` Paolo Bonzini 2020-12-17 15:41 ` [PATCH V3] " Lai Jiangshan 2 siblings, 0 replies; 10+ messages in thread From: Paolo Bonzini @ 2020-12-16 16:37 UTC (permalink / raw) To: Sean Christopherson, Lai Jiangshan Cc: linux-kernel, kvm, Lai Jiangshan, stable On 15/12/20 19:44, Sean Christopherson wrote: > Note, you don't actually need to Ccstable@vger.kernel.org when sending the > patch. If/when the patch is merged to Linus' tree, the stable tree maintainers, > or more accurately their scripts, will automatically pick up the patch and apply > it to the relevant stable trees. > > You'll probably be getting the following letter from Greg KH any time now:-) > > <formletter> > > This is not the correct way to submit patches for inclusion in the > stable kernel tree. Please read: > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html > for how to do this properly. > > </formletter> As far as I know, as long as he sees LKML or something else in Cc he assumes it's an indication _for the maintainer_. Don't underestimate the power of Greg's mailing list filters. :) Paolo ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH V3] kvm: check tlbs_dirty directly 2020-12-15 18:44 ` Sean Christopherson 2020-12-16 9:46 ` Greg KH 2020-12-16 16:37 ` Paolo Bonzini @ 2020-12-17 15:41 ` Lai Jiangshan 2020-12-21 18:31 ` Paolo Bonzini 2 siblings, 1 reply; 10+ messages in thread From: Lai Jiangshan @ 2020-12-17 15:41 UTC (permalink / raw) To: linux-kernel, kvm Cc: Paolo Bonzini, Sean Christopherson, Lai Jiangshan, stable From: Lai Jiangshan <laijs@linux.alibaba.com> In kvm_mmu_notifier_invalidate_range_start(), tlbs_dirty is used as: need_tlb_flush |= kvm->tlbs_dirty; with need_tlb_flush's type being int and tlbs_dirty's type being long. It means that tlbs_dirty is always used as int and the higher 32 bits is useless. We need to check tlbs_dirty in a correct way and this change checks it directly without propagating it to need_tlb_flush. Note: it's _extremely_ unlikely this neglecting of higher 32 bits can cause problems in practice. It would require encountering tlbs_dirty on a 4 billion count boundary, and KVM would need to be using shadow paging or be running a nested guest. Cc: stable@vger.kernel.org Fixes: a4ee1ca4a36e ("KVM: MMU: delay flush all tlbs on sync_page path") Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> --- Changed from V1: Update the patch and the changelog as Sean Christopherson suggested. Changed from v2: don't change the type of need_tlb_flush virt/kvm/kvm_main.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 2541a17ff1c4..3083fb53861d 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -482,9 +482,8 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, kvm->mmu_notifier_count++; need_tlb_flush = kvm_unmap_hva_range(kvm, range->start, range->end, range->flags); - need_tlb_flush |= kvm->tlbs_dirty; /* we've to flush the tlb before the pages can be freed */ - if (need_tlb_flush) + if (need_tlb_flush || kvm->tlbs_dirty) kvm_flush_remote_tlbs(kvm); spin_unlock(&kvm->mmu_lock); -- 2.19.1.6.gb485710b ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH V3] kvm: check tlbs_dirty directly 2020-12-17 15:41 ` [PATCH V3] " Lai Jiangshan @ 2020-12-21 18:31 ` Paolo Bonzini 0 siblings, 0 replies; 10+ messages in thread From: Paolo Bonzini @ 2020-12-21 18:31 UTC (permalink / raw) To: Lai Jiangshan, linux-kernel, kvm Cc: Sean Christopherson, Lai Jiangshan, stable On 17/12/20 16:41, Lai Jiangshan wrote: > From: Lai Jiangshan <laijs@linux.alibaba.com> > > In kvm_mmu_notifier_invalidate_range_start(), tlbs_dirty is used as: > need_tlb_flush |= kvm->tlbs_dirty; > with need_tlb_flush's type being int and tlbs_dirty's type being long. > > It means that tlbs_dirty is always used as int and the higher 32 bits > is useless. We need to check tlbs_dirty in a correct way and this > change checks it directly without propagating it to need_tlb_flush. > > Note: it's _extremely_ unlikely this neglecting of higher 32 bits can > cause problems in practice. It would require encountering tlbs_dirty > on a 4 billion count boundary, and KVM would need to be using shadow > paging or be running a nested guest. > > Cc: stable@vger.kernel.org > Fixes: a4ee1ca4a36e ("KVM: MMU: delay flush all tlbs on sync_page path") > Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> > --- > Changed from V1: > Update the patch and the changelog as Sean Christopherson suggested. > > Changed from v2: > don't change the type of need_tlb_flush > > virt/kvm/kvm_main.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 2541a17ff1c4..3083fb53861d 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -482,9 +482,8 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, > kvm->mmu_notifier_count++; > need_tlb_flush = kvm_unmap_hva_range(kvm, range->start, range->end, > range->flags); > - need_tlb_flush |= kvm->tlbs_dirty; > /* we've to flush the tlb before the pages can be freed */ > - if (need_tlb_flush) > + if (need_tlb_flush || kvm->tlbs_dirty) > kvm_flush_remote_tlbs(kvm); > > spin_unlock(&kvm->mmu_lock); > Queued, thanks. Paolo ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-12-21 18:33 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-13 4:49 [PATCH] kvm: don't lose the higher 32 bits of tlbs_dirty Lai Jiangshan 2020-12-14 17:20 ` Sean Christopherson 2020-12-15 5:14 ` Lai Jiangshan 2020-12-15 10:32 ` Paolo Bonzini 2020-12-15 14:52 ` [PATCH V2] kvm: check tlbs_dirty directly Lai Jiangshan 2020-12-15 18:44 ` Sean Christopherson 2020-12-16 9:46 ` Greg KH 2020-12-16 16:37 ` Paolo Bonzini 2020-12-17 15:41 ` [PATCH V3] " Lai Jiangshan 2020-12-21 18:31 ` Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).