On 5/28/20 7:13 AM, Paul Mackerras wrote:
On Tue, May 05, 2020 at 12:47:16PM +0530, Aneesh Kumar K.V wrote:
The locking rules for walking partition scoped table is different from process
scoped table. Hence add a helper for secondary linux page table walk and also
add check whether we are holding the right locks.
This patch is causing new warnings to appear when testing migration,
like this:
[ 142.090159] ------------[ cut here ]------------
[ 142.090160] find_kvm_secondary_pte called with kvm mmu_lock not held
[ 142.090176] WARNING: CPU: 23 PID: 5341 at
arch/powerpc/include/asm/kvm_book3s_64.h:644
kvmppc_hv_get_dirty_log_radix+0x2e4/0x340 [kvm_hv]
[ 142.090177] Modules linked in: xt_conntrack nf_conntrack nf_defrag_ipv6
nf_defrag_ipv4 libcrc32c bridge stp llc ebtable_filter ebtables ip6table_filter
ip6_tables bpfilter overlay binfmt_misc input_leds raid_class
scsi_transport_sas sch_fq_codel sunrpc kvm_hv kvm
[ 142.090188] CPU: 23 PID: 5341 Comm: qemu-system-ppc Tainted: G W
5.7.0-rc5-kvm-00211-g9ccf10d6d088 #432
[ 142.090189] NIP: c008000000fe848c LR: c008000000fe8488 CTR: 0000000000000000
[ 142.090190] REGS: c000001e19f077e0 TRAP: 0700 Tainted: G W
(5.7.0-rc5-kvm-00211-g9ccf10d6d088)
[ 142.090191] MSR: 9000000000029033 <SF,HV,EE,ME,IR,DR,RI,LE> CR: 42222422
XER: 20040000
[ 142.090196] 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
[ 142.090217] NIP [c008000000fe848c] kvmppc_hv_get_dirty_log_radix+0x2e4/0x340
[kvm_hv]
[ 142.090223] LR [c008000000fe8488] kvmppc_hv_get_dirty_log_radix+0x2e0/0x340
[kvm_hv]
[ 142.090224] Call Trace:
[ 142.090230] [c000001e19f07a70] [c008000000fe8488]
kvmppc_hv_get_dirty_log_radix+0x2e0/0x340 [kvm_hv] (unreliable)
[ 142.090236] [c000001e19f07b50] [c008000000fd42e4]
kvm_vm_ioctl_get_dirty_log_hv+0x33c/0x3c0 [kvm_hv]
[ 142.090292] [c000001e19f07be0] [c008000000eea878]
kvm_vm_ioctl_get_dirty_log+0x30/0x50 [kvm]
[ 142.090300] [c000001e19f07c00] [c008000000edc818] kvm_vm_ioctl+0x2b0/0xc00
[kvm]
[ 142.090302] [c000001e19f07d50] [c00000000046e148] ksys_ioctl+0xf8/0x150
[ 142.090305] [c000001e19f07da0] [c00000000046e1c8] sys_ioctl+0x28/0x80
[ 142.090307] [c000001e19f07dc0] [c00000000003652c]
system_call_exception+0x16c/0x240
[ 142.090309] [c000001e19f07e20] [c00000000000d070]
system_call_common+0xf0/0x278
[ 142.090310] Instruction dump:
[ 142.090312] 7d3a512a 4200ffd0 7ffefb78 4bfffdc4 60000000 3c820000 e8848468
3c620000
[ 142.090317] e86384a8 38840010 4800673d e8410018 <0fe00000> 4bfffdd4 60000000
60000000
[ 142.090322] ---[ end trace 619d45057b6919e0 ]---
Indeed, kvm_radix_test_clear_dirty() tests the PTE dirty bit
locklessly, and only takes the kvm->mmu_lock once it finds a dirty
PTE. I think that is important for performance, since on any given
scan of the guest real address space we may only find a small
proportion of the guest pages to be dirty.
Are you now relying on the kvm->mmu_lock to protect the existence of
the PTEs, or just their content?
The patch series should not change any rules w.r.t kvm partition scoped
page table walk. We only added helpers to make it explicit that this is
different from regular linux page table walk. And kvm->mmu_lock is what
was used to protect the partition scoped table walk earlier.
In this specific case, what we need probably is an open coded kvm
partition scoped walk with a comment around explaining why is it ok to
walk that partition scoped table without taking kvm->mmu_lock.
What happens when a parallel invalidate happens to Qemu address space?
Since we are not holding kvm->mmu_lock mmu notifier will complete and we
will go ahead with unmapping partition scoped table.
Do we need a change like below?
@@ -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;
@@ -1049,11 +1049,23 @@ static int kvm_radix_test_clear_dirty(struct kvm
*kvm,
return ret;
ptep = find_kvm_secondary_pte(kvm, gpa, &shift);
- if (ptep && pte_present(*ptep) && pte_dirty(*ptep)) {
+ if (!ptep)
+ return 0;
+
+ pte = READ_ONCE(*ptep);
+ if (pte_present(pte) && pte_dirty(pte)) {
ret = 1;
if (shift)
ret = 1 << (shift - PAGE_SHIFT);
spin_lock(&kvm->mmu_lock);
+ /*
+ * Recheck the pte again
+ */
+ if (pte_val(pte) != pte_val(*ptep)) {
+ spin_unlock(&kvm->mmu_lock);
+ return 0;
+ }
+
old = kvmppc_radix_update_pte(kvm, ptep, _PAGE_DIRTY, 0,
gpa, shift);
kvmppc_radix_tlbie_page(kvm, gpa, shift, kvm->arch.lpid);
