* [PATCH] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed @ 2020-08-24 10:18 Lai Jiangshan 2020-08-28 1:47 ` Lai Jiangshan 2020-08-28 1:49 ` Lai Jiangshan 0 siblings, 2 replies; 12+ messages in thread From: Lai Jiangshan @ 2020-08-24 10:18 UTC (permalink / raw) To: linux-kernel Cc: Lai Jiangshan, Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, kvm From: Lai Jiangshan <laijs@linux.alibaba.com> 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes) changed it without giving any reason in the changelog. In theory, the syncing is needed, and need to be fixed by reverting this part of change. Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> --- arch/x86/kvm/mmu/mmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 4e03841f053d..9a93de921f2b 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, } if (sp->unsync_children) - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu); + kvm_make_request(KVM_REQ_MMU_SYNC, vcpu); __clear_sp_write_flooding_count(sp); -- 2.19.1.6.gb485710b ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed 2020-08-24 10:18 [PATCH] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed Lai Jiangshan @ 2020-08-28 1:47 ` Lai Jiangshan 2020-08-28 1:49 ` Lai Jiangshan 1 sibling, 0 replies; 12+ messages in thread From: Lai Jiangshan @ 2020-08-28 1:47 UTC (permalink / raw) To: LKML Ping @Sean Christopherson On Mon, Aug 24, 2020 at 5:18 PM Lai Jiangshan <jiangshanlai@gmail.com> wrote: > > From: Lai Jiangshan <laijs@linux.alibaba.com> > > 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes) > changed it without giving any reason in the changelog. > > In theory, the syncing is needed, and need to be fixed by reverting > this part of change. > > Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> > --- > arch/x86/kvm/mmu/mmu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index 4e03841f053d..9a93de921f2b 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, > } > > if (sp->unsync_children) > - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu); > + kvm_make_request(KVM_REQ_MMU_SYNC, vcpu); > > __clear_sp_write_flooding_count(sp); > > -- > 2.19.1.6.gb485710b > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed 2020-08-24 10:18 [PATCH] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed Lai Jiangshan 2020-08-28 1:47 ` Lai Jiangshan @ 2020-08-28 1:49 ` Lai Jiangshan 2020-08-31 13:09 ` Vitaly Kuznetsov 1 sibling, 1 reply; 12+ messages in thread From: Lai Jiangshan @ 2020-08-28 1:49 UTC (permalink / raw) To: LKML, Sean Christopherson Cc: Lai Jiangshan, Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, X86 ML, H. Peter Anvin, kvm Ping @Sean Christopherson On Mon, Aug 24, 2020 at 5:18 PM Lai Jiangshan <jiangshanlai@gmail.com> wrote: > > From: Lai Jiangshan <laijs@linux.alibaba.com> > > 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes) > changed it without giving any reason in the changelog. > > In theory, the syncing is needed, and need to be fixed by reverting > this part of change. > > Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> > --- > arch/x86/kvm/mmu/mmu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index 4e03841f053d..9a93de921f2b 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, > } > > if (sp->unsync_children) > - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu); > + kvm_make_request(KVM_REQ_MMU_SYNC, vcpu); > > __clear_sp_write_flooding_count(sp); > > -- > 2.19.1.6.gb485710b > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed 2020-08-28 1:49 ` Lai Jiangshan @ 2020-08-31 13:09 ` Vitaly Kuznetsov 2020-09-01 1:29 ` Lai Jiangshan 0 siblings, 1 reply; 12+ messages in thread From: Vitaly Kuznetsov @ 2020-08-31 13:09 UTC (permalink / raw) To: Lai Jiangshan, LKML, Sean Christopherson Cc: Lai Jiangshan, Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, X86 ML, H. Peter Anvin, kvm Lai Jiangshan <jiangshanlai@gmail.com> writes: > Ping @Sean Christopherson > Let's try 'Beetlejuice' instead :-) > On Mon, Aug 24, 2020 at 5:18 PM Lai Jiangshan <jiangshanlai@gmail.com> wrote: >> >> From: Lai Jiangshan <laijs@linux.alibaba.com> >> >> 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes) >> changed it without giving any reason in the changelog. >> >> In theory, the syncing is needed, and need to be fixed by reverting >> this part of change. Even if the original commit is not wordy enough this is hardly better. Are you seeing a particular scenario when a change in current vCPU's MMU requires flushing TLB entries for *other* contexts, ... (see below) >> >> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> >> --- >> arch/x86/kvm/mmu/mmu.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c >> index 4e03841f053d..9a93de921f2b 100644 >> --- a/arch/x86/kvm/mmu/mmu.c >> +++ b/arch/x86/kvm/mmu/mmu.c >> @@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, >> } >> >> if (sp->unsync_children) >> - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu); >> + kvm_make_request(KVM_REQ_MMU_SYNC, vcpu); ... in particular, why are you reverting only this hunk? Please elaborate. >> >> __clear_sp_write_flooding_count(sp); >> >> -- >> 2.19.1.6.gb485710b >> > -- Vitaly ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed 2020-08-31 13:09 ` Vitaly Kuznetsov @ 2020-09-01 1:29 ` Lai Jiangshan 2020-09-01 8:10 ` Vitaly Kuznetsov 0 siblings, 1 reply; 12+ messages in thread From: Lai Jiangshan @ 2020-09-01 1:29 UTC (permalink / raw) To: Vitaly Kuznetsov Cc: LKML, Sean Christopherson, Lai Jiangshan, Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, X86 ML, H. Peter Anvin, kvm On Mon, Aug 31, 2020 at 9:09 PM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > Lai Jiangshan <jiangshanlai@gmail.com> writes: > > > Ping @Sean Christopherson > > > > Let's try 'Beetlejuice' instead :-) > > > On Mon, Aug 24, 2020 at 5:18 PM Lai Jiangshan <jiangshanlai@gmail.com> wrote: > >> > >> From: Lai Jiangshan <laijs@linux.alibaba.com> > >> > >> 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes) > >> changed it without giving any reason in the changelog. > >> > >> In theory, the syncing is needed, and need to be fixed by reverting > >> this part of change. > > Even if the original commit is not wordy enough this is hardly > better. Hello, Thank you for reviewing it. I'm sorry that when I said "reverting this part of change", I meant "reverting this line of code". This line of code itself is quite clear that it is not related to the original commit according to its changelog. > Are you seeing a particular scenario when a change in current > vCPU's MMU requires flushing TLB entries for *other* contexts, ... (see > below) So I don't think the patch needs to explain this because the patch does not change/revert anything about it. Anyway, using a "revert" in the changelog is misleading, when it is not really reverting the whole targeted commit. I would remove this wording. For the change in my patch, when kvm_mmu_get_page() gets a page with unsync children, the host side pagetable is unsynchronized with the guest side pagedtable, and the guest might not issue a "flush" operation on it. It is all about the host's emulation of the pagetable. So the host has the responsibility to synchronize the pagetables. Thanks Lai > >> > >> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> > >> --- > >> arch/x86/kvm/mmu/mmu.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > >> index 4e03841f053d..9a93de921f2b 100644 > >> --- a/arch/x86/kvm/mmu/mmu.c > >> +++ b/arch/x86/kvm/mmu/mmu.c > >> @@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, > >> } > >> > >> if (sp->unsync_children) > >> - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu); > >> + kvm_make_request(KVM_REQ_MMU_SYNC, vcpu); > > ... in particular, why are you reverting only this hunk? Please elaborate. > > >> > >> __clear_sp_write_flooding_count(sp); > >> > >> -- > >> 2.19.1.6.gb485710b > >> > > > > -- > Vitaly > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed 2020-09-01 1:29 ` Lai Jiangshan @ 2020-09-01 8:10 ` Vitaly Kuznetsov 2020-09-02 13:54 ` [PATCH V2] " Lai Jiangshan 0 siblings, 1 reply; 12+ messages in thread From: Vitaly Kuznetsov @ 2020-09-01 8:10 UTC (permalink / raw) To: Lai Jiangshan Cc: LKML, Sean Christopherson, Lai Jiangshan, Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, X86 ML, H. Peter Anvin, kvm Lai Jiangshan <jiangshanlai@gmail.com> writes: > On Mon, Aug 31, 2020 at 9:09 PM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >> >> Lai Jiangshan <jiangshanlai@gmail.com> writes: >> >> > Ping @Sean Christopherson >> > >> >> Let's try 'Beetlejuice' instead :-) >> >> > On Mon, Aug 24, 2020 at 5:18 PM Lai Jiangshan <jiangshanlai@gmail.com> wrote: >> >> >> >> From: Lai Jiangshan <laijs@linux.alibaba.com> >> >> >> >> 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes) >> >> changed it without giving any reason in the changelog. >> >> >> >> In theory, the syncing is needed, and need to be fixed by reverting >> >> this part of change. >> >> Even if the original commit is not wordy enough this is hardly >> better. > > Hello, > Thank you for reviewing it. > > I'm sorry that when I said "reverting this part of change", > I meant "reverting this line of code". This line of code itself > is quite clear that it is not related to the original commit > according to its changelog. > >> Are you seeing a particular scenario when a change in current >> vCPU's MMU requires flushing TLB entries for *other* contexts, ... (see >> below) > > So I don't think the patch needs to explain this because the patch > does not change/revert anything about it. > > Anyway, using a "revert" in the changelog is misleading, when it > is not really reverting the whole targeted commit. I would > remove this wording. > > For the change in my patch, when kvm_mmu_get_page() gets a > page with unsync children, the host side pagetable is > unsynchronized with the guest side pagedtable, and the > guest might not issue a "flush" operation on it. It is > all about the host's emulation of the pagetable. So the host > has the responsibility to synchronize the pagetables. > Ah, I see now, so it seems Sean's commit has a stray change in it: the intention was to change KVM_REQ_TLB_FLUSH -> KVM_REQ_TLB_FLUSH_CURRENT so we don't unneedlesly flush other contexts but one of the hunks changed KVM_REQ_MMU_SYNC instead. Syncronizing MMU roots can't be replaced with a TLB flush, we need to revert back the change. This sounds reasonable to me, please send out v2 with the updated description. Thanks! -- Vitaly ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed 2020-09-01 8:10 ` Vitaly Kuznetsov @ 2020-09-02 13:54 ` Lai Jiangshan 2020-09-02 14:12 ` Vitaly Kuznetsov 2020-09-11 17:16 ` [PATCH V2] " Paolo Bonzini 0 siblings, 2 replies; 12+ messages in thread From: Lai Jiangshan @ 2020-09-02 13:54 UTC (permalink / raw) To: linux-kernel Cc: Lai Jiangshan, Sean Christopherson, Vitaly Kuznetsov, Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, kvm From: Lai Jiangshan <laijs@linux.alibaba.com> When kvm_mmu_get_page() gets a page with unsynced children, the spt pagetable is unsynchronized with the guest pagetable. But the guest might not issue a "flush" operation on it when the pagetable entry is changed from zero or other cases. The hypervisor has the responsibility to synchronize the pagetables. The linux kernel behaves as above for many years, But 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes) inadvertently included a line of code to change it without giving any reason in the changelog. It is clear that the commit's intention was to change KVM_REQ_TLB_FLUSH -> KVM_REQ_TLB_FLUSH_CURRENT, so we don't unneedlesly flush other contexts but one of the hunks changed nearby KVM_REQ_MMU_SYNC instead. The this patch changes it back. Link: https://lore.kernel.org/lkml/20200320212833.3507-26-sean.j.christopherson@intel.com/ Cc: Sean Christopherson <sean.j.christopherson@intel.com> Cc: Vitaly Kuznetsov <vkuznets@redhat.com> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> --- Changed from v1: update patch description arch/x86/kvm/mmu/mmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 4e03841f053d..9a93de921f2b 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, } if (sp->unsync_children) - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu); + kvm_make_request(KVM_REQ_MMU_SYNC, vcpu); __clear_sp_write_flooding_count(sp); -- 2.19.1.6.gb485710b ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH V2] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed 2020-09-02 13:54 ` [PATCH V2] " Lai Jiangshan @ 2020-09-02 14:12 ` Vitaly Kuznetsov 2020-09-03 1:22 ` Sean Christopherson 2020-09-11 17:16 ` [PATCH V2] " Paolo Bonzini 1 sibling, 1 reply; 12+ messages in thread From: Vitaly Kuznetsov @ 2020-09-02 14:12 UTC (permalink / raw) To: Lai Jiangshan, linux-kernel Cc: Lai Jiangshan, Sean Christopherson, Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, kvm Lai Jiangshan <jiangshanlai@gmail.com> writes: > From: Lai Jiangshan <laijs@linux.alibaba.com> > > When kvm_mmu_get_page() gets a page with unsynced children, the spt > pagetable is unsynchronized with the guest pagetable. But the > guest might not issue a "flush" operation on it when the pagetable > entry is changed from zero or other cases. The hypervisor has the > responsibility to synchronize the pagetables. > > The linux kernel behaves as above for many years, But > 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes) Nit: checkpatch.pl complains here with ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 8c8560b83390 ("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes")' #118: 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes) > inadvertently included a line of code to change it without giving any > reason in the changelog. It is clear that the commit's intention was to > change KVM_REQ_TLB_FLUSH -> KVM_REQ_TLB_FLUSH_CURRENT, so we don't > unneedlesly flush other contexts but one of the hunks changed > nearby KVM_REQ_MMU_SYNC instead. > > The this patch changes it back. > > Link: https://lore.kernel.org/lkml/20200320212833.3507-26-sean.j.christopherson@intel.com/ > Cc: Sean Christopherson <sean.j.christopherson@intel.com> > Cc: Vitaly Kuznetsov <vkuznets@redhat.com> > Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> > --- > Changed from v1: > update patch description > > arch/x86/kvm/mmu/mmu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index 4e03841f053d..9a93de921f2b 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, > } > > if (sp->unsync_children) > - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu); > + kvm_make_request(KVM_REQ_MMU_SYNC, vcpu); > > __clear_sp_write_flooding_count(sp); FWIW, Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> but it'd be great to hear from Sean). -- Vitaly ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed 2020-09-02 14:12 ` Vitaly Kuznetsov @ 2020-09-03 1:22 ` Sean Christopherson 2020-09-03 16:23 ` [PATCH V3] " Lai Jiangshan 0 siblings, 1 reply; 12+ messages in thread From: Sean Christopherson @ 2020-09-03 1:22 UTC (permalink / raw) To: Vitaly Kuznetsov Cc: Lai Jiangshan, linux-kernel, Lai Jiangshan, Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, kvm On Wed, Sep 02, 2020 at 04:12:55PM +0200, Vitaly Kuznetsov wrote: > Lai Jiangshan <jiangshanlai@gmail.com> writes: > > > From: Lai Jiangshan <laijs@linux.alibaba.com> > > > > When kvm_mmu_get_page() gets a page with unsynced children, the spt > > pagetable is unsynchronized with the guest pagetable. But the > > guest might not issue a "flush" operation on it when the pagetable > > entry is changed from zero or other cases. The hypervisor has the > > responsibility to synchronize the pagetables. > > > > The linux kernel behaves as above for many years, But > > 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes) > > Nit: checkpatch.pl complains here with > > ERROR: Please use git commit description style 'commit <12+ chars of > sha1> ("<title line>")' - ie: 'commit 8c8560b83390 ("KVM: x86/mmu: Use > KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes")' > #118: > 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes) Definitely needs a Fixes: 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes) At that point I'd just have the changelog say "a recent commit". > > inadvertently included a line of code to change it without giving any > > reason in the changelog. It is clear that the commit's intention was to > > change KVM_REQ_TLB_FLUSH -> KVM_REQ_TLB_FLUSH_CURRENT, so we don't > > unneedlesly flush other contexts but one of the hunks changed > > nearby KVM_REQ_MMU_SYNC instead. > > > > The this patch changes it back. > > > > Link: https://lore.kernel.org/lkml/20200320212833.3507-26-sean.j.christopherson@intel.com/ > > Cc: Sean Christopherson <sean.j.christopherson@intel.com> > > Cc: Vitaly Kuznetsov <vkuznets@redhat.com> > > Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> > > --- > > Changed from v1: > > update patch description > > > > arch/x86/kvm/mmu/mmu.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > > index 4e03841f053d..9a93de921f2b 100644 > > --- a/arch/x86/kvm/mmu/mmu.c > > +++ b/arch/x86/kvm/mmu/mmu.c > > @@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, > > } > > > > if (sp->unsync_children) > > - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu); > > + kvm_make_request(KVM_REQ_MMU_SYNC, vcpu); > > > > __clear_sp_write_flooding_count(sp); > > FWIW, > > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > but it'd be great to hear from Sean). I got nothing, AFAICT I was simply overzealous. Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V3] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed 2020-09-03 1:22 ` Sean Christopherson @ 2020-09-03 16:23 ` Lai Jiangshan 2020-09-10 10:21 ` Lai Jiangshan 0 siblings, 1 reply; 12+ messages in thread From: Lai Jiangshan @ 2020-09-03 16:23 UTC (permalink / raw) To: linux-kernel Cc: Lai Jiangshan, Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, kvm From: Lai Jiangshan <laijs@linux.alibaba.com> When kvm_mmu_get_page() gets a page with unsynced children, the spt pagetable is unsynchronized with the guest pagetable. But the guest might not issue a "flush" operation on it when the pagetable entry is changed from zero or other cases. The hypervisor has the responsibility to synchronize the pagetables. The linux kernel behaves correctly as above for many years, but a recent commit 8c8560b83390 ("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes") inadvertently included a line of code to change it without giving any reason in the changelog. It is clear that the commit's intention was to change KVM_REQ_TLB_FLUSH -> KVM_REQ_TLB_FLUSH_CURRENT, so we don't unneedlesly flush other contexts but one of the hunks changed nearby KVM_REQ_MMU_SYNC instead. This patch changes it back. Fixes: 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes) Link: https://lore.kernel.org/lkml/20200320212833.3507-26-sean.j.christopherson@intel.com/ Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> --- Changed from v1: update patch description Changed form v2: update patch description arch/x86/kvm/mmu/mmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 4e03841f053d..9a93de921f2b 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, } if (sp->unsync_children) - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu); + kvm_make_request(KVM_REQ_MMU_SYNC, vcpu); __clear_sp_write_flooding_count(sp); -- 2.19.1.6.gb485710b ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH V3] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed 2020-09-03 16:23 ` [PATCH V3] " Lai Jiangshan @ 2020-09-10 10:21 ` Lai Jiangshan 0 siblings, 0 replies; 12+ messages in thread From: Lai Jiangshan @ 2020-09-10 10:21 UTC (permalink / raw) To: LKML, Paolo Bonzini Cc: Lai Jiangshan, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, X86 ML, H. Peter Anvin, kvm Hi, Paolo Could you pick the patch please? I think it'd be better to be merged before v5.9 is released since it fixes a flaw. Thanks Lai On Thu, Sep 3, 2020 at 11:22 PM Lai Jiangshan <jiangshanlai@gmail.com> wrote: > > From: Lai Jiangshan <laijs@linux.alibaba.com> > > When kvm_mmu_get_page() gets a page with unsynced children, the spt > pagetable is unsynchronized with the guest pagetable. But the > guest might not issue a "flush" operation on it when the pagetable > entry is changed from zero or other cases. The hypervisor has the > responsibility to synchronize the pagetables. > > The linux kernel behaves correctly as above for many years, but a recent > commit 8c8560b83390 ("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for > MMU specific flushes") inadvertently included a line of code to change it > without giving any reason in the changelog. It is clear that the commit's > intention was to change KVM_REQ_TLB_FLUSH -> KVM_REQ_TLB_FLUSH_CURRENT, > so we don't unneedlesly flush other contexts but one of the hunks changed > nearby KVM_REQ_MMU_SYNC instead. > > This patch changes it back. > > Fixes: 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes) > Link: https://lore.kernel.org/lkml/20200320212833.3507-26-sean.j.christopherson@intel.com/ > Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com> > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> > Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> > --- > Changed from v1: > update patch description > > Changed form v2: > update patch description > > arch/x86/kvm/mmu/mmu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index 4e03841f053d..9a93de921f2b 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, > } > > if (sp->unsync_children) > - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu); > + kvm_make_request(KVM_REQ_MMU_SYNC, vcpu); > > __clear_sp_write_flooding_count(sp); > > -- > 2.19.1.6.gb485710b > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed 2020-09-02 13:54 ` [PATCH V2] " Lai Jiangshan 2020-09-02 14:12 ` Vitaly Kuznetsov @ 2020-09-11 17:16 ` Paolo Bonzini 1 sibling, 0 replies; 12+ messages in thread From: Paolo Bonzini @ 2020-09-11 17:16 UTC (permalink / raw) To: Lai Jiangshan, linux-kernel Cc: Lai Jiangshan, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, kvm On 02/09/20 15:54, Lai Jiangshan wrote: > From: Lai Jiangshan <laijs@linux.alibaba.com> > > When kvm_mmu_get_page() gets a page with unsynced children, the spt > pagetable is unsynchronized with the guest pagetable. But the > guest might not issue a "flush" operation on it when the pagetable > entry is changed from zero or other cases. The hypervisor has the > responsibility to synchronize the pagetables. > > The linux kernel behaves as above for many years, But > 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes) > inadvertently included a line of code to change it without giving any > reason in the changelog. It is clear that the commit's intention was to > change KVM_REQ_TLB_FLUSH -> KVM_REQ_TLB_FLUSH_CURRENT, so we don't > unneedlesly flush other contexts but one of the hunks changed > nearby KVM_REQ_MMU_SYNC instead. > > The this patch changes it back. > > Link: https://lore.kernel.org/lkml/20200320212833.3507-26-sean.j.christopherson@intel.com/ > Cc: Sean Christopherson <sean.j.christopherson@intel.com> > Cc: Vitaly Kuznetsov <vkuznets@redhat.com> > Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> > --- > Changed from v1: > update patch description > > arch/x86/kvm/mmu/mmu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index 4e03841f053d..9a93de921f2b 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, > } > > if (sp->unsync_children) > - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu); > + kvm_make_request(KVM_REQ_MMU_SYNC, vcpu); > > __clear_sp_write_flooding_count(sp); > > Queued, thanks. Paolo ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-09-11 17:17 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-24 10:18 [PATCH] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed Lai Jiangshan 2020-08-28 1:47 ` Lai Jiangshan 2020-08-28 1:49 ` Lai Jiangshan 2020-08-31 13:09 ` Vitaly Kuznetsov 2020-09-01 1:29 ` Lai Jiangshan 2020-09-01 8:10 ` Vitaly Kuznetsov 2020-09-02 13:54 ` [PATCH V2] " Lai Jiangshan 2020-09-02 14:12 ` Vitaly Kuznetsov 2020-09-03 1:22 ` Sean Christopherson 2020-09-03 16:23 ` [PATCH V3] " Lai Jiangshan 2020-09-10 10:21 ` Lai Jiangshan 2020-09-11 17:16 ` [PATCH V2] " 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).