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 kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
