On Wed, Sep 26, 2012 at 11:11 PM, Min-gyu Kim [off-list ref] 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.

the guest kernel will flush the dcache when it maps the page
initially. However, when we swap on the host we might use that same
page at the same virtual address as the original guest in another
guest or on the host, and the icache will now contain incorrect code
that can be executed from the guest in case of vipt caches. In the
case of pipt caches, if the page is used for instructions on any
virtual address, incorrect entries can be executed from the icache
once this page is used for guest instructions again. If you have
suggestions on how to optimize this, it would be great, but I see not
good way around it.