Re: [PATCH 08/11] KVM: MMU: use page track for non-leaf shadow pages

From: Kai Huang
Date: Wed Dec 16 2015 - 21:48:36 EST




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. ;)



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/