On 27/09/12 04:11, Min-gyu Kim wrote:

quoted

-----Original Message-----
From: kvm-owner at vger.kernel.org [mailto:kvm-owner at vger.kernel.org] On
Behalf Of Christoffer Dall
Sent: Tuesday, September 25, 2012 9:39 PM
To: Min-gyu Kim
Cc: kvm at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
kvmarm at lists.cs.columbia.edu; ???
Subject: Re: [PATCH 13/15] KVM: ARM: Handle guest faults in KVM

quoted

quoted

+
+     /*
+      * If this is a write fault (think COW) we need to make sure the
+      * existing page, which other CPUs might still read, doesn't go
away
+      * from under us, by calling gfn_to_pfn_prot(write_fault=true).
+      * Therefore, we call gfn_to_pfn_prot(write_fault=false), which
will
+      * pin the existing page, then we get a new page for the user

space

quoted

quoted

+      * pte and map this in the stage-2 table where we also make sure

to

quoted

quoted

+      * flush the TLB for the VM, if there was an existing entry
+ (the
entry
+      * was updated setting the write flag to the potentially new

page).

quoted

quoted

quoted

+      */
+     if (fault_status == FSC_PERM) {
+             pfn_existing = gfn_to_pfn_prot(vcpu->kvm, gfn, false,

NULL);

quoted

quoted

quoted

+             if (is_error_pfn(pfn_existing))
+                     return -EFAULT;
+     }
+
+     pfn = gfn_to_pfn_prot(vcpu->kvm, gfn, write_fault, &writable);
+     if (is_error_pfn(pfn)) {
+             ret = -EFAULT;
+             goto out_put_existing;
+     }
+
+     /* We need minimum second+third level pages */
+     ret = mmu_topup_memory_cache(memcache, 2, KVM_NR_MEM_OBJS);
+     if (ret)
+             goto out;
+     new_pte = pfn_pte(pfn, PAGE_KVM_GUEST);
+     if (writable)
+             pte_val(new_pte) |= L_PTE2_WRITE;
+     coherent_icache_guest_page(vcpu->kvm, gfn);

why don't you flush icache only when guest has mapped executable page
as __sync_icache_dcache function does currently?

because we don't know if the guest will map the page executable. The guest
may read the page through a normal load, which causes the fault, and
subsequently execute it (even possible through different guest mappings).
The only way to see this happening would be to mark all pages as non-
executable and catch the fault when it occurs - unfortunately the HPFAR
which gives us the IPA is not populated on execute never faults, so we
would have to translate the PC's va to ipa using cp15 functionality when
this happens, which is then also racy with other CPUs. So the question is
really if this will even be an optimization, but it's definitely something
that requires further investigation.

OK. I understand your point.

But if guest maps a page for execution, guest will flush Icache
from __sync_icache_dcache. Then coherent_icache_guest_page doesn't seem to
be
necessary again. One thing I'm not sure in this case is when guest maps
for kernel executable page(module loading) and it reuses the kernel
executable page
from host(module unloading). But in that case, I think it is possible to
reduce 
the number of flush by limiting the address range for flush.

I think you're missing the major point:
When the guest maps a page for execution, it knows it has to synchronize
Icache and Dcache. But the guest never knows when we swap out a page
because of memory pressure.

When the guest eventually faults that page back in, chances are it will
be a different physical page, and the cache content may be inconsistent.
We must then sync Icache/Dcache for this page.

Now, as Christoffer mentioned, there's a number of schemes we could
potentially use to mitigate this effect (using the XN bit in the Stage2
page tables), but it remains to be seen how effective it will be.

	M.
-- 
Jazz is not dead. It just smells funny...