Message ID | 20200528080456.87797-1-aneesh.kumar@linux.ibm.com |
---|---|
State | Changes Requested |
Headers | show |
Series | powerpc/book3s64/kvm: Fix secondary page table walk warning during migration | expand |
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes: > This patch fix the below warning reported during migration. > > find_kvm_secondary_pte called with kvm mmu_lock not held > CPU: 23 PID: 5341 Comm: qemu-system-ppc Tainted: G W 5.7.0-rc5-kvm-00211-g9ccf10d6d088 #432 > NIP: c008000000fe848c LR: c008000000fe8488 CTR: 0000000000000000 > REGS: c000001e19f077e0 TRAP: 0700 Tainted: G W (5.7.0-rc5-kvm-00211-g9ccf10d6d088) > MSR: 9000000000029033 <SF,HV,EE,ME,IR,DR,RI,LE> CR: 42222422 XER: 20040000 > CFAR: c00000000012f5ac IRQMASK: 0 > GPR00: c008000000fe8488 c000001e19f07a70 c008000000ffe200 0000000000000039 > GPR04: 0000000000000001 c000001ffc8b4900 0000000000018840 0000000000000007 > GPR08: 0000000000000003 0000000000000001 0000000000000007 0000000000000001 > GPR12: 0000000000002000 c000001fff6d9400 000000011f884678 00007fff70b70000 > GPR16: 00007fff7137cb90 00007fff7dcb4410 0000000000000001 0000000000000000 > GPR20: 000000000ffe0000 0000000000000000 0000000000000001 0000000000000000 > GPR24: 8000000000000000 0000000000000001 c000001e1f67e600 c000001e1fd82410 > GPR28: 0000000000001000 c000001e2e410000 0000000000000fff 0000000000000ffe > NIP [c008000000fe848c] kvmppc_hv_get_dirty_log_radix+0x2e4/0x340 [kvm_hv] > LR [c008000000fe8488] kvmppc_hv_get_dirty_log_radix+0x2e0/0x340 [kvm_hv] > Call Trace: > [c000001e19f07a70] [c008000000fe8488] kvmppc_hv_get_dirty_log_radix+0x2e0/0x340 [kvm_hv] (unreliable) > [c000001e19f07b50] [c008000000fd42e4] kvm_vm_ioctl_get_dirty_log_hv+0x33c/0x3c0 [kvm_hv] > [c000001e19f07be0] [c008000000eea878] kvm_vm_ioctl_get_dirty_log+0x30/0x50 [kvm] > [c000001e19f07c00] [c008000000edc818] kvm_vm_ioctl+0x2b0/0xc00 [kvm] > [c000001e19f07d50] [c00000000046e148] ksys_ioctl+0xf8/0x150 > [c000001e19f07da0] [c00000000046e1c8] sys_ioctl+0x28/0x80 > [c000001e19f07dc0] [c00000000003652c] system_call_exception+0x16c/0x240 > [c000001e19f07e20] [c00000000000d070] system_call_common+0xf0/0x278 > Instruction dump: > 7d3a512a 4200ffd0 7ffefb78 4bfffdc4 60000000 3c820000 e8848468 3c620000 > e86384a8 38840010 4800673d e8410018 <0fe00000> 4bfffdd4 60000000 60000000 > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > --- > arch/powerpc/include/asm/kvm_book3s_64.h | 9 ++++++ > arch/powerpc/kvm/book3s_64_mmu_radix.c | 35 ++++++++++++++++++++---- > 2 files changed, 38 insertions(+), 6 deletions(-) > > diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h > index c58e64a0a74f..cd5bad08b8d3 100644 > --- a/arch/powerpc/include/asm/kvm_book3s_64.h > +++ b/arch/powerpc/include/asm/kvm_book3s_64.h > @@ -635,6 +635,15 @@ extern void kvmhv_remove_nest_rmap_range(struct kvm *kvm, > unsigned long gpa, unsigned long hpa, > unsigned long nbytes); > > +static inline pte_t *__find_kvm_secondary_pte(struct kvm *kvm, unsigned long ea, > + unsigned *hshift) > +{ > + pte_t *pte; > + > + pte = __find_linux_pte(kvm->arch.pgtable, ea, NULL, hshift); > + return pte; > +} Why not just open code this in the single caller? Leaving it here someone will invariably decide to call it one day. If you think it's worth keeping then it should have a comment explaining why it doesn't check the lock, and find_kvm_secondary_pte() should call it no? cheers
On 5/28/20 6:23 PM, Michael Ellerman wrote: > "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes: >> This patch fix the below warning reported during migration. >> >> find_kvm_secondary_pte called with kvm mmu_lock not held >> CPU: 23 PID: 5341 Comm: qemu-system-ppc Tainted: G W 5.7.0-rc5-kvm-00211-g9ccf10d6d088 #432 >> NIP: c008000000fe848c LR: c008000000fe8488 CTR: 0000000000000000 >> REGS: c000001e19f077e0 TRAP: 0700 Tainted: G W (5.7.0-rc5-kvm-00211-g9ccf10d6d088) >> MSR: 9000000000029033 <SF,HV,EE,ME,IR,DR,RI,LE> CR: 42222422 XER: 20040000 >> CFAR: c00000000012f5ac IRQMASK: 0 >> GPR00: c008000000fe8488 c000001e19f07a70 c008000000ffe200 0000000000000039 >> GPR04: 0000000000000001 c000001ffc8b4900 0000000000018840 0000000000000007 >> GPR08: 0000000000000003 0000000000000001 0000000000000007 0000000000000001 >> GPR12: 0000000000002000 c000001fff6d9400 000000011f884678 00007fff70b70000 >> GPR16: 00007fff7137cb90 00007fff7dcb4410 0000000000000001 0000000000000000 >> GPR20: 000000000ffe0000 0000000000000000 0000000000000001 0000000000000000 >> GPR24: 8000000000000000 0000000000000001 c000001e1f67e600 c000001e1fd82410 >> GPR28: 0000000000001000 c000001e2e410000 0000000000000fff 0000000000000ffe >> NIP [c008000000fe848c] kvmppc_hv_get_dirty_log_radix+0x2e4/0x340 [kvm_hv] >> LR [c008000000fe8488] kvmppc_hv_get_dirty_log_radix+0x2e0/0x340 [kvm_hv] >> Call Trace: >> [c000001e19f07a70] [c008000000fe8488] kvmppc_hv_get_dirty_log_radix+0x2e0/0x340 [kvm_hv] (unreliable) >> [c000001e19f07b50] [c008000000fd42e4] kvm_vm_ioctl_get_dirty_log_hv+0x33c/0x3c0 [kvm_hv] >> [c000001e19f07be0] [c008000000eea878] kvm_vm_ioctl_get_dirty_log+0x30/0x50 [kvm] >> [c000001e19f07c00] [c008000000edc818] kvm_vm_ioctl+0x2b0/0xc00 [kvm] >> [c000001e19f07d50] [c00000000046e148] ksys_ioctl+0xf8/0x150 >> [c000001e19f07da0] [c00000000046e1c8] sys_ioctl+0x28/0x80 >> [c000001e19f07dc0] [c00000000003652c] system_call_exception+0x16c/0x240 >> [c000001e19f07e20] [c00000000000d070] system_call_common+0xf0/0x278 >> Instruction dump: >> 7d3a512a 4200ffd0 7ffefb78 4bfffdc4 60000000 3c820000 e8848468 3c620000 >> e86384a8 38840010 4800673d e8410018 <0fe00000> 4bfffdd4 60000000 60000000 >> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >> --- >> arch/powerpc/include/asm/kvm_book3s_64.h | 9 ++++++ >> arch/powerpc/kvm/book3s_64_mmu_radix.c | 35 ++++++++++++++++++++---- >> 2 files changed, 38 insertions(+), 6 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h >> index c58e64a0a74f..cd5bad08b8d3 100644 >> --- a/arch/powerpc/include/asm/kvm_book3s_64.h >> +++ b/arch/powerpc/include/asm/kvm_book3s_64.h >> @@ -635,6 +635,15 @@ extern void kvmhv_remove_nest_rmap_range(struct kvm *kvm, >> unsigned long gpa, unsigned long hpa, >> unsigned long nbytes); >> >> +static inline pte_t *__find_kvm_secondary_pte(struct kvm *kvm, unsigned long ea, >> + unsigned *hshift) >> +{ >> + pte_t *pte; >> + >> + pte = __find_linux_pte(kvm->arch.pgtable, ea, NULL, hshift); >> + return pte; >> +} > > Why not just open code this in the single caller? We could do that. But I though it is confusing and we want to avoid using linux page table walker (__find_linux_pte()) directly from within kvm code to walk partition scoped table. > > Leaving it here someone will invariably decide to call it one day. > I was looking at documenting it at the call site. We can possibly add a comment here explaining avoid calling this directly and if used should have a comments around explaining why it is safe. > If you think it's worth keeping then it should have a comment explaining > why it doesn't check the lock, and find_kvm_secondary_pte() should call > it no? > Was trying to avoid that indirection. I guess we should add a comment which suggest to avoid using __find_kvm_secondary_pte() and if used we should ask for comment at the call site explaining why it is safe? -aneesh
On Thu, 28 May 2020 13:34:56 +0530, Aneesh Kumar K.V wrote: > This patch fix the below warning reported during migration. > > find_kvm_secondary_pte called with kvm mmu_lock not held > CPU: 23 PID: 5341 Comm: qemu-system-ppc Tainted: G W 5.7.0-rc5-kvm-00211-g9ccf10d6d088 #432 > NIP: c008000000fe848c LR: c008000000fe8488 CTR: 0000000000000000 > REGS: c000001e19f077e0 TRAP: 0700 Tainted: G W (5.7.0-rc5-kvm-00211-g9ccf10d6d088) > MSR: 9000000000029033 <SF,HV,EE,ME,IR,DR,RI,LE> CR: 42222422 XER: 20040000 > CFAR: c00000000012f5ac IRQMASK: 0 > GPR00: c008000000fe8488 c000001e19f07a70 c008000000ffe200 0000000000000039 > GPR04: 0000000000000001 c000001ffc8b4900 0000000000018840 0000000000000007 > GPR08: 0000000000000003 0000000000000001 0000000000000007 0000000000000001 > GPR12: 0000000000002000 c000001fff6d9400 000000011f884678 00007fff70b70000 > GPR16: 00007fff7137cb90 00007fff7dcb4410 0000000000000001 0000000000000000 > GPR20: 000000000ffe0000 0000000000000000 0000000000000001 0000000000000000 > GPR24: 8000000000000000 0000000000000001 c000001e1f67e600 c000001e1fd82410 > GPR28: 0000000000001000 c000001e2e410000 0000000000000fff 0000000000000ffe > NIP [c008000000fe848c] kvmppc_hv_get_dirty_log_radix+0x2e4/0x340 [kvm_hv] > LR [c008000000fe8488] kvmppc_hv_get_dirty_log_radix+0x2e0/0x340 [kvm_hv] > Call Trace: > [c000001e19f07a70] [c008000000fe8488] kvmppc_hv_get_dirty_log_radix+0x2e0/0x340 [kvm_hv] (unreliable) > [c000001e19f07b50] [c008000000fd42e4] kvm_vm_ioctl_get_dirty_log_hv+0x33c/0x3c0 [kvm_hv] > [c000001e19f07be0] [c008000000eea878] kvm_vm_ioctl_get_dirty_log+0x30/0x50 [kvm] > [c000001e19f07c00] [c008000000edc818] kvm_vm_ioctl+0x2b0/0xc00 [kvm] > [c000001e19f07d50] [c00000000046e148] ksys_ioctl+0xf8/0x150 > [c000001e19f07da0] [c00000000046e1c8] sys_ioctl+0x28/0x80 > [c000001e19f07dc0] [c00000000003652c] system_call_exception+0x16c/0x240 > [c000001e19f07e20] [c00000000000d070] system_call_common+0xf0/0x278 > Instruction dump: > 7d3a512a 4200ffd0 7ffefb78 4bfffdc4 60000000 3c820000 e8848468 3c620000 > e86384a8 38840010 4800673d e8410018 <0fe00000> 4bfffdd4 60000000 60000000 Applied to powerpc/next. [1/1] powerpc/book3s64/kvm: Fix secondary page table walk warning during migration https://git.kernel.org/powerpc/c/bf8036a4098d1548cdccf9ed5c523ef4e83e3c68 cheers
diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h index c58e64a0a74f..cd5bad08b8d3 100644 --- a/arch/powerpc/include/asm/kvm_book3s_64.h +++ b/arch/powerpc/include/asm/kvm_book3s_64.h @@ -635,6 +635,15 @@ extern void kvmhv_remove_nest_rmap_range(struct kvm *kvm, unsigned long gpa, unsigned long hpa, unsigned long nbytes); +static inline pte_t *__find_kvm_secondary_pte(struct kvm *kvm, unsigned long ea, + unsigned *hshift) +{ + pte_t *pte; + + pte = __find_linux_pte(kvm->arch.pgtable, ea, NULL, hshift); + return pte; +} + static inline pte_t *find_kvm_secondary_pte(struct kvm *kvm, unsigned long ea, unsigned *hshift) { diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c index 271f1c3d8443..a328db6a5ef8 100644 --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c @@ -1040,7 +1040,7 @@ static int kvm_radix_test_clear_dirty(struct kvm *kvm, { unsigned long gfn = memslot->base_gfn + pagenum; unsigned long gpa = gfn << PAGE_SHIFT; - pte_t *ptep; + pte_t *ptep, pte; unsigned int shift; int ret = 0; unsigned long old, *rmapp; @@ -1048,12 +1048,35 @@ static int kvm_radix_test_clear_dirty(struct kvm *kvm, if (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_DONE) return ret; - ptep = find_kvm_secondary_pte(kvm, gpa, &shift); - if (ptep && pte_present(*ptep) && pte_dirty(*ptep)) { - ret = 1; - if (shift) - ret = 1 << (shift - PAGE_SHIFT); + /* + * For performance reason we don't hold kvm->mmu_lock + * while walking the partition scoped table. + */ + ptep = __find_kvm_secondary_pte(kvm, gpa, &shift); + if (!ptep) + return 0; + + pte = READ_ONCE(*ptep); + if (pte_present(pte) && pte_dirty(pte)) { spin_lock(&kvm->mmu_lock); + /* + * Recheck the pte again + */ + if (pte_val(pte) != pte_val(*ptep)) { + /* + * We have KVM_MEM_LOG_DIRTY_PAGES enabled. Hence we + * can only find PAGE_SIZE pte entries here. We can + * continue to use the pte addr returned by above + * page table walk. + */ + if (!pte_present(*ptep) || !pte_dirty(*ptep)) { + spin_unlock(&kvm->mmu_lock); + return 0; + } + } + + ret = 1; + VM_BUG_ON(shift); old = kvmppc_radix_update_pte(kvm, ptep, _PAGE_DIRTY, 0, gpa, shift); kvmppc_radix_tlbie_page(kvm, gpa, shift, kvm->arch.lpid);
This patch fix the below warning reported during migration. find_kvm_secondary_pte called with kvm mmu_lock not held CPU: 23 PID: 5341 Comm: qemu-system-ppc Tainted: G W 5.7.0-rc5-kvm-00211-g9ccf10d6d088 #432 NIP: c008000000fe848c LR: c008000000fe8488 CTR: 0000000000000000 REGS: c000001e19f077e0 TRAP: 0700 Tainted: G W (5.7.0-rc5-kvm-00211-g9ccf10d6d088) MSR: 9000000000029033 <SF,HV,EE,ME,IR,DR,RI,LE> CR: 42222422 XER: 20040000 CFAR: c00000000012f5ac IRQMASK: 0 GPR00: c008000000fe8488 c000001e19f07a70 c008000000ffe200 0000000000000039 GPR04: 0000000000000001 c000001ffc8b4900 0000000000018840 0000000000000007 GPR08: 0000000000000003 0000000000000001 0000000000000007 0000000000000001 GPR12: 0000000000002000 c000001fff6d9400 000000011f884678 00007fff70b70000 GPR16: 00007fff7137cb90 00007fff7dcb4410 0000000000000001 0000000000000000 GPR20: 000000000ffe0000 0000000000000000 0000000000000001 0000000000000000 GPR24: 8000000000000000 0000000000000001 c000001e1f67e600 c000001e1fd82410 GPR28: 0000000000001000 c000001e2e410000 0000000000000fff 0000000000000ffe NIP [c008000000fe848c] kvmppc_hv_get_dirty_log_radix+0x2e4/0x340 [kvm_hv] LR [c008000000fe8488] kvmppc_hv_get_dirty_log_radix+0x2e0/0x340 [kvm_hv] Call Trace: [c000001e19f07a70] [c008000000fe8488] kvmppc_hv_get_dirty_log_radix+0x2e0/0x340 [kvm_hv] (unreliable) [c000001e19f07b50] [c008000000fd42e4] kvm_vm_ioctl_get_dirty_log_hv+0x33c/0x3c0 [kvm_hv] [c000001e19f07be0] [c008000000eea878] kvm_vm_ioctl_get_dirty_log+0x30/0x50 [kvm] [c000001e19f07c00] [c008000000edc818] kvm_vm_ioctl+0x2b0/0xc00 [kvm] [c000001e19f07d50] [c00000000046e148] ksys_ioctl+0xf8/0x150 [c000001e19f07da0] [c00000000046e1c8] sys_ioctl+0x28/0x80 [c000001e19f07dc0] [c00000000003652c] system_call_exception+0x16c/0x240 [c000001e19f07e20] [c00000000000d070] system_call_common+0xf0/0x278 Instruction dump: 7d3a512a 4200ffd0 7ffefb78 4bfffdc4 60000000 3c820000 e8848468 3c620000 e86384a8 38840010 4800673d e8410018 <0fe00000> 4bfffdd4 60000000 60000000 Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> --- arch/powerpc/include/asm/kvm_book3s_64.h | 9 ++++++ arch/powerpc/kvm/book3s_64_mmu_radix.c | 35 ++++++++++++++++++++---- 2 files changed, 38 insertions(+), 6 deletions(-)