| Message ID | 20200824101825.4106-1-jiangshanlai@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed | expand |
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 >
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 >> >
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 >
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!
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);