Messages in this thread | | | Subject | Re: [PATCH 08/11] KVM: MMU: use page track for non-leaf shadow pages | From | Kai Huang <> | Date | Thu, 17 Dec 2015 10:44:12 +0800 |
| |
On 12/16/2015 04:39 PM, Xiao Guangrong wrote: > > > On 12/16/2015 03:51 PM, Kai Huang wrote: >> >> >> On 12/15/2015 05:10 PM, Xiao Guangrong wrote: >>> >>> >>> On 12/15/2015 03:52 PM, Kai Huang wrote: >>> >>>>> static bool __mmu_gfn_lpage_is_disallowed(gfn_t gfn, int level, >>>>> @@ -2140,12 +2150,18 @@ static struct kvm_mmu_page >>>>> *kvm_mmu_get_page(struct kvm_vcpu *vcpu, >>>>> hlist_add_head(&sp->hash_link, >>>>> &vcpu->kvm->arch.mmu_page_hash[kvm_page_table_hashfn(gfn)]); >>>>> if (!direct) { >>>>> - if (rmap_write_protect(vcpu, gfn)) >>>>> + /* >>>>> + * we should do write protection before syncing pages >>>>> + * otherwise the content of the synced shadow page may >>>>> + * be inconsistent with guest page table. >>>>> + */ >>>>> + account_shadowed(vcpu->kvm, sp); >>>>> + >>>>> + if (level == PT_PAGE_TABLE_LEVEL && >>>>> + rmap_write_protect(vcpu, gfn)) >>>>> kvm_flush_remote_tlbs(vcpu->kvm); >>>> I think your modification is good but I am little bit confused >>>> here. In account_shadowed, if >>>> sp->role.level > PT_PAGE_TABLE_LEVEL, the sp->gfn is write >>>> protected, and this is reasonable. So why >>>> write protecting the gfn of PT_PAGE_TABLE_LEVEL here? >>> >>> Because the shadow page will become 'sync' that means the shadow >>> page will be synced >>> with the page table in guest. So the shadow page need to be >>> write-protected to avoid >>> the guest page table is changed when we do the 'sync' thing. >>> >>> The shadow page need to be write-protected to avoid that guest page >>> table is changed >>> when we are syncing the shadow page table. See kvm_sync_pages() >>> after doing >>> rmap_write_protect(). >> I see. So why are you treat PT_PAGE_TABLE_LEVEL gfn separately here? >> why this cannot be done in >> account_shadowed, as you did for upper level sp? > > non-leaf shadow pages are keepking write-protected which page fault > handler can not fix write > access on it. And leaf shadow pages are not. My point is the original code didn't separate the two cases so I am not sure why you need to separate. Perhaps you want to make account_shadowed imply the non-leaf guest page table is write-protected while leaf page table is not.
Thanks, -Kai >> Actually I am thinking whether account_shadowed is >> overdoing things. From it's name it should only *account* shadow sp, >> but now it also does write >> protection and disable large page mapping. >> > > Hmm.. disable large page mapping is already in current code... i think > account_shadowed() can > be understood as new page is taken into account, so protection things > are needed there. > > But I am not good at naming function and also my english is not good > enough, any other better name > is welcome. ;) > >
| |