cr3 OOS optimisation breaks 32-bit GNU/kFreeBSD guest
Hi,
Since kvm-81, I have noticed that GNU/kFreeBSD 32-bit guest are crashing under high load (during a compilation for example) with the following error message:
| Fatal trap 12: page fault while in kernel mode | fault virtual address = 0x4 | fault code = supervisor read, page not present | instruction pointer = 0x20:0xc0a4fc00 | stack pointer = 0x28:0xe66d7a70 | frame pointer = 0x28:0xe66d7a80 | code segment = base 0x0, limit 0xfffff, type 0x1b | = DPL 0, pres 1, def32 1, gran 1 | processor eflags = interrupt enabled, resume, IOPL = 0 | current process = 24037 (bash) | trap number = 12 | panic: page fault | Uptime: 4m7s | Cannot dump. No dump device defined. | Automatic reboot in 15 seconds - press a key on the console to abort
I haven't tried yet with a plain FreeBSD guest, but I also expect it to crash given the kernel (version 7.1) is almost the same. A closer investigation has shown that the following commit is causing the problem:
| commit 6364a3918cb5c28376849e7fca3e09bd66b859f3 | Author: Marcelo Tosatti <[email protected]> | Date: Mon Dec 1 22:32:04 2008 -0200 | | KVM: MMU: skip global pgtables on sync due to cr3 switch | | Skip syncing global pages on cr3 switch (but not on cr4/cr0). This is | important for Linux 32-bit guests with PAE, where the kmap page is | marked as global. | | Signed-off-by: Marcelo Tosatti <[email protected]> | Signed-off-by: Avi Kivity <[email protected]>
As expected, loading the KVM module with oos_shadow=0 workaround the problem. Please note that the guest is running in 32-bit mode, does not use PAE, and uses global pages. My host has an Intel Q9450 CPU, and the problem appears with both a 2.6.26 and a 2.6.28 64-bit kernel.
Does anybody see any problem in this patch? How can I further debug the problem?
Aurelien
Signature · 2 lines
Aurelien Jarno GPG: 1024D/F1BCDB73 [email protected] http://www.aurel32.net
Quoted 40 lines · Aurelien Jarno
Hi,
Since kvm-81, I have noticed that GNU/kFreeBSD 32-bit guest are crashing under high load (during a compilation for example) with the following error message:
| Fatal trap 12: page fault while in kernel mode | fault virtual address = 0x4 | fault code = supervisor read, page not present | instruction pointer = 0x20:0xc0a4fc00 | stack pointer = 0x28:0xe66d7a70 | frame pointer = 0x28:0xe66d7a80 | code segment = base 0x0, limit 0xfffff, type 0x1b | = DPL 0, pres 1, def32 1, gran 1 | processor eflags = interrupt enabled, resume, IOPL = 0 | current process = 24037 (bash) | trap number = 12 | panic: page fault | Uptime: 4m7s | Cannot dump. No dump device defined. | Automatic reboot in 15 seconds - press a key on the console to abortI haven't tried yet with a plain FreeBSD guest, but I also expect it to crash given the kernel (version 7.1) is almost the same. A closer investigation has shown that the following commit is causing the problem:
| commit 6364a3918cb5c28376849e7fca3e09bd66b859f3 | Author: Marcelo Tosatti <[email protected]> | Date: Mon Dec 1 22:32:04 2008 -0200 | | KVM: MMU: skip global pgtables on sync due to cr3 switch | | Skip syncing global pages on cr3 switch (but not on cr4/cr0). This is | important for Linux 32-bit guests with PAE, where the kmap page is | marked as global. | | Signed-off-by: Marcelo Tosatti <[email protected]> | Signed-off-by: Avi Kivity <[email protected]>
Quoted 7 lines
As expected, loading the KVM module with oos_shadow=0 workaround the problem. Please note that the guest is running in 32-bit mode, does not use PAE, and uses global pages. My host has an Intel Q9450 CPU, and the problem appears with both a 2.6.26 and a 2.6.28 64-bit kernel.
Does anybody see any problem in this patch? How can I further debug the problem?
Aurelien,
Maybe there is a bug in the syncing code (eg: not all global pages are sync'ed when the OS requests a global sync), or FreeBSD is "relying" on invlpg/cr3 write to sync global pages (remember TLB entries can be invalidated internally by CPU).
If you want to debug it, would suggest looping over all MMU pages in mmu_sync_global, after the kvm_sync_page loop, and
WARN_ON(sp->unsync && sp->global);
If that fails, check if the unsync and global flags mean what they are supposed to.
Sorry for the trouble and thanks for the detailed report, will take a close look at it this week.
Quoted 49 lines · Marcelo Tosatti
Quoted 40 lines · Aurelien Jarno
Hi,
Since kvm-81, I have noticed that GNU/kFreeBSD 32-bit guest are crashing under high load (during a compilation for example) with the following error message:
| Fatal trap 12: page fault while in kernel mode | fault virtual address = 0x4 | fault code = supervisor read, page not present | instruction pointer = 0x20:0xc0a4fc00 | stack pointer = 0x28:0xe66d7a70 | frame pointer = 0x28:0xe66d7a80 | code segment = base 0x0, limit 0xfffff, type 0x1b | = DPL 0, pres 1, def32 1, gran 1 | processor eflags = interrupt enabled, resume, IOPL = 0 | current process = 24037 (bash) | trap number = 12 | panic: page fault | Uptime: 4m7s | Cannot dump. No dump device defined. | Automatic reboot in 15 seconds - press a key on the console to abortI haven't tried yet with a plain FreeBSD guest, but I also expect it to crash given the kernel (version 7.1) is almost the same. A closer investigation has shown that the following commit is causing the problem:
| commit 6364a3918cb5c28376849e7fca3e09bd66b859f3 | Author: Marcelo Tosatti <[email protected]> | Date: Mon Dec 1 22:32:04 2008 -0200 | | KVM: MMU: skip global pgtables on sync due to cr3 switch | | Skip syncing global pages on cr3 switch (but not on cr4/cr0). This is | important for Linux 32-bit guests with PAE, where the kmap page is | marked as global. | | Signed-off-by: Marcelo Tosatti <[email protected]> | Signed-off-by: Avi Kivity <[email protected]>Quoted 7 lines
As expected, loading the KVM module with oos_shadow=0 workaround the problem. Please note that the guest is running in 32-bit mode, does not use PAE, and uses global pages. My host has an Intel Q9450 CPU, and the problem appears with both a 2.6.26 and a 2.6.28 64-bit kernel.
Does anybody see any problem in this patch? How can I further debug the problem?
Quoted 6 lines
Aurelien,
Maybe there is a bug in the syncing code (eg: not all global pages are sync'ed when the OS requests a global sync), or FreeBSD is "relying" on invlpg/cr3 write to sync global pages (remember TLB entries can be invalidated internally by CPU).
As far as I understand the Intel IA32 manual, using invlpg to sync global pages is correct. OTOH, cr3 is not. I think that if FreeBSD is using cr3 to sync global pages that should also cause a problem on real hardware sooner or later, right?
I'll try to look at the kernel code.
Quoted 7 lines
If you want to debug it, would suggest looping over all MMU pages in
mmu_sync_global, after thekvm_sync_pageloop, andWARN_ON(sp->unsync && sp->global);If that fails, check if the unsync and global flags mean what they are supposed to.
This doesn't detect any problem, which means that all pages marked as global are synced correctly.
I have also tried to call kvm_mmu_sync_global() in kvm_set_cr3(), and as expected the guest works correctly in that case.
If I understand correctly, and if the problem is not on the FreeBSD side, the only remaining possible problem is if normal pages are wrongly marked as global, and thus should be synced with cr3, while they are not. Am I right here?
Quoted 3 lines
Sorry for the trouble and thanks for the detailed report, will take a close look at it this week.
Thanks for your fast answer and for your help for debugging.
Signature · 2 lines
Aurelien Jarno GPG: 1024D/F1BCDB73 [email protected] http://www.aurel32.net
Quoted 9 lines · Aurelien Jarno
Quoted 4 lines
Maybe there is a bug in the syncing code (eg: not all global pages are sync'ed when the OS requests a global sync), or FreeBSD is "relying" on invlpg/cr3 write to sync global pages (remember TLB entries can be invalidated internally by CPU).
As far as I understand the Intel IA32 manual, using invlpg to sync global pages is correct. OTOH, cr3 is not. I think that if FreeBSD is using cr3 to sync global pages that should also cause a problem on real hardware sooner or later, right?
From my understanding of the documentation, yes. Note:
5.1 Invalidation Instructions
The processor is always free to invalidate additional entries in the TLBs and paging-structure caches. The following are some examples:
• INVLPG may invalidate TLB entries for pages other than the one corresponding to its linear-address operand.
• MOV to CR3 may invalidate TLB entries for global pages.
^^^
• On a processor supporting Hyper-Threading Technology, invalidations performed on one logical processor may invalidate entries in the TLBs and paging-structure caches used by other logical processors.
Quoted 13 lines
I'll try to look at the kernel code.
Quoted 7 lines
If you want to debug it, would suggest looping over all MMU pages in
mmu_sync_global, after thekvm_sync_pageloop, andWARN_ON(sp->unsync && sp->global);If that fails, check if the unsync and global flags mean what they are supposed to.
This doesn't detect any problem, which means that all pages marked as global are synced correctly.
Quoted 7 lines
I have also tried to call
kvm_mmu_sync_global()inkvm_set_cr3(), and as expected the guest works correctly in that case.If I understand correctly, and if the problem is not on the FreeBSD side, the only remaining possible problem is if normal pages are wrongly marked as global, and thus should be synced with cr3, while they are not. Am I right here?
Yes, this is the most likely problematic scenario.
Quoted 5 lines
Quoted 3 lines
Sorry for the trouble and thanks for the detailed report, will take a close look at it this week.
Thanks for your fast answer and for your help for debugging.
If you confirm that FreeBSD is indeed relying on cr3 to sync global pages, it might be better to disable the optimization. Lets hope that is not the case.
Thanks
Quoted 7 lines · Marcelo Tosatti
Quoted 2 lines
Thanks for your fast answer and for your help for debugging.
If you confirm that FreeBSD is indeed relying on cr3 to sync global pages, it might be better to disable the optimization. Lets hope that is not the case.
cr3 writes explicitly do not flush global pages; otherwise what would be the point of global pages at all?
In other words, the only difference between global and non-global pages is visible via cr3 writes.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 11 lines · Avi Kivity
Quoted 7 lines · Marcelo Tosatti
Quoted 2 lines
Thanks for your fast answer and for your help for debugging.
If you confirm that FreeBSD is indeed relying on cr3 to sync global pages, it might be better to disable the optimization. Lets hope that is not the case.
cr3 writes explicitly do not flush global pages; otherwise what would be the point of global pages at all?
From the Intel TLB doc:
The processor is always free to invalidate additional entries in the TLBs and paging-structure caches. The following are some examples:
• MOV to CR3 may invalidate TLB entries for global pages.
The reasoning was if an optimization breaks an important guest which contains a bug that happens to not trigger on real HW due to positioning of the stars, it is reasonable to disable that optimization.
Quoted 2 lines
In other words, the only difference between global and non-global pages is visible via cr3 writes.
Quoted 17 lines · Marcelo Tosatti
Quoted 5 lines · Avi Kivity
Quoted 3 lines · Marcelo Tosatti
Quoted 3 lines
Thanks for your fast answer and for your help for debugging.
Quoted 7 lines
Quoted 5 lines
If you confirm that FreeBSD is indeed relying on cr3 to sync global pages, it might be better to disable the optimization. Lets hope that is not the case.
cr3 writes explicitly do not flush global pages; otherwise what would be the point of global pages at all?
From the Intel TLB doc:
Quoted 9 lines
The processor is always free to invalidate additional entries in the TLBs and paging-structure caches. The following are some examples:
• MOV to CR3 may invalidate TLB entries for global pages.
The reasoning was if an optimization breaks an important guest which contains a bug that happens to not trigger on real HW due to positioning of the stars, it is reasonable to disable that optimization.
This means the OS may not rely on the TLB retaining its contents. For example, you can't do
1. set pte to global+present
2. access through pte to load tlb entry
3. clear pte
4. switch cr3
5. access through same pte again, relying on tlb entry to service the
access
So the processor may choose to ignore the global bit on some or all tlb entries, but software cannot assume that it does. Typically it will honor the global bit since otherwise it's useless.
I don't think this is what is happening with FreeBSD. It may be that spte population on invlpg is confusing the guest (though that is allowed as a speculative read?). For example, the sequence:
1. invlpg
2. set pte to A (present+accessed)
3. set pte to B (present+accessed)
kvm behaves as if a speculative read always happens between 2 and 3, which would be very rare on real hardware.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 6 lines · Aurelien Jarno
Hi,
Since kvm-81, I have noticed that GNU/kFreeBSD 32-bit guest are crashing under high load (during a compilation for example) with the following error message:
Quoted 15 lines
| Fatal trap 12: page fault while in kernel mode | fault virtual address = 0x4 | fault code = supervisor read, page not present | instruction pointer = 0x20:0xc0a4fc00 | stack pointer = 0x28:0xe66d7a70 | frame pointer = 0x28:0xe66d7a80 | code segment = base 0x0, limit 0xfffff, type 0x1b | = DPL 0, pres 1, def32 1, gran 1 | processor eflags = interrupt enabled, resume, IOPL = 0 | current process = 24037 (bash) | trap number = 12 | panic: page fault | Uptime: 4m7s | Cannot dump. No dump device defined. | Automatic reboot in 15 seconds - press a key on the console to abort
Aurelien,
Can you try this patch please.
-------
KVM: MMU: do not allow unsync global pages to become unreachable
Section 5.1 of the Intel TLB doc:
"• INVLPG. This instruction takes a single operand, which is a linear address. The instruction invalidates any TLB entries for the page number of that linear address including those for global pages (see Section 7). INVLPG also invalidates all entries in all paging-structure caches regardless of the linear addresses to which they correspond."
Section 7:
"The following items detail the invalidation of global TLB entries:
• An execution of INVLPG invalidates any TLB entries for the page number of the linear address that is its operand, even if the entries are global."
If an unsync global page becomes unreachable via the shadow tree, which can happen if one its parent pages is zapped, invlpg will fail to invalidate translations for gvas contained in such unreachable pages.
So invalidate all unsync global entries when zapping a page. Another possibility would be to cover global pages in the unsync bitmaps, but that would increase overhead for mmu_sync_roots.
arch/x86/kvm/mmu.c +12 -0
@@ -1362,6 +1362,16 @@ static void kvm_mmu_unlink_parents(struc } } +static void mmu_invalidate_unsync_global (struct kvm *kvm) +{ +struct kvm_mmu_page *sp, *n; + +list_for_each_entry_safe (sp, n, &kvm->arch.oos_global_pages, oos_link) { +kvm_mmu_page_unlink_children (kvm, sp); +kvm_unlink_unsync_page (kvm, sp); + } +} +static int mmu_zap_unsync_children (struct kvm *kvm,struct kvm_mmu_page *parent) {
@@ -1384,6 +1394,8 @@ static int mmu_zap_unsync_children(struckvm_mmu_pages_init (parent, &parents, &pages); } +mmu_invalidate_unsync_global (kvm); +return zapped; }
Quoted 23 lines · Marcelo Tosatti
Quoted 6 lines · Aurelien Jarno
Hi,
Since kvm-81, I have noticed that GNU/kFreeBSD 32-bit guest are crashing under high load (during a compilation for example) with the following error message:
Quoted 15 lines
| Fatal trap 12: page fault while in kernel mode | fault virtual address = 0x4 | fault code = supervisor read, page not present | instruction pointer = 0x20:0xc0a4fc00 | stack pointer = 0x28:0xe66d7a70 | frame pointer = 0x28:0xe66d7a80 | code segment = base 0x0, limit 0xfffff, type 0x1b | = DPL 0, pres 1, def32 1, gran 1 | processor eflags = interrupt enabled, resume, IOPL = 0 | current process = 24037 (bash) | trap number = 12 | panic: page fault | Uptime: 4m7s | Cannot dump. No dump device defined. | Automatic reboot in 15 seconds - press a key on the console to abort
Quoted 3 lines
Aurelien,
Can you try this patch please.
I have just tried it, and it works. Thanks a lot for your work!
Quoted 60 lines
-------
KVM: MMU: do not allow unsync global pages to become unreachable
Section 5.1 of the Intel TLB doc:
"• INVLPG. This instruction takes a single operand, which is a linear address. The instruction invalidates any TLB entries for the page number of that linear address including those for global pages (see Section 7). INVLPG also invalidates all entries in all paging-structure caches regardless of the linear addresses to which they correspond."
Section 7:
"The following items detail the invalidation of global TLB entries:
• An execution of INVLPG invalidates any TLB entries for the page number of the linear address that is its operand, even if the entries are global."
If an unsync global page becomes unreachable via the shadow tree, which can happen if one its parent pages is zapped, invlpg will fail to invalidate translations for gvas contained in such unreachable pages.
So invalidate all unsync global entries when zapping a page. Another possibility would be to cover global pages in the unsync bitmaps, but that would increase overhead for
mmu_sync_roots.Index:
kvm/arch/x86/kvm/mmu.c
===================================================================arch/x86/kvm/mmu.c +12
-0@@ -1362,6 +1362,16 @@ static void kvm_mmu_unlink_parents(struc } } +static void mmu_invalidate_unsync_global (struct kvm *kvm) +{ +struct kvm_mmu_page *sp, *n; + +list_for_each_entry_safe (sp, n, &kvm->arch.oos_global_pages, oos_link) { +kvm_mmu_page_unlink_children (kvm, sp); +kvm_unlink_unsync_page (kvm, sp); + } +} +static int mmu_zap_unsync_children (struct kvm *kvm,struct kvm_mmu_page *parent) {@@ -1384,6 +1394,8 @@ static int mmu_zap_unsync_children(struckvm_mmu_pages_init (parent, &parents, &pages); } +mmu_invalidate_unsync_global (kvm); +return zapped; }
Signature · 2 lines
Aurelien Jarno GPG: 1024D/F1BCDB73 [email protected] http://www.aurel32.net
Quoted 79 lines · Marcelo Tosatti
Quoted 5 lines · Aurelien Jarno
Hi,
Since kvm-81, I have noticed that GNU/kFreeBSD 32-bit guest are crashing under high load (during a compilation for example) with the following error message:
Quoted 16 lines
| Fatal trap 12: page fault while in kernel mode | fault virtual address = 0x4 | fault code = supervisor read, page not present | instruction pointer = 0x20:0xc0a4fc00 | stack pointer = 0x28:0xe66d7a70 | frame pointer = 0x28:0xe66d7a80 | code segment = base 0x0, limit 0xfffff, type 0x1b | = DPL 0, pres 1, def32 1, gran 1 | processor eflags = interrupt enabled, resume, IOPL = 0 | current process = 24037 (bash) | trap number = 12 | panic: page fault | Uptime: 4m7s | Cannot dump. No dump device defined. | Automatic reboot in 15 seconds - press a key on the console to abortAurelien,
Can you try this patch please.
-------
KVM: MMU: do not allow unsync global pages to become unreachable
Section 5.1 of the Intel TLB doc:
"• INVLPG. This instruction takes a single operand, which is a linear address. The instruction invalidates any TLB entries for the page number of that linear address including those for global pages (see Section 7). INVLPG also invalidates all entries in all paging-structure caches regardless of the linear addresses to which they correspond."
Section 7:
"The following items detail the invalidation of global TLB entries:
• An execution of INVLPG invalidates any TLB entries for the page number of the linear address that is its operand, even if the entries are global."
If an unsync global page becomes unreachable via the shadow tree, which can happen if one its parent pages is zapped, invlpg will fail to invalidate translations for gvas contained in such unreachable pages.
So invalidate all unsync global entries when zapping a page. Another possibility would be to cover global pages in the unsync bitmaps, but that would increase overhead for
mmu_sync_roots.Index:
kvm/arch/x86/kvm/mmu.c
===================================================================arch/x86/kvm/mmu.c +10
-0@@ -1362,6 +1362,16 @@ static void kvm_mmu_unlink_parents(struc } } +static void mmu_invalidate_unsync_global (struct kvm *kvm) +{ +struct kvm_mmu_page *sp, *n; + +list_for_each_entry_safe (sp, n, &kvm->arch.oos_global_pages, oos_link) { +kvm_mmu_page_unlink_children (kvm, sp); +kvm_unlink_unsync_page (kvm, sp); + } +} +static int mmu_zap_unsync_children (struct kvm *kvm,struct kvm_mmu_page *parent) {
arch/x86/kvm/mmu.c static int mmu_zap_unsync_children(struc · line 1401
Quoted 8 lines
arch/x86/kvm/mmu.c +2
-0@@ -1384,6 +1394,8 @@ static int mmu_zap_unsync_children(struckvm_mmu_pages_init (parent, &parents, &pages); } +mmu_invalidate_unsync_global (kvm); +return zapped; }
Good catch, indeed. But is it sufficient? We could unlink a page through other means, for example by the guest zapping a page directory entry. Maybe it's best to resync when relinking a global page?
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 3 lines · Avi Kivity
Good catch, indeed. But is it sufficient? We could unlink a page through other means, for example by the guest zapping a page directory entry.
Yep.
Quoted 1 line
Maybe it's best to resync when relinking a global page?
How about this. It will shorten the unsync period of global pages, unfortunately.
arch/x86/kvm/mmu.c +4 -0
@@ -1238,6 +1238,10 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,set_bit (KVM_REQ_MMU_SYNC , &vcpu->requests);kvm_mmu_mark_parents_unsync (vcpu, sp); } +if (role.level !=PT_PAGE_TABLE_LEVEL && + !list_empty (&vcpu->kvm->arch.oos_global_pages)) +set_bit (KVM_REQ_MMU_GLOBAL_SYNC , &vcpu->requests); +pgprintk ("%s: found\n" , __func__);return sp; }
arch/x86/kvm/x86.c +2 -0
@@ -3109,6 +3109,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)kvm_write_guest_time (vcpu);if (test_and_clear_bit (KVM_REQ_MMU_SYNC , &vcpu->requests))kvm_mmu_sync_roots (vcpu); +if (test_and_clear_bit (KVM_REQ_MMU_GLOBAL_SYNC , &vcpu->requests)) +kvm_mmu_sync_global (vcpu);if (test_and_clear_bit (KVM_REQ_TLB_FLUSH , &vcpu->requests)) kvm_x86_ops->tlb_flush (vcpu); if (test_and_clear_bit(KVM_REQ_REPORT_TPR_ACCESS,
include/linux/kvm_host.h +2 -1
@@ -37,7 +37,8 @@#define KVM_REQ_PENDING_TIMER 5#define KVM_REQ_UNHALT 6#define KVM_REQ_MMU_SYNC 7 -#define KVM_REQ_KVMCLOCK_UPDATE 8 +#define KVM_REQ_MMU_GLOBAL_SYNC 8 +#define KVM_REQ_KVMCLOCK_UPDATE 9#define KVM_USERSPACE_IRQ_SOURCE_ID 0
Quoted 26 lines · Marcelo Tosatti
Quoted 2 lines
Maybe it's best to resync when relinking a global page?
How about this. It will shorten the unsync period of global pages, unfortunately.
arch/x86/kvm/mmu.c +4
-0@@ -1238,6 +1238,10 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,set_bit (KVM_REQ_MMU_SYNC , &vcpu->requests);kvm_mmu_mark_parents_unsync (vcpu, sp); } +if (role.level !=PT_PAGE_TABLE_LEVEL && + !list_empty (&vcpu->kvm->arch.oos_global_pages)) +set_bit (KVM_REQ_MMU_GLOBAL_SYNC , &vcpu->requests); +pgprintk ("%s: found\n" , __func__);return sp; }arch/x86/kvm/x86.c +0
-0
arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) · line 3116
Quoted 8 lines
arch/x86/kvm/x86.c +2
-0@@ -3109,6 +3109,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)kvm_write_guest_time (vcpu);if (test_and_clear_bit (KVM_REQ_MMU_SYNC , &vcpu->requests))kvm_mmu_sync_roots (vcpu); +if (test_and_clear_bit (KVM_REQ_MMU_GLOBAL_SYNC , &vcpu->requests)) +kvm_mmu_sync_global (vcpu);if (test_and_clear_bit (KVM_REQ_TLB_FLUSH , &vcpu->requests)) kvm_x86_ops->tlb_flush (vcpu); if (test_and_clear_bit(KVM_REQ_REPORT_TPR_ACCESS
Windows will (I think) write a PDE on every context switch, so this effectively disables global unsync for that guest.
What about recursively syncing the newly linked page in FNAME(fetch)()? If the page isn't global, this becomes a no-op, so no new overhead. The only question is the expense when linking a populated top-level page, especially in long mode.
Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
Quoted 39 lines · Avi Kivity
Quoted 26 lines · Marcelo Tosatti
Quoted 2 lines
Maybe it's best to resync when relinking a global page?
How about this. It will shorten the unsync period of global pages, unfortunately.
arch/x86/kvm/mmu.c +4
-0@@ -1238,6 +1238,10 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,set_bit (KVM_REQ_MMU_SYNC , &vcpu->requests);kvm_mmu_mark_parents_unsync (vcpu, sp); } +if (role.level !=PT_PAGE_TABLE_LEVEL && + !list_empty (&vcpu->kvm->arch.oos_global_pages)) +set_bit (KVM_REQ_MMU_GLOBAL_SYNC , &vcpu->requests); +pgprintk ("%s: found\n" , __func__);return sp; }arch/x86/kvm/x86.c +0
-0arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) · line 3116
Quoted 8 lines
arch/x86/kvm/x86.c +2
-0@@ -3109,6 +3109,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)kvm_write_guest_time (vcpu);if (test_and_clear_bit (KVM_REQ_MMU_SYNC , &vcpu->requests))kvm_mmu_sync_roots (vcpu); +if (test_and_clear_bit (KVM_REQ_MMU_GLOBAL_SYNC , &vcpu->requests)) +kvm_mmu_sync_global (vcpu);if (test_and_clear_bit (KVM_REQ_TLB_FLUSH , &vcpu->requests)) kvm_x86_ops->tlb_flush (vcpu); if (test_and_clear_bit(KVM_REQ_REPORT_TPR_ACCESSWindows will (I think) write a PDE on every context switch, so this effectively disables global unsync for that guest.
Quoted 4 lines
What about recursively syncing the newly linked page in FNAME(fetch)()? If the page isn't global, this becomes a no-op, so no new overhead. The only question is the expense when linking a populated top-level page, especially in long mode.
Yes, I started doing that but it touches the nice fastpath in fetch(). I'll see if I can come up with something and with numbers.
Thanks
Quoted 16 lines · Avi Kivity
arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) · line 3116
Quoted 12 lines
index 2ea8262..48169d7 100644
arch/x86/kvm/x86.c +2
-0@@ -3109,6 +3109,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)kvm_write_guest_time (vcpu);if (test_and_clear_bit (KVM_REQ_MMU_SYNC , &vcpu->requests))kvm_mmu_sync_roots (vcpu); +if (test_and_clear_bit (KVM_REQ_MMU_GLOBAL_SYNC , &vcpu->requests)) +kvm_mmu_sync_global (vcpu);if (test_and_clear_bit (KVM_REQ_TLB_FLUSH , &vcpu->requests)) kvm_x86_ops->tlb_flush (vcpu); if (test_and_clear_bit(KVM_REQ_REPORT_TPR_ACCESSWindows will (I think) write a PDE on every context switch, so this effectively disables global unsync for that guest.
Quoted 4 lines
What about recursively syncing the newly linked page in FNAME(fetch)()? If the page isn't global, this becomes a no-op, so no new overhead. The only question is the expense when linking a populated top-level page, especially in long mode.
How about this?
KVM: MMU: sync global pages on fetch()
If an unsync global page becomes unreachable via the shadow tree, which can happen if one its parent pages is zapped, invlpg will fail to invalidate translations for gvas contained in such unreachable pages.
So sync global pages in fetch().
arch/x86/kvm/paging_tmpl.h +7 -1
@@ -308,8 +308,14 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,break ; } -if (is_shadow_present_pte (*sptep) && !is_large_pte (*sptep)) +if (is_shadow_present_pte (*sptep) && !is_large_pte (*sptep)) { +if (level-1 ==PT_PAGE_TABLE_LEVEL ) { + shadow_page =page_header (__pa (sptep)); +if (shadow_page->unsync && shadow_page->global) +kvm_sync_page (vcpu, shadow_page); + }continue ; + }if (is_large_pte (*sptep)) {rmap_remove (vcpu->kvm, sptep);
Quoted 43 lines · Marcelo Tosatti
Quoted 15 lines · Avi Kivity
arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) · line 3116
Quoted 13 lines
index 2ea8262..48169d7 100644
arch/x86/kvm/x86.c +2
-0@@ -3109,6 +3109,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)kvm_write_guest_time (vcpu);if (test_and_clear_bit (KVM_REQ_MMU_SYNC , &vcpu->requests))kvm_mmu_sync_roots (vcpu); +if (test_and_clear_bit (KVM_REQ_MMU_GLOBAL_SYNC , &vcpu->requests)) +kvm_mmu_sync_global (vcpu);if (test_and_clear_bit (KVM_REQ_TLB_FLUSH , &vcpu->requests)) kvm_x86_ops->tlb_flush (vcpu); if (test_and_clear_bit(KVM_REQ_REPORT_TPR_ACCESSWindows will (I think) write a PDE on every context switch, so this effectively disables global unsync for that guest.
Quoted 2 lines
What about recursively syncing the newly linked page in FNAME(fetch)()? If the page isn't global, this becomes a no-op, so no new overhead. The only question is the expense when linking a populated top-level page, especially in long mode.
How about this?
KVM: MMU: sync global pages on
fetch()If an unsync global page becomes unreachable via the shadow tree, which can happen if one its parent pages is zapped, invlpg will fail to invalidate translations for gvas contained in such unreachable pages.
So sync global pages in
fetch().Signed-off-by Marcelo Tosatti <[email protected]>arch/x86/kvm/paging_tmpl.h +1
-1@@ -308,8 +308,14 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,break ; } -if (is_shadow_present_pte (*sptep) && !is_large_pte (*sptep)) +if (is_shadow_present_pte (*sptep) && !is_large_pte (*sptep)) {
arch/x86/kvm/paging_tmpl.h static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr, · line 321
Quoted 10 lines
arch/x86/kvm/paging_tmpl.h +6
-0+if (level-1 ==PT_PAGE_TABLE_LEVEL ) { + shadow_page =page_header (__pa (sptep)); +if (shadow_page->unsync && shadow_page->global) +kvm_sync_page (vcpu, shadow_page); + }continue ; + }if (is_large_pte (*sptep)) {rmap_remove (vcpu->kvm, sptep);
But here the shadow page is already linked? Isn't the root cause that an invlpg was called when the page wasn't linked, so it wasn't seen by invlpg?
So I thought the best place would be in fetch(), after kvm_mmu_get_page(). If we're linking a page which contains global ptes, they might be unsynced due to invlpgs that we've missed.
Or am I missing something about the root cause?
Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.
Quoted 24 lines · Avi Kivity
Quoted 9 lines
arch/x86/kvm/paging_tmpl.h +1
-0@@ -308,8 +308,14 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,break ; } - if (is_shadow_present_pte(*sptep) && !is_large_pte(*sptep)) +if (is_shadow_present_pte (*sptep) && !is_large_pte (*sptep)) {arch/x86/kvm/paging_tmpl.h static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr, · line 321
Quoted 10 lines
arch/x86/kvm/paging_tmpl.h +6
-0+if (level-1 ==PT_PAGE_TABLE_LEVEL ) { + shadow_page =page_header (__pa (sptep)); +if (shadow_page->unsync && shadow_page->global) +kvm_sync_page (vcpu, shadow_page); + }continue ; + }if (is_large_pte (*sptep)) {rmap_remove (vcpu->kvm, sptep);But here the shadow page is already linked? Isn't the root cause that an invlpg was called when the page wasn't linked, so it wasn't seen by invlpg?
Quoted 5 lines
So I thought the best place would be in
fetch(), afterkvm_mmu_get_page(). If we're linking a page which contains global ptes, they might be unsynced due to invlpgs that we've missed.Or am I missing something about the root cause?
The problem is when the page is unreachable due to a higher level path being unlinked. Say:
level 4 -> level 3 . level 2 -> level 1 (global unsync)
The dot there means level 3 is not linked to level 2, so invlpg can't reach the global unsync at level 1.
kvm_mmu_get_page does sync pages when it finds them, so the code is already safe for the "linking a page which contains global ptes" case you mention above.
Quoted 12 lines · Marcelo Tosatti
The problem is when the page is unreachable due to a higher level path being unlinked. Say:
level 4 -> level 3 . level 2 -> level 1 (global unsync)
The dot there means level 3 is not linked to level 2, so invlpg can't reach the global unsync at level 1.
kvm_mmu_get_pagedoes sync pages when it finds them, so the code is already safe for the "linking a page which contains global ptes" case you mention above.
The recursive resync ignores global pages (or it would hit them on cr3 switch too).
But we have a bigger problem, invlpg can miss even if nothing is unlinked:
access address x through global pte
-> pte instantiated into spte
switch to cr3 where x is not mapped, or mapped differently write to pte
-> no fault since pte is unsync
invlpg x
-> no way this can hit the spte
switch cr3 back
access address x
-> use old spte
Here's one way to make this work:
- add a hash of global pagetables, indexed by virtual address instead
of the pagetable's gfn
- invlpg checks this hash in addition to the recursive walk
We'd need to make the virtual address part of sp->role to avoid needing to link the same page multiple times in the virtual address hash.
Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.
Quoted 30 lines · Avi Kivity
Quoted 12 lines · Marcelo Tosatti
The problem is when the page is unreachable due to a higher level path being unlinked. Say:
level 4 -> level 3 . level 2 -> level 1 (global unsync)
The dot there means level 3 is not linked to level 2, so invlpg can't reach the global unsync at level 1.
kvm_mmu_get_pagedoes sync pages when it finds them, so the code is already safe for the "linking a page which contains global ptes" case you mention above.The recursive resync ignores global pages (or it would hit them on cr3 switch too).
But we have a bigger problem, invlpg can miss even if nothing is unlinked:
access address x through global pte
-> pte instantiated into spte
switch to cr3 where x is not mapped, or mapped differently write to pte
-> no fault since pte is unsync
invlpg x
-> no way this can hit the spte
switch cr3 back
access address x
-> use old spte
Quoted 8 lines
Here's one way to make this work:
- add a hash of global pagetables, indexed by virtual address instead
of the pagetable's gfn
- invlpg checks this hash in addition to the recursive walkWe'd need to make the virtual address part of
sp->roleto avoid needing to link the same page multiple times in the virtual address hash.
Humpf, yes. It seems its too expensive/complex to handle this, for such small gain (~= 2% on AIM7 with RHEL3 guest).
Are you okay with just disabling the global pages optimization?
Quoted 6 lines · Marcelo Tosatti
Quoted 6 lines
Here's one way to make this work:
- add a hash of global pagetables, indexed by virtual address instead
of the pagetable's gfn
- invlpg checks this hash in addition to the recursive walk
Quoted 8 lines
Quoted 2 lines
We'd need to make the virtual address part of
sp->roleto avoid needing to link the same page multiple times in the virtual address hash.Humpf, yes. It seems its too expensive/complex to handle this, for such small gain (~= 2% on AIM7 with RHEL3 guest).
Are you okay with just disabling the global pages optimization?
Definitely to plug the hole; and probably for later as well, unless people cry out due to regressions.
Please send it in two patches: one a trivial one to disable global page detection which can be sent to -stable as well, and a follow on which rips out the global page machinery until (and if) we decide to reimplement it correctly.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 26 lines · Marcelo Tosatti
Quoted 16 lines · Avi Kivity
arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) · line 3116
Quoted 12 lines
index 2ea8262..48169d7 100644
arch/x86/kvm/x86.c +2
-0@@ -3109,6 +3109,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)kvm_write_guest_time (vcpu);if (test_and_clear_bit (KVM_REQ_MMU_SYNC , &vcpu->requests))kvm_mmu_sync_roots (vcpu); +if (test_and_clear_bit (KVM_REQ_MMU_GLOBAL_SYNC , &vcpu->requests)) +kvm_mmu_sync_global (vcpu);if (test_and_clear_bit (KVM_REQ_TLB_FLUSH , &vcpu->requests)) kvm_x86_ops->tlb_flush (vcpu); if (test_and_clear_bit(KVM_REQ_REPORT_TPR_ACCESSWindows will (I think) write a PDE on every context switch, so this effectively disables global unsync for that guest.
Quoted 4 lines
What about recursively syncing the newly linked page in FNAME(fetch)()? If the page isn't global, this becomes a no-op, so no new overhead. The only question is the expense when linking a populated top-level page, especially in long mode.
How about this?
KVM: MMU: sync global pages on
fetch()
Quoted 7 lines
If an unsync global page becomes unreachable via the shadow tree, which can happen if one its parent pages is zapped, invlpg will fail to invalidate translations for gvas contained in such unreachable pages.
So sync global pages in
fetch().Signed-off-by Marcelo Tosatti <[email protected]>
I have tried this patch, and unfortunately it does not solve the original problem, while the previous one did.
Signature · 2 lines
Aurelien Jarno GPG: 1024D/F1BCDB73 [email protected] http://www.aurel32.net
Quoted 11 lines · Marcelo Tosatti
Quoted 3 lines · Avi Kivity
Good catch, indeed. But is it sufficient? We could unlink a page through other means, for example by the guest zapping a page directory entry.
Yep.
Quoted 1 line
Maybe it's best to resync when relinking a global page?
How about this. It will shorten the unsync period of global pages, unfortunately.
JFYI, it still fixes the problem seen with FreeBSD.
Quoted 43 lines
arch/x86/kvm/mmu.c +4
-0@@ -1238,6 +1238,10 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,set_bit (KVM_REQ_MMU_SYNC , &vcpu->requests);kvm_mmu_mark_parents_unsync (vcpu, sp); } +if (role.level !=PT_PAGE_TABLE_LEVEL && + !list_empty (&vcpu->kvm->arch.oos_global_pages)) +set_bit (KVM_REQ_MMU_GLOBAL_SYNC , &vcpu->requests); +pgprintk ("%s: found\n" , __func__);return sp; }arch/x86/kvm/x86.c +2
-0@@ -3109,6 +3109,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)kvm_write_guest_time (vcpu);if (test_and_clear_bit (KVM_REQ_MMU_SYNC , &vcpu->requests))kvm_mmu_sync_roots (vcpu); +if (test_and_clear_bit (KVM_REQ_MMU_GLOBAL_SYNC , &vcpu->requests)) +kvm_mmu_sync_global (vcpu);if (test_and_clear_bit (KVM_REQ_TLB_FLUSH , &vcpu->requests)) kvm_x86_ops->tlb_flush (vcpu); if (test_and_clear_bit(KVM_REQ_REPORT_TPR_ACCESS,include/linux/kvm_host.h +2
-1@@ -37,7 +37,8 @@#define KVM_REQ_PENDING_TIMER 5#define KVM_REQ_UNHALT 6#define KVM_REQ_MMU_SYNC 7 -#define KVM_REQ_KVMCLOCK_UPDATE 8 +#define KVM_REQ_MMU_GLOBAL_SYNC 8 +#define KVM_REQ_KVMCLOCK_UPDATE 9#define KVM_USERSPACE_IRQ_SOURCE_ID 0
Signature · 2 lines
Aurelien Jarno GPG: 1024D/F1BCDB73 [email protected] http://www.aurel32.net