All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] powerpc/book3s64/kvm: Fix secondary page table walk warning during migration
@ 2020-05-28  8:16 ` Aneesh Kumar K.V
  0 siblings, 0 replies; 8+ messages in thread
From: Aneesh Kumar K.V @ 2020-05-28  8:04 UTC (permalink / raw)
  To: kvm-ppc, paulus; +Cc: linuxppc-dev, Aneesh Kumar K.V

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;
+}
+
 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);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH] powerpc/book3s64/kvm: Fix secondary page table walk warning during migration
@ 2020-05-28  8:16 ` Aneesh Kumar K.V
  0 siblings, 0 replies; 8+ messages in thread
From: Aneesh Kumar K.V @ 2020-05-28  8:16 UTC (permalink / raw)
  To: kvm-ppc, paulus; +Cc: linuxppc-dev, Aneesh Kumar K.V

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;
+}
+
 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);
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] powerpc/book3s64/kvm: Fix secondary page table walk warning during migration
  2020-05-28  8:16 ` Aneesh Kumar K.V
@ 2020-05-28 12:53   ` Michael Ellerman
  -1 siblings, 0 replies; 8+ messages in thread
From: Michael Ellerman @ 2020-05-28 12:53 UTC (permalink / raw)
  To: Aneesh Kumar K.V, kvm-ppc, paulus; +Cc: Aneesh Kumar K.V, linuxppc-dev

"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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] powerpc/book3s64/kvm: Fix secondary page table walk warning during migration
@ 2020-05-28 12:53   ` Michael Ellerman
  0 siblings, 0 replies; 8+ messages in thread
From: Michael Ellerman @ 2020-05-28 12:53 UTC (permalink / raw)
  To: Aneesh Kumar K.V, kvm-ppc, paulus; +Cc: Aneesh Kumar K.V, linuxppc-dev

"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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] powerpc/book3s64/kvm: Fix secondary page table walk warning during migration
  2020-05-28 12:53   ` Michael Ellerman
@ 2020-05-28 13:20     ` Aneesh Kumar K.V
  -1 siblings, 0 replies; 8+ messages in thread
From: Aneesh Kumar K.V @ 2020-05-28 13:08 UTC (permalink / raw)
  To: Michael Ellerman, kvm-ppc, paulus; +Cc: linuxppc-dev

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] powerpc/book3s64/kvm: Fix secondary page table walk warning during migration
@ 2020-05-28 13:20     ` Aneesh Kumar K.V
  0 siblings, 0 replies; 8+ messages in thread
From: Aneesh Kumar K.V @ 2020-05-28 13:20 UTC (permalink / raw)
  To: Michael Ellerman, kvm-ppc, paulus; +Cc: linuxppc-dev

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] powerpc/book3s64/kvm: Fix secondary page table walk warning during migration
  2020-05-28  8:16 ` Aneesh Kumar K.V
@ 2020-06-09  5:28   ` Michael Ellerman
  -1 siblings, 0 replies; 8+ messages in thread
From: Michael Ellerman @ 2020-06-09  5:28 UTC (permalink / raw)
  To: paulus, Aneesh Kumar K.V, kvm-ppc; +Cc: linuxppc-dev

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] powerpc/book3s64/kvm: Fix secondary page table walk warning during migration
@ 2020-06-09  5:28   ` Michael Ellerman
  0 siblings, 0 replies; 8+ messages in thread
From: Michael Ellerman @ 2020-06-09  5:28 UTC (permalink / raw)
  To: paulus, Aneesh Kumar K.V, kvm-ppc; +Cc: linuxppc-dev

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-06-09  5:37 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-28  8:04 [PATCH] powerpc/book3s64/kvm: Fix secondary page table walk warning during migration Aneesh Kumar K.V
2020-05-28  8:16 ` Aneesh Kumar K.V
2020-05-28 12:53 ` Michael Ellerman
2020-05-28 12:53   ` Michael Ellerman
2020-05-28 13:08   ` Aneesh Kumar K.V
2020-05-28 13:20     ` Aneesh Kumar K.V
2020-06-09  5:28 ` Michael Ellerman
2020-06-09  5:28   ` Michael Ellerman

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.