[PATCH 0/8] KVM: PPC: E500: Implement MMU Notifiers

[PATCH 0/8] KVM: PPC: E500: Implement MMU Notifiers

[Alexander Graf]

This patch set adds a very simple implementation of MMU notifiers for the
e500 target. Along the way, I stumbled over a few other things that I've
put into the same patch set, namely:

  * e500 prerequisites
  * icache flushing on page map (probably also hits ARM!)
  * exit trace point for e500

Alexander Graf (8):
  KVM: PPC: BookE: Expose remote TLB flushes in debugfs
  KVM: PPC: E500: Fix clear_tlb_refs
  KVM: PPC: PR: Use generic tracepoint for guest exit
  KVM: PPC: Expose SYNC cap based on mmu notifiers
  KVM: Add hva_to_memslot
  KVM: PPC: E500: Implement MMU notifiers
  KVM: Add page map arch callback
  KVM: PPC: Add cache flush on page map

 arch/powerpc/include/asm/kvm_host.h |   13 +++++-
 arch/powerpc/include/asm/kvm_ppc.h  |    1 +
 arch/powerpc/kvm/Kconfig            |    2 +
 arch/powerpc/kvm/book3s_pr.c        |    2 +-
 arch/powerpc/kvm/booke.c            |   27 ++++++++++++
 arch/powerpc/kvm/e500_tlb.c         |   60 +++++++++++++++++++++++++-
 arch/powerpc/kvm/powerpc.c          |    8 +++-
 arch/powerpc/kvm/trace.h            |   79 ++++++++++++++++++++++------------
 arch/powerpc/mm/mem.c               |    1 +
 include/linux/kvm_host.h            |    1 +
 virt/kvm/kvm_main.c                 |   20 ++++++++-
 11 files changed, 180 insertions(+), 34 deletions(-)

[PATCH 1/8] KVM: PPC: BookE: Expose remote TLB flushes in debugfs

[Alexander Graf]

We're already counting remote TLB flushes in a variable, but don't export
it to user space yet. Do so, so we know what's going on.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/kvm/booke.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index d25a097..91b9662 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -62,6 +62,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 	{ "halt_wakeup", VCPU_STAT(halt_wakeup) },
 	{ "doorbell", VCPU_STAT(dbell_exits) },
 	{ "guest doorbell", VCPU_STAT(gdbell_exits) },
+	{ "remote_tlb_flush", VM_STAT(remote_tlb_flush) },
 	{ NULL }
 };
 
-- 
1.6.0.2

[PATCH 2/8] KVM: PPC: E500: Fix clear_tlb_refs

[Alexander Graf]

Our mapping code assumes that TLB0 entries are always mapped. However, after
calling clear_tlb_refs() this is no longer the case.

Map them dynamically if we find an entry unmapped in TLB0.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/kvm/e500_tlb.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kvm/e500_tlb.c b/arch/powerpc/kvm/e500_tlb.c
index c8f6c58..d26e705 100644
--- a/arch/powerpc/kvm/e500_tlb.c
+++ b/arch/powerpc/kvm/e500_tlb.c
@@ -1037,8 +1037,12 @@ void kvmppc_mmu_map(struct kvm_vcpu *vcpu, u64 eaddr, gpa_t gpaddr,
 		sesel = 0; /* unused */
 		priv = &vcpu_e500->gtlb_priv[tlbsel][esel];
 
-		kvmppc_e500_setup_stlbe(vcpu, gtlbe, BOOK3E_PAGESZ_4K,
-					&priv->ref, eaddr, &stlbe);
+		/* Only triggers after clear_tlb_refs */
+		if (unlikely(!(priv->ref.flags & E500_TLB_VALID)))
+			kvmppc_e500_tlb0_map(vcpu_e500, esel, &stlbe);
+		else
+			kvmppc_e500_setup_stlbe(vcpu, gtlbe, BOOK3E_PAGESZ_4K,
+						&priv->ref, eaddr, &stlbe);
 		break;
 
 	case 1: {
-- 
1.6.0.2

[PATCH 3/8] KVM: PPC: PR: Use generic tracepoint for guest exit

[Alexander Graf]

We want to have tracing information on guest exits for booke as well
as book3s. Since most information is identical, use a common trace point.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/kvm/book3s_pr.c |    2 +-
 arch/powerpc/kvm/booke.c     |    3 ++
 arch/powerpc/kvm/trace.h     |   79 +++++++++++++++++++++++++++---------------
 3 files changed, 55 insertions(+), 29 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index a1baec3..0fe4cd4 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -551,7 +551,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 	/* We get here with MSR.EE=0, so enable it to be a nice citizen */
 	__hard_irq_enable();
 
-	trace_kvm_book3s_exit(exit_nr, vcpu);
+	trace_kvm_exit(exit_nr, vcpu);
 	preempt_enable();
 	kvm_resched(vcpu);
 	switch (exit_nr) {
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 91b9662..1d4ce9a 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -39,6 +39,7 @@
 
 #include "timing.h"
 #include "booke.h"
+#include "trace.h"
 
 unsigned long kvmppc_booke_handlers;
 
@@ -678,6 +679,8 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 
 	local_irq_enable();
 
+	trace_kvm_exit(exit_nr, vcpu);
+
 	run->exit_reason = KVM_EXIT_UNKNOWN;
 	run->ready_for_interrupt_injection = 1;
 
diff --git a/arch/powerpc/kvm/trace.h b/arch/powerpc/kvm/trace.h
index 877186b..9fab6ed 100644
--- a/arch/powerpc/kvm/trace.h
+++ b/arch/powerpc/kvm/trace.h
@@ -31,6 +31,57 @@ TRACE_EVENT(kvm_ppc_instr,
 		  __entry->inst, __entry->pc, __entry->emulate)
 );
 
+TRACE_EVENT(kvm_exit,
+	TP_PROTO(unsigned int exit_nr, struct kvm_vcpu *vcpu),
+	TP_ARGS(exit_nr, vcpu),
+
+	TP_STRUCT__entry(
+		__field(	unsigned int,	exit_nr		)
+		__field(	unsigned long,	pc		)
+		__field(	unsigned long,	msr		)
+		__field(	unsigned long,	dar		)
+#ifdef CONFIG_KVM_BOOK3S_PR
+		__field(	unsigned long,	srr1		)
+#endif
+		__field(	unsigned long,	last_inst	)
+	),
+
+	TP_fast_assign(
+#ifdef CONFIG_KVM_BOOK3S_PR
+		struct kvmppc_book3s_shadow_vcpu *svcpu;
+#endif
+		__entry->exit_nr	= exit_nr;
+		__entry->pc		= kvmppc_get_pc(vcpu);
+		__entry->dar		= kvmppc_get_fault_dar(vcpu);
+		__entry->msr		= vcpu->arch.shared->msr;
+#ifdef CONFIG_KVM_BOOK3S_PR
+		svcpu = svcpu_get(vcpu);
+		__entry->srr1		= svcpu->shadow_srr1;
+		svcpu_put(svcpu);
+#endif
+		__entry->last_inst	= vcpu->arch.last_inst;
+	),
+
+	TP_printk("exit=0x%x"
+		" | pc=0x%lx"
+		" | msr=0x%lx"
+		" | dar=0x%lx"
+#ifdef CONFIG_KVM_BOOK3S_PR
+		" | srr1=0x%lx"
+#endif
+		" | last_inst=0x%lx"
+		,
+		__entry->exit_nr,
+		__entry->pc,
+		__entry->msr,
+		__entry->dar,
+#ifdef CONFIG_KVM_BOOK3S_PR
+		__entry->srr1,
+#endif
+		__entry->last_inst
+		)
+);
+
 TRACE_EVENT(kvm_stlb_inval,
 	TP_PROTO(unsigned int stlb_index),
 	TP_ARGS(stlb_index),
@@ -105,34 +156,6 @@ TRACE_EVENT(kvm_gtlb_write,
 
 #ifdef CONFIG_KVM_BOOK3S_PR
 
-TRACE_EVENT(kvm_book3s_exit,
-	TP_PROTO(unsigned int exit_nr, struct kvm_vcpu *vcpu),
-	TP_ARGS(exit_nr, vcpu),
-
-	TP_STRUCT__entry(
-		__field(	unsigned int,	exit_nr		)
-		__field(	unsigned long,	pc		)
-		__field(	unsigned long,	msr		)
-		__field(	unsigned long,	dar		)
-		__field(	unsigned long,	srr1		)
-	),
-
-	TP_fast_assign(
-		struct kvmppc_book3s_shadow_vcpu *svcpu;
-		__entry->exit_nr	= exit_nr;
-		__entry->pc		= kvmppc_get_pc(vcpu);
-		__entry->dar		= kvmppc_get_fault_dar(vcpu);
-		__entry->msr		= vcpu->arch.shared->msr;
-		svcpu = svcpu_get(vcpu);
-		__entry->srr1		= svcpu->shadow_srr1;
-		svcpu_put(svcpu);
-	),
-
-	TP_printk("exit=0x%x | pc=0x%lx | msr=0x%lx | dar=0x%lx | srr1=0x%lx",
-		  __entry->exit_nr, __entry->pc, __entry->msr, __entry->dar,
-		  __entry->srr1)
-);
-
 TRACE_EVENT(kvm_book3s_reenter,
 	TP_PROTO(int r, struct kvm_vcpu *vcpu),
 	TP_ARGS(r, vcpu),
-- 
1.6.0.2

[PATCH 4/8] KVM: PPC: Expose SYNC cap based on mmu notifiers

[Alexander Graf]

Semantically, the "SYNC" cap means that we have mmu notifiers available.
Express this in our #ifdef'ery around the feature, so that we can be sure
we don't miss out on ppc targets when they get their implementation.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/kvm/powerpc.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index a69ca4a..e2511de 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -264,10 +264,16 @@ int kvm_dev_ioctl_check_extension(long ext)
 		if (cpu_has_feature(CPU_FTR_ARCH_201))
 			r = 2;
 		break;
+#endif
 	case KVM_CAP_SYNC_MMU:
+#ifdef CONFIG_KVM_BOOK3S_64_HV
 		r = cpu_has_feature(CPU_FTR_ARCH_206) ? 1 : 0;
-		break;
+#elif defined(KVM_ARCH_WANT_MMU_NOTIFIER)
+		r = 1;
+#else
+		r = 0;
 #endif
+		break;
 	case KVM_CAP_NR_VCPUS:
 		/*
 		 * Recommending a number of CPUs is somewhat arbitrary; we
-- 
1.6.0.2

[PATCH 5/8] KVM: Add hva_to_memslot

[Alexander Graf]

Architecture code might want to figure out if an hva that it gets for example
via the mmu notifier callbacks actually is in guest mapped memory, and if so,
in which memory slot.

This patch introduces a helper function to enable it to do so. It is a
prerequisite for the e500 mmu notifier implementation.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 include/linux/kvm_host.h |    1 +
 virt/kvm/kvm_main.c      |   14 ++++++++++++++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index dbc65f9..2b92928 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -464,6 +464,7 @@ int kvm_gfn_to_hva_cache_init(struct kvm *kvm, struct gfn_to_hva_cache *ghc,
 int kvm_clear_guest_page(struct kvm *kvm, gfn_t gfn, int offset, int len);
 int kvm_clear_guest(struct kvm *kvm, gpa_t gpa, unsigned long len);
 struct kvm_memory_slot *gfn_to_memslot(struct kvm *kvm, gfn_t gfn);
+struct kvm_memory_slot *hva_to_memslot(struct kvm *kvm, hva_t hva);
 int kvm_is_visible_gfn(struct kvm *kvm, gfn_t gfn);
 unsigned long kvm_host_page_size(struct kvm *kvm, gfn_t gfn);
 void mark_page_dirty(struct kvm *kvm, gfn_t gfn);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index bcf973e..d42591d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -999,6 +999,20 @@ struct kvm_memory_slot *gfn_to_memslot(struct kvm *kvm, gfn_t gfn)
 }
 EXPORT_SYMBOL_GPL(gfn_to_memslot);
 
+struct kvm_memory_slot *hva_to_memslot(struct kvm *kvm, hva_t hva)
+{
+	struct kvm_memslots *slots = kvm_memslots(kvm);
+	struct kvm_memory_slot *memslot;
+
+	kvm_for_each_memslot(memslot, slots)
+		if (hva >= memslot->userspace_addr &&
+		      hva < memslot->userspace_addr + memslot->npages)
+			return memslot;
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(hva_to_memslot);
+
 int kvm_is_visible_gfn(struct kvm *kvm, gfn_t gfn)
 {
 	struct kvm_memory_slot *memslot = gfn_to_memslot(kvm, gfn);
-- 
1.6.0.2

Re: [kvmarm] [PATCH 5/8] KVM: Add hva_to_memslot

[Christoffer Dall]

On Tue, Aug 7, 2012 at 6:57 AM, Alexander Graf <agraf@suse.de> wrote:
> Architecture code might want to figure out if an hva that it gets for example
> via the mmu notifier callbacks actually is in guest mapped memory, and if so,
> in which memory slot.
>
> This patch introduces a helper function to enable it to do so. It is a
> prerequisite for the e500 mmu notifier implementation.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  include/linux/kvm_host.h |    1 +
>  virt/kvm/kvm_main.c      |   14 ++++++++++++++
>  2 files changed, 15 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index dbc65f9..2b92928 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -464,6 +464,7 @@ int kvm_gfn_to_hva_cache_init(struct kvm *kvm, struct gfn_to_hva_cache *ghc,
>  int kvm_clear_guest_page(struct kvm *kvm, gfn_t gfn, int offset, int len);
>  int kvm_clear_guest(struct kvm *kvm, gpa_t gpa, unsigned long len);
>  struct kvm_memory_slot *gfn_to_memslot(struct kvm *kvm, gfn_t gfn);
> +struct kvm_memory_slot *hva_to_memslot(struct kvm *kvm, hva_t hva);
>  int kvm_is_visible_gfn(struct kvm *kvm, gfn_t gfn);
>  unsigned long kvm_host_page_size(struct kvm *kvm, gfn_t gfn);
>  void mark_page_dirty(struct kvm *kvm, gfn_t gfn);
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index bcf973e..d42591d 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -999,6 +999,20 @@ struct kvm_memory_slot *gfn_to_memslot(struct kvm *kvm, gfn_t gfn)
>  }
>  EXPORT_SYMBOL_GPL(gfn_to_memslot);
>
> +struct kvm_memory_slot *hva_to_memslot(struct kvm *kvm, hva_t hva)
> +{
> +       struct kvm_memslots *slots = kvm_memslots(kvm);
> +       struct kvm_memory_slot *memslot;
> +
> +       kvm_for_each_memslot(memslot, slots)
> +               if (hva >= memslot->userspace_addr &&
> +                     hva < memslot->userspace_addr + memslot->npages)

addr + npages, this doesn't look right

> +                       return memslot;
> +
> +       return NULL;
> +}
> +EXPORT_SYMBOL_GPL(hva_to_memslot);

consider also adding a hva_to_gpa wrapper now when you're add it, then
ARM code will be so happy:

bool hva_to_gpa(struct kvm *kvm, unsigned long hva, gpa_t *gpa)
{
	struct kvm_memory_slot *memslot;

	memslot = hva_to_memslot(kvm, hva);
	if (!memslot)
		return false;

	gpa_t gpa_offset = hva - memslot->userspace_addr;
	*gpa = (memslot->base_gfn << PAGE_SHIFT) + gpa_offset;
	return true;
}

> +
>  int kvm_is_visible_gfn(struct kvm *kvm, gfn_t gfn)
>  {
>         struct kvm_memory_slot *memslot = gfn_to_memslot(kvm, gfn);
> --
> 1.6.0.2
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

Re: [kvmarm] [PATCH 5/8] KVM: Add hva_to_memslot

[Alexander Graf]

On 08.08.2012, at 06:55, Christoffer Dall wrote:

> On Tue, Aug 7, 2012 at 6:57 AM, Alexander Graf <agraf@suse.de> wrote:
>> Architecture code might want to figure out if an hva that it gets for example
>> via the mmu notifier callbacks actually is in guest mapped memory, and if so,
>> in which memory slot.
>> 
>> This patch introduces a helper function to enable it to do so. It is a
>> prerequisite for the e500 mmu notifier implementation.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> include/linux/kvm_host.h |    1 +
>> virt/kvm/kvm_main.c      |   14 ++++++++++++++
>> 2 files changed, 15 insertions(+), 0 deletions(-)
>> 
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index dbc65f9..2b92928 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -464,6 +464,7 @@ int kvm_gfn_to_hva_cache_init(struct kvm *kvm, struct gfn_to_hva_cache *ghc,
>> int kvm_clear_guest_page(struct kvm *kvm, gfn_t gfn, int offset, int len);
>> int kvm_clear_guest(struct kvm *kvm, gpa_t gpa, unsigned long len);
>> struct kvm_memory_slot *gfn_to_memslot(struct kvm *kvm, gfn_t gfn);
>> +struct kvm_memory_slot *hva_to_memslot(struct kvm *kvm, hva_t hva);
>> int kvm_is_visible_gfn(struct kvm *kvm, gfn_t gfn);
>> unsigned long kvm_host_page_size(struct kvm *kvm, gfn_t gfn);
>> void mark_page_dirty(struct kvm *kvm, gfn_t gfn);
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index bcf973e..d42591d 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -999,6 +999,20 @@ struct kvm_memory_slot *gfn_to_memslot(struct kvm *kvm, gfn_t gfn)
>> }
>> EXPORT_SYMBOL_GPL(gfn_to_memslot);
>> 
>> +struct kvm_memory_slot *hva_to_memslot(struct kvm *kvm, hva_t hva)
>> +{
>> +       struct kvm_memslots *slots = kvm_memslots(kvm);
>> +       struct kvm_memory_slot *memslot;
>> +
>> +       kvm_for_each_memslot(memslot, slots)
>> +               if (hva >= memslot->userspace_addr &&
>> +                     hva < memslot->userspace_addr + memslot->npages)
> 
> addr + npages, this doesn't look right

Thanks a lot for spotting that one!

> 
>> +                       return memslot;
>> +
>> +       return NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(hva_to_memslot);
> 
> consider also adding a hva_to_gpa wrapper now when you're add it, then
> ARM code will be so happy:
> 
> bool hva_to_gpa(struct kvm *kvm, unsigned long hva, gpa_t *gpa)
> {
> 	struct kvm_memory_slot *memslot;
> 
> 	memslot = hva_to_memslot(kvm, hva);
> 	if (!memslot)
> 		return false;
> 
> 	gpa_t gpa_offset = hva - memslot->userspace_addr;
> 	*gpa = (memslot->base_gfn << PAGE_SHIFT) + gpa_offset;
> 	return true;
> }

What do you need this for? I usually don't like to add framework code that has no users (yet).


Alex

Re: [PATCH 5/8] KVM: Add hva_to_memslot

[Takuya Yoshikawa]

On Tue,  7 Aug 2012 12:57:13 +0200
Alexander Graf <agraf@suse.de> wrote:

> +struct kvm_memory_slot *hva_to_memslot(struct kvm *kvm, hva_t hva)
> +{
> +	struct kvm_memslots *slots = kvm_memslots(kvm);
> +	struct kvm_memory_slot *memslot;
> +
> +	kvm_for_each_memslot(memslot, slots)
> +		if (hva >= memslot->userspace_addr &&
> +		      hva < memslot->userspace_addr + memslot->npages)
> +			return memslot;
> +
> +	return NULL;
> +}

Can't we have two memory slots which contain that hva?
I thought that's why hva handler had to check all slots.

Thanks,
	Takuya

Re: [PATCH 5/8] KVM: Add hva_to_memslot

[Avi Kivity]

On 08/09/2012 01:34 PM, Takuya Yoshikawa wrote:
> On Tue,  7 Aug 2012 12:57:13 +0200
> Alexander Graf <agraf@suse.de> wrote:
> 
>> +struct kvm_memory_slot *hva_to_memslot(struct kvm *kvm, hva_t hva)
>> +{
>> +	struct kvm_memslots *slots = kvm_memslots(kvm);
>> +	struct kvm_memory_slot *memslot;
>> +
>> +	kvm_for_each_memslot(memslot, slots)
>> +		if (hva >= memslot->userspace_addr &&
>> +		      hva < memslot->userspace_addr + memslot->npages)
>> +			return memslot;
>> +
>> +	return NULL;
>> +}
> 
> Can't we have two memory slots which contain that hva?
> I thought that's why hva handler had to check all slots.

We can and do.  Good catch.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 5/8] KVM: Add hva_to_memslot

[Alexander Graf]

On 09.08.2012, at 12:36, Avi Kivity <avi@redhat.com> wrote:

> On 08/09/2012 01:34 PM, Takuya Yoshikawa wrote:
>> On Tue,  7 Aug 2012 12:57:13 +0200
>> Alexander Graf <agraf@suse.de> wrote:
>> 
>>> +struct kvm_memory_slot *hva_to_memslot(struct kvm *kvm, hva_t hva)
>>> +{
>>> +    struct kvm_memslots *slots = kvm_memslots(kvm);
>>> +    struct kvm_memory_slot *memslot;
>>> +
>>> +    kvm_for_each_memslot(memslot, slots)
>>> +        if (hva >= memslot->userspace_addr &&
>>> +              hva < memslot->userspace_addr + memslot->npages)
>>> +            return memslot;
>>> +
>>> +    return NULL;
>>> +}
>> 
>> Can't we have two memory slots which contain that hva?
>> I thought that's why hva handler had to check all slots.
> 
> We can and do.  Good catch.
> 

Hrm. So I guess we can only do an hva_is_guest_memory() helper? That's all I really need anyways :)

Alex


> 
> -- 
> error compiling committee.c: too many arguments to function
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/8] KVM: Add hva_to_memslot

[Avi Kivity]

On 08/09/2012 08:02 PM, Alexander Graf wrote:
> 
> 
> On 09.08.2012, at 12:36, Avi Kivity <avi@redhat.com> wrote:
> 
>> On 08/09/2012 01:34 PM, Takuya Yoshikawa wrote:
>>> On Tue,  7 Aug 2012 12:57:13 +0200
>>> Alexander Graf <agraf@suse.de> wrote:
>>> 
>>>> +struct kvm_memory_slot *hva_to_memslot(struct kvm *kvm, hva_t hva)
>>>> +{
>>>> +    struct kvm_memslots *slots = kvm_memslots(kvm);
>>>> +    struct kvm_memory_slot *memslot;
>>>> +
>>>> +    kvm_for_each_memslot(memslot, slots)
>>>> +        if (hva >= memslot->userspace_addr &&
>>>> +              hva < memslot->userspace_addr + memslot->npages)
>>>> +            return memslot;
>>>> +
>>>> +    return NULL;
>>>> +}
>>> 
>>> Can't we have two memory slots which contain that hva?
>>> I thought that's why hva handler had to check all slots.
>> 
>> We can and do.  Good catch.
>> 
> 
> Hrm. So I guess we can only do an hva_is_guest_memory() helper? That's all I really need anyways :)
> 

How about kvm_for_each_memslot_hva_range()?  That can useful in
kvm_handle_hva_range().  For your use case, you just do you stuff and
return immediately.




-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 5/8] KVM: Add hva_to_memslot

[Alexander Graf]

On 12.08.2012, at 11:24, Avi Kivity wrote:

> On 08/09/2012 08:02 PM, Alexander Graf wrote:
>> 
>> 
>> On 09.08.2012, at 12:36, Avi Kivity <avi@redhat.com> wrote:
>> 
>>> On 08/09/2012 01:34 PM, Takuya Yoshikawa wrote:
>>>> On Tue,  7 Aug 2012 12:57:13 +0200
>>>> Alexander Graf <agraf@suse.de> wrote:
>>>> 
>>>>> +struct kvm_memory_slot *hva_to_memslot(struct kvm *kvm, hva_t hva)
>>>>> +{
>>>>> +    struct kvm_memslots *slots = kvm_memslots(kvm);
>>>>> +    struct kvm_memory_slot *memslot;
>>>>> +
>>>>> +    kvm_for_each_memslot(memslot, slots)
>>>>> +        if (hva >= memslot->userspace_addr &&
>>>>> +              hva < memslot->userspace_addr + memslot->npages)
>>>>> +            return memslot;
>>>>> +
>>>>> +    return NULL;
>>>>> +}
>>>> 
>>>> Can't we have two memory slots which contain that hva?
>>>> I thought that's why hva handler had to check all slots.
>>> 
>>> We can and do.  Good catch.
>>> 
>> 
>> Hrm. So I guess we can only do an hva_is_guest_memory() helper? That's all I really need anyways :)
>> 
> 
> How about kvm_for_each_memslot_hva_range()?  That can useful in
> kvm_handle_hva_range().  For your use case, you just do you stuff and
> return immediately.

Well, for now I just dropped the whole thing. In general, chances are pretty good that an HVA we get notified on with mmu notifiers is representing guest memory. And flushing a few times too often shouldn't hurt.


Alex

Re: [PATCH 5/8] KVM: Add hva_to_memslot

[Avi Kivity]

On 08/12/2012 02:03 PM, Alexander Graf wrote:
> 
> Well, for now I just dropped the whole thing. In general, chances are pretty good that an HVA we get notified on with mmu notifiers is representing guest memory. And flushing a few times too often shouldn't hurt.

That is not the case, actually.  We did this optimization for x86 for
this reason.



-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 5/8] KVM: Add hva_to_memslot

[Alexander Graf]

On 12.08.2012, at 13:21, Avi Kivity <avi@redhat.com> wrote:

> On 08/12/2012 02:03 PM, Alexander Graf wrote:
>> 
>> Well, for now I just dropped the whole thing. In general, chances are pretty good that an HVA we get notified on with mmu notifiers is representing guest memory. And flushing a few times too often shouldn't hurt.
> 
> That is not the case, actually.  We did this optimization for x86 for
> this reason.

Well, it's still what it is: an optimization. I'll start worrying about it again when I have a way to search for cache entries by hva :)

Alex

[PATCH 6/8] KVM: PPC: E500: Implement MMU notifiers

[Alexander Graf]

The e500 target has lived without mmu notifiers ever since it got
introduced, but fails for the user space check on them with hugetlbfs.

So in order to get that one working, implement mmu notifiers in a
reasonably dumb fashion and be happy. On embedded hardware, we almost
never end up with mmu notifier calls, since most people don't overcommit.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/include/asm/kvm_host.h |    3 +-
 arch/powerpc/include/asm/kvm_ppc.h  |    1 +
 arch/powerpc/kvm/Kconfig            |    2 +
 arch/powerpc/kvm/booke.c            |   23 +++++++++++++++
 arch/powerpc/kvm/e500_tlb.c         |   52 +++++++++++++++++++++++++++++++++++
 5 files changed, 80 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index 572ad01..ed75bc9 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -45,7 +45,8 @@
 #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
 #endif
 
-#ifdef CONFIG_KVM_BOOK3S_64_HV
+#if defined(CONFIG_KVM_BOOK3S_64_HV) || defined(CONFIG_KVM_E500V2) || \
+    defined(CONFIG_KVM_E500MC)
 #include <linux/mmu_notifier.h>
 
 #define KVM_ARCH_WANT_MMU_NOTIFIER
diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
index 0124937..c38e824 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -104,6 +104,7 @@ extern void kvmppc_core_queue_external(struct kvm_vcpu *vcpu,
                                        struct kvm_interrupt *irq);
 extern void kvmppc_core_dequeue_external(struct kvm_vcpu *vcpu,
                                          struct kvm_interrupt *irq);
+extern void kvmppc_core_flush_tlb(struct kvm_vcpu *vcpu);
 
 extern int kvmppc_core_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
                                   unsigned int op, int *advance);
diff --git a/arch/powerpc/kvm/Kconfig b/arch/powerpc/kvm/Kconfig
index f4dacb9..40cad8c 100644
--- a/arch/powerpc/kvm/Kconfig
+++ b/arch/powerpc/kvm/Kconfig
@@ -123,6 +123,7 @@ config KVM_E500V2
 	depends on EXPERIMENTAL && E500 && !PPC_E500MC
 	select KVM
 	select KVM_MMIO
+	select MMU_NOTIFIER
 	---help---
 	  Support running unmodified E500 guest kernels in virtual machines on
 	  E500v2 host processors.
@@ -138,6 +139,7 @@ config KVM_E500MC
 	select KVM
 	select KVM_MMIO
 	select KVM_BOOKE_HV
+	select MMU_NOTIFIER
 	---help---
 	  Support running unmodified E500MC/E5500 (32-bit) guest kernels in
 	  virtual machines on E500MC/E5500 host processors.
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 1d4ce9a..e794c3c 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -461,6 +461,15 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
 	return r;
 }
 
+static void kvmppc_check_requests(struct kvm_vcpu *vcpu)
+{
+#if defined(CONFIG_KVM_E500V2) || defined(CONFIG_KVM_E500MC)
+	if (vcpu->requests)
+		if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu))
+			kvmppc_core_flush_tlb(vcpu);
+#endif
+}
+
 /*
  * Common checks before entering the guest world.  Call with interrupts
  * disabled.
@@ -485,12 +494,24 @@ static int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
 			break;
 		}
 
+		smp_mb();
+		kvmppc_check_requests(vcpu);
+
 		if (kvmppc_core_prepare_to_enter(vcpu)) {
 			/* interrupts got enabled in between, so we
 			   are back at square 1 */
 			continue;
 		}
 
+		if (vcpu->mode == EXITING_GUEST_MODE) {
+			r = 1;
+			break;
+		}
+
+		/* Going into guest context! Yay! */
+		vcpu->mode = IN_GUEST_MODE;
+		smp_wmb();
+
 		break;
 	}
 
@@ -560,6 +581,8 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 #endif
 
 	kvm_guest_exit();
+	vcpu->mode = OUTSIDE_GUEST_MODE;
+	smp_wmb();
 
 out:
 	local_irq_enable();
diff --git a/arch/powerpc/kvm/e500_tlb.c b/arch/powerpc/kvm/e500_tlb.c
index d26e705..3f78756 100644
--- a/arch/powerpc/kvm/e500_tlb.c
+++ b/arch/powerpc/kvm/e500_tlb.c
@@ -357,6 +357,13 @@ static void clear_tlb_refs(struct kvmppc_vcpu_e500 *vcpu_e500)
 	clear_tlb_privs(vcpu_e500);
 }
 
+void kvmppc_core_flush_tlb(struct kvm_vcpu *vcpu)
+{
+	struct kvmppc_vcpu_e500 *vcpu_e500 = to_e500(vcpu);
+	clear_tlb_refs(vcpu_e500);
+	clear_tlb1_bitmap(vcpu_e500);
+}
+
 static inline void kvmppc_e500_deliver_tlb_miss(struct kvm_vcpu *vcpu,
 		unsigned int eaddr, int as)
 {
@@ -1062,6 +1069,51 @@ void kvmppc_mmu_map(struct kvm_vcpu *vcpu, u64 eaddr, gpa_t gpaddr,
 	write_stlbe(vcpu_e500, gtlbe, &stlbe, stlbsel, sesel);
 }
 
+/************* MMU Notifiers *************/
+
+int kvm_unmap_hva(struct kvm *kvm, unsigned long hva)
+{
+	/* Is this a guest page? */
+	if (!hva_to_memslot(kvm, hva))
+		return 0;
+
+	/*
+	 * Flush all shadow tlb entries everywhere. This is slow, but
+	 * we are 100% sure that we catch the to be unmapped page
+	 */
+	kvm_flush_remote_tlbs(kvm);
+
+	return 0;
+}
+
+int kvm_unmap_hva_range(struct kvm *kvm, unsigned long start, unsigned long end)
+{
+	/* kvm_unmap_hva flushes everything anyways */
+	kvm_unmap_hva(kvm, start);
+
+	return 0;
+}
+
+int kvm_age_hva(struct kvm *kvm, unsigned long hva)
+{
+	/* XXX could be more clever ;) */
+	return 0;
+}
+
+int kvm_test_age_hva(struct kvm *kvm, unsigned long hva)
+{
+	/* XXX could be more clever ;) */
+	return 0;
+}
+
+void kvm_set_spte_hva(struct kvm *kvm, unsigned long hva, pte_t pte)
+{
+	/* The page will get remapped properly on its next fault */
+	kvm_unmap_hva(kvm, hva);
+}
+
+/*****************************************/
+
 static void free_gtlb(struct kvmppc_vcpu_e500 *vcpu_e500)
 {
 	int i;
-- 
1.6.0.2

Re: [PATCH 6/8] KVM: PPC: E500: Implement MMU notifiers

[Avi Kivity]

On 08/07/2012 01:57 PM, Alexander Graf wrote:
> The e500 target has lived without mmu notifiers ever since it got
> introduced, but fails for the user space check on them with hugetlbfs.
> 
> So in order to get that one working, implement mmu notifiers in a
> reasonably dumb fashion and be happy. On embedded hardware, we almost
> never end up with mmu notifier calls, since most people don't overcommit.
> 
>  
> +static void kvmppc_check_requests(struct kvm_vcpu *vcpu)
> +{
> +#if defined(CONFIG_KVM_E500V2) || defined(CONFIG_KVM_E500MC)
> +	if (vcpu->requests)
> +		if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu))
> +			kvmppc_core_flush_tlb(vcpu);
> +#endif
> +}
> +
>  /*
>   * Common checks before entering the guest world.  Call with interrupts
>   * disabled.
> @@ -485,12 +494,24 @@ static int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>  			break;
>  		}
>  
> +		smp_mb();
> +		kvmppc_check_requests(vcpu);
> +

On x86 we do the requests processing while in normal preemptible
context, then do an additional check for requests != 0 during guest
entry.  This allows us to do sleepy things in request processing, and
reduces the amount of work we do with interrupts disabled.

>  		if (kvmppc_core_prepare_to_enter(vcpu)) {
>  			/* interrupts got enabled in between, so we
>  			   are back at square 1 */
>  			continue;
>  		}
>  
> +		if (vcpu->mode == EXITING_GUEST_MODE) {
> +			r = 1;
> +			break;
> +		}
> +
> +		/* Going into guest context! Yay! */
> +		vcpu->mode = IN_GUEST_MODE;
> +		smp_wmb();
> +
>  		break;
>  	}
>  
> @@ -560,6 +581,8 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
>  #endif
>  
>  	kvm_guest_exit();
> +	vcpu->mode = OUTSIDE_GUEST_MODE;
> +	smp_wmb();
>  
> +/************* MMU Notifiers *************/
> +
> +int kvm_unmap_hva(struct kvm *kvm, unsigned long hva)
> +{
> +	/* Is this a guest page? */
> +	if (!hva_to_memslot(kvm, hva))
> +		return 0;
> +
> +	/*
> +	 * Flush all shadow tlb entries everywhere. This is slow, but
> +	 * we are 100% sure that we catch the to be unmapped page
> +	 */
> +	kvm_flush_remote_tlbs(kvm);

Wow.

> +
> +	return 0;
> +}
> +

Where do you drop the reference count when installing a page in a shadow
tlb entry?


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 6/8] KVM: PPC: E500: Implement MMU notifiers

[Alexander Graf]

On 07.08.2012, at 15:30, Avi Kivity <avi@redhat.com> wrote:

> On 08/07/2012 01:57 PM, Alexander Graf wrote:
>> The e500 target has lived without mmu notifiers ever since it got
>> introduced, but fails for the user space check on them with hugetlbfs.
>> 
>> So in order to get that one working, implement mmu notifiers in a
>> reasonably dumb fashion and be happy. On embedded hardware, we almost
>> never end up with mmu notifier calls, since most people don't overcommit.
>> 
>> 
>> +static void kvmppc_check_requests(struct kvm_vcpu *vcpu)
>> +{
>> +#if defined(CONFIG_KVM_E500V2) || defined(CONFIG_KVM_E500MC)
>> +    if (vcpu->requests)
>> +        if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu))
>> +            kvmppc_core_flush_tlb(vcpu);
>> +#endif
>> +}
>> +
>> /*
>>  * Common checks before entering the guest world.  Call with interrupts
>>  * disabled.
>> @@ -485,12 +494,24 @@ static int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>>            break;
>>        }
>> 
>> +        smp_mb();
>> +        kvmppc_check_requests(vcpu);
>> +
> 
> On x86 we do the requests processing while in normal preemptible
> context, then do an additional check for requests != 0 during guest
> entry.  This allows us to do sleepy things in request processing, and
> reduces the amount of work we do with interrupts disabled.

Hrm. We could do the same I guess. Let me give it a try.

> 
>>        if (kvmppc_core_prepare_to_enter(vcpu)) {
>>            /* interrupts got enabled in between, so we
>>               are back at square 1 */
>>            continue;
>>        }
>> 
>> +        if (vcpu->mode == EXITING_GUEST_MODE) {
>> +            r = 1;
>> +            break;
>> +        }
>> +
>> +        /* Going into guest context! Yay! */
>> +        vcpu->mode = IN_GUEST_MODE;
>> +        smp_wmb();
>> +
>>        break;
>>    }
>> 
>> @@ -560,6 +581,8 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
>> #endif
>> 
>>    kvm_guest_exit();
>> +    vcpu->mode = OUTSIDE_GUEST_MODE;
>> +    smp_wmb();
>> 
>> +/************* MMU Notifiers *************/
>> +
>> +int kvm_unmap_hva(struct kvm *kvm, unsigned long hva)
>> +{
>> +    /* Is this a guest page? */
>> +    if (!hva_to_memslot(kvm, hva))
>> +        return 0;
>> +
>> +    /*
>> +     * Flush all shadow tlb entries everywhere. This is slow, but
>> +     * we are 100% sure that we catch the to be unmapped page
>> +     */
>> +    kvm_flush_remote_tlbs(kvm);
> 
> Wow.

Yeah, cool, eh? It sounds worse than it is. Usually when we need to page out, we're under memory pressure. So we would get called multiple times to unmap different pages. If we just drop all shadow tlb entries, we also freed a lot of memory that can now be paged out without callbacks.

> 
>> +
>> +    return 0;
>> +}
>> +
> 
> Where do you drop the reference count when installing a page in a shadow
> tlb entry?

Which reference count? Essentially the remote tlb flush calls kvmppc_e500_prov_release() on all currently mapped shadow tlb entries. Are we missing out on something more?


Alex

> 
> 
> -- 
> error compiling committee.c: too many arguments to function

Re: [PATCH 6/8] KVM: PPC: E500: Implement MMU notifiers

[Avi Kivity]

On 08/07/2012 04:52 PM, Alexander Graf wrote:
>>> 
>>> +/************* MMU Notifiers *************/
>>> +
>>> +int kvm_unmap_hva(struct kvm *kvm, unsigned long hva)
>>> +{
>>> +    /* Is this a guest page? */
>>> +    if (!hva_to_memslot(kvm, hva))
>>> +        return 0;
>>> +
>>> +    /*
>>> +     * Flush all shadow tlb entries everywhere. This is slow, but
>>> +     * we are 100% sure that we catch the to be unmapped page
>>> +     */
>>> +    kvm_flush_remote_tlbs(kvm);
>> 
>> Wow.
> 
> Yeah, cool, eh? It sounds worse than it is. Usually when we need to page out, we're under memory pressure. So we would get called multiple times to unmap different pages. If we just drop all shadow tlb entries, we also freed a lot of memory that can now be paged out without callbacks.

And it's just a shadow tlb yes?  So there's a limited amount of stuff
there.  But it'd be hell on x86.

> 
>> 
>>> +
>>> +    return 0;
>>> +}
>>> +
>> 
>> Where do you drop the reference count when installing a page in a shadow
>> tlb entry?
> 
> Which reference count? Essentially the remote tlb flush calls kvmppc_e500_prov_release() on all currently mapped shadow tlb entries. Are we missing out on something more?
> 

With mmu notifiers mapped pages are kept without elevated reference
counts; the mmu notifier protects them, not the refcount.  This allows
core mm code that looks at refcounts to work.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 6/8] KVM: PPC: E500: Implement MMU notifiers

[Alexander Graf]

On 07.08.2012, at 16:14, Avi Kivity <avi@redhat.com> wrote:

> On 08/07/2012 04:52 PM, Alexander Graf wrote:
>>>> 
>>>> +/************* MMU Notifiers *************/
>>>> +
>>>> +int kvm_unmap_hva(struct kvm *kvm, unsigned long hva)
>>>> +{
>>>> +    /* Is this a guest page? */
>>>> +    if (!hva_to_memslot(kvm, hva))
>>>> +        return 0;
>>>> +
>>>> +    /*
>>>> +     * Flush all shadow tlb entries everywhere. This is slow, but
>>>> +     * we are 100% sure that we catch the to be unmapped page
>>>> +     */
>>>> +    kvm_flush_remote_tlbs(kvm);
>>> 
>>> Wow.
>> 
>> Yeah, cool, eh? It sounds worse than it is. Usually when we need to page out, we're under memory pressure. So we would get called multiple times to unmap different pages. If we just drop all shadow tlb entries, we also freed a lot of memory that can now be paged out without callbacks.
> 
> And it's just a shadow tlb yes?  So there's a limited amount of stuff
> there.  But it'd be hell on x86.
> 
>> 
>>> 
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>> 
>>> Where do you drop the reference count when installing a page in a shadow
>>> tlb entry?
>> 
>> Which reference count? Essentially the remote tlb flush calls kvmppc_e500_prov_release() on all currently mapped shadow tlb entries. Are we missing out on something more?
>> 
> 
> With mmu notifiers mapped pages are kept without elevated reference
> counts; the mmu notifier protects them, not the refcount.  This allows
> core mm code that looks at refcounts to work.

Hrm. I wonder why it works then. We only drop the refcount after we got an mmu notifier callback. Maybe we get a callback on an unmapped page, but then happen to clear all the shadow entries as well, hence unpinning them along the way?

That explains why it works, but sure isn't exactly working as intended. Thanks for the hint!


Alex

Re: [PATCH 6/8] KVM: PPC: E500: Implement MMU notifiers

[Paul Mackerras]

On Tue, Aug 07, 2012 at 12:57:14PM +0200, Alexander Graf wrote:
> The e500 target has lived without mmu notifiers ever since it got
> introduced, but fails for the user space check on them with hugetlbfs.

Ironically that user space check isn't necessary any more since David
Gibson's fix for the hugetlbfs bug went in (90481622, "hugepages: fix
use after free bug in "quota" handling").  So on sufficiently recent
kernels you can just remove the userspace check.  Implementing
mmu-notifiers is a good thing for other reasons though.

Paul.

Re: [PATCH 6/8] KVM: PPC: E500: Implement MMU notifiers

[Alexander Graf]

On 08.08.2012, at 05:31, Paul Mackerras <paulus@samba.org> wrote:

> On Tue, Aug 07, 2012 at 12:57:14PM +0200, Alexander Graf wrote:
>> The e500 target has lived without mmu notifiers ever since it got
>> introduced, but fails for the user space check on them with hugetlbfs.
> 
> Ironically that user space check isn't necessary any more since David
> Gibson's fix for the hugetlbfs bug went in (90481622, "hugepages: fix
> use after free bug in "quota" handling").  So on sufficiently recent
> kernels you can just remove the userspace check.  Implementing
> mmu-notifiers is a good thing for other reasons though.

Yeah, it's probably best to just require mmu notifiers from every target. It would however be good to have a CAP nevertheless (or change the semantics of SYNC_MMU) so that we can run HV KVM on 970 without changes to QEMU.


Alex

[PATCH 7/8] KVM: Add page map arch callback

[Alexander Graf]

Some archs need to ensure that their icache is flushed when mapping a new
page. Add a callback to the generic code for an arch to implement any cache
flush logic it may need.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 virt/kvm/kvm_main.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index d42591d..4e0882d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1161,8 +1161,12 @@ static pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async,
 			pfn = get_fault_pfn();
 		}
 		up_read(&current->mm->mmap_sem);
-	} else
+	} else {
 		pfn = page_to_pfn(page[0]);
+#ifdef __KVM_HAVE_ARCH_MAP_PAGE
+		kvm_arch_map_page(page[0]);
+#endif
+	}
 
 	return pfn;
 }
-- 
1.6.0.2

Re: [PATCH 7/8] KVM: Add page map arch callback

[Avi Kivity]

On 08/07/2012 01:57 PM, Alexander Graf wrote:
> Some archs need to ensure that their icache is flushed when mapping a new
> page. Add a callback to the generic code for an arch to implement any cache
> flush logic it may need.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  virt/kvm/kvm_main.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index d42591d..4e0882d 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1161,8 +1161,12 @@ static pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async,
>  			pfn = get_fault_pfn();
>  		}
>  		up_read(&current->mm->mmap_sem);
> -	} else
> +	} else {
>  		pfn = page_to_pfn(page[0]);
> +#ifdef __KVM_HAVE_ARCH_MAP_PAGE
> +		kvm_arch_map_page(page[0]);
> +#endif
> +	}
>  

Please call it unconditionally, and have a stub inline ifndef
__KVM_HAVE_ARCH_MAP_PAGE.

Is this the correct place?  Who says the caller of hva_to_pfn() is going
to map it?


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 7/8] KVM: Add page map arch callback

[Alexander Graf]

On 07.08.2012, at 15:32, Avi Kivity <avi@redhat.com> wrote:

> On 08/07/2012 01:57 PM, Alexander Graf wrote:
>> Some archs need to ensure that their icache is flushed when mapping a new
>> page. Add a callback to the generic code for an arch to implement any cache
>> flush logic it may need.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> virt/kvm/kvm_main.c |    6 +++++-
>> 1 files changed, 5 insertions(+), 1 deletions(-)
>> 
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index d42591d..4e0882d 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -1161,8 +1161,12 @@ static pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async,
>>            pfn = get_fault_pfn();
>>        }
>>        up_read(&current->mm->mmap_sem);
>> -    } else
>> +    } else {
>>        pfn = page_to_pfn(page[0]);
>> +#ifdef __KVM_HAVE_ARCH_MAP_PAGE
>> +        kvm_arch_map_page(page[0]);
>> +#endif
>> +    }
>> 
> 
> Please call it unconditionally, and have a stub inline ifndef
> __KVM_HAVE_ARCH_MAP_PAGE.

Sure, works fine with me.

> 
> Is this the correct place?  Who says the caller of hva_to_pfn() is going
> to map it?

I don't think anyone is. However, we need the struct page, and all the generic kvm mm code tries hard to hide it from its users. The alternative would be to expose all those details, and I'm not sure that's a good idea.

Essentially, we don't care if we're overly cautious. Clearing one page too much is way better than clearing one too few.


Alex

Re: [PATCH 7/8] KVM: Add page map arch callback

[Avi Kivity]

On 08/07/2012 04:44 PM, Alexander Graf wrote:
> 
>> 
>> Is this the correct place?  Who says the caller of hva_to_pfn() is going
>> to map it?
> 
> I don't think anyone is. However, we need the struct page, and all the generic kvm mm code tries hard to hide it from its users. The alternative would be to expose all those details, and I'm not sure that's a good idea.
> 
> Essentially, we don't care if we're overly cautious. Clearing one page too much is way better than clearing one too few.

Are you sure everyone uses hva_to_pfn()?  x86 uses gfn_to_hva_many(), in
one place.



-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 7/8] KVM: Add page map arch callback

[Alexander Graf]

On 07.08.2012, at 15:58, Avi Kivity <avi@redhat.com> wrote:

> On 08/07/2012 04:44 PM, Alexander Graf wrote:
>> 
>>> 
>>> Is this the correct place?  Who says the caller of hva_to_pfn() is going
>>> to map it?
>> 
>> I don't think anyone is. However, we need the struct page, and all the generic kvm mm code tries hard to hide it from its users. The alternative would be to expose all those details, and I'm not sure that's a good idea.
>> 
>> Essentially, we don't care if we're overly cautious. Clearing one page too much is way better than clearing one too few.
> 
> Are you sure everyone uses hva_to_pfn()?  x86 uses gfn_to_hva_many(), in
> one place.

Nope, I only checked that e500 adheres to that flow so far. I'm not even 100% sure that book3s is always happy yet.

But I figured this is a step in the right direction. If we missed out on one, we can always add it later. The many function is a good spot. Maybe I'll just ckeck up all of kvm_main.c again for potential users.


Alex

Re: [PATCH 7/8] KVM: Add page map arch callback

[Avi Kivity]

On 08/07/2012 05:08 PM, Alexander Graf wrote:
> 
> 
> On 07.08.2012, at 15:58, Avi Kivity <avi@redhat.com> wrote:
> 
>> On 08/07/2012 04:44 PM, Alexander Graf wrote:
>>> 
>>>> 
>>>> Is this the correct place?  Who says the caller of hva_to_pfn() is going
>>>> to map it?
>>> 
>>> I don't think anyone is. However, we need the struct page, and all the generic kvm mm code tries hard to hide it from its users. The alternative would be to expose all those details, and I'm not sure that's a good idea.
>>> 
>>> Essentially, we don't care if we're overly cautious. Clearing one page too much is way better than clearing one too few.
>> 
>> Are you sure everyone uses hva_to_pfn()?  x86 uses gfn_to_hva_many(), in
>> one place.
> 
> Nope, I only checked that e500 adheres to that flow so far. I'm not even 100% sure that book3s is always happy yet.
> 
> But I figured this is a step in the right direction. If we missed out on one, we can always add it later. The many function is a good spot. Maybe I'll just ckeck up all of kvm_main.c again for potential users.

I'm not sure.  We have lots of functions of this sort, and their number
keeps increasing.  Maybe a better place is pre-map.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 7/8] KVM: Add page map arch callback

[Alexander Graf]

On 07.08.2012, at 16:10, Avi Kivity <avi@redhat.com> wrote:

> On 08/07/2012 05:08 PM, Alexander Graf wrote:
>> 
>> 
>> On 07.08.2012, at 15:58, Avi Kivity <avi@redhat.com> wrote:
>> 
>>> On 08/07/2012 04:44 PM, Alexander Graf wrote:
>>>> 
>>>>> 
>>>>> Is this the correct place?  Who says the caller of hva_to_pfn() is going
>>>>> to map it?
>>>> 
>>>> I don't think anyone is. However, we need the struct page, and all the generic kvm mm code tries hard to hide it from its users. The alternative would be to expose all those details, and I'm not sure that's a good idea.
>>>> 
>>>> Essentially, we don't care if we're overly cautious. Clearing one page too much is way better than clearing one too few.
>>> 
>>> Are you sure everyone uses hva_to_pfn()?  x86 uses gfn_to_hva_many(), in
>>> one place.
>> 
>> Nope, I only checked that e500 adheres to that flow so far. I'm not even 100% sure that book3s is always happy yet.
>> 
>> But I figured this is a step in the right direction. If we missed out on one, we can always add it later. The many function is a good spot. Maybe I'll just ckeck up all of kvm_main.c again for potential users.
> 
> I'm not sure.  We have lots of functions of this sort, and their number
> keeps increasing.  Maybe a better place is pre-map.

Pre-map? How?

Alex

> 
> 
> -- 
> error compiling committee.c: too many arguments to function

Re: [PATCH 7/8] KVM: Add page map arch callback

[Avi Kivity]

On 08/07/2012 05:14 PM, Alexander Graf wrote:
> 
> 
> On 07.08.2012, at 16:10, Avi Kivity <avi@redhat.com> wrote:
> 
>> On 08/07/2012 05:08 PM, Alexander Graf wrote:
>>> 
>>> 
>>> On 07.08.2012, at 15:58, Avi Kivity <avi@redhat.com> wrote:
>>> 
>>>> On 08/07/2012 04:44 PM, Alexander Graf wrote:
>>>>> 
>>>>>> 
>>>>>> Is this the correct place?  Who says the caller of hva_to_pfn() is going
>>>>>> to map it?
>>>>> 
>>>>> I don't think anyone is. However, we need the struct page, and all the generic kvm mm code tries hard to hide it from its users. The alternative would be to expose all those details, and I'm not sure that's a good idea.
>>>>> 
>>>>> Essentially, we don't care if we're overly cautious. Clearing one page too much is way better than clearing one too few.
>>>> 
>>>> Are you sure everyone uses hva_to_pfn()?  x86 uses gfn_to_hva_many(), in
>>>> one place.
>>> 
>>> Nope, I only checked that e500 adheres to that flow so far. I'm not even 100% sure that book3s is always happy yet.
>>> 
>>> But I figured this is a step in the right direction. If we missed out on one, we can always add it later. The many function is a good spot. Maybe I'll just ckeck up all of kvm_main.c again for potential users.
>> 
>> I'm not sure.  We have lots of functions of this sort, and their number
>> keeps increasing.  Maybe a better place is pre-map.
> 
> Pre-map? How?

In arch code before you install the page in a pte/tlbe.

We don't have a single point we can hook unfortunately.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 7/8] KVM: Add page map arch callback

[Alexander Graf]

On 07.08.2012, at 16:20, Avi Kivity <avi@redhat.com> wrote:

> On 08/07/2012 05:14 PM, Alexander Graf wrote:
>> 
>> 
>> On 07.08.2012, at 16:10, Avi Kivity <avi@redhat.com> wrote:
>> 
>>> On 08/07/2012 05:08 PM, Alexander Graf wrote:
>>>> 
>>>> 
>>>> On 07.08.2012, at 15:58, Avi Kivity <avi@redhat.com> wrote:
>>>> 
>>>>> On 08/07/2012 04:44 PM, Alexander Graf wrote:
>>>>>> 
>>>>>>> 
>>>>>>> Is this the correct place?  Who says the caller of hva_to_pfn() is going
>>>>>>> to map it?
>>>>>> 
>>>>>> I don't think anyone is. However, we need the struct page, and all the generic kvm mm code tries hard to hide it from its users. The alternative would be to expose all those details, and I'm not sure that's a good idea.
>>>>>> 
>>>>>> Essentially, we don't care if we're overly cautious. Clearing one page too much is way better than clearing one too few.
>>>>> 
>>>>> Are you sure everyone uses hva_to_pfn()?  x86 uses gfn_to_hva_many(), in
>>>>> one place.
>>>> 
>>>> Nope, I only checked that e500 adheres to that flow so far. I'm not even 100% sure that book3s is always happy yet.
>>>> 
>>>> But I figured this is a step in the right direction. If we missed out on one, we can always add it later. The many function is a good spot. Maybe I'll just ckeck up all of kvm_main.c again for potential users.
>>> 
>>> I'm not sure.  We have lots of functions of this sort, and their number
>>> keeps increasing.  Maybe a better place is pre-map.
>> 
>> Pre-map? How?
> 
> In arch code before you install the page in a pte/tlbe.

So how do I get to the struct page in there?

Alex

> 
> We don't have a single point we can hook unfortunately.
> 
> 
> -- 
> error compiling committee.c: too many arguments to function

Re: [PATCH 7/8] KVM: Add page map arch callback

[Avi Kivity]

On 08/07/2012 05:24 PM, Alexander Graf wrote:
>>> 
>>> Pre-map? How?
>> 
>> In arch code before you install the page in a pte/tlbe.
> 
> So how do I get to the struct page in there?
> 

pfn_to_page()


-- 
error compiling committee.c: too many arguments to function

[PATCH 8/8] KVM: PPC: Add cache flush on page map

[Alexander Graf]

When we map a page that wasn't icache cleared before, do so when first
mapping it in KVM using the same information bits as the Linux mapping
logic. That way we are 100% sure that any page we map does not have stale
entries in the icache.

What we really would need is a method to flush the icache only when we
actually map a page executable for the guest. But with the current
abstraction that is hard to do, and this way we don't have a huge performance
penalty, so better be safe now and micro optimize things later.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/include/asm/kvm_host.h |   10 ++++++++++
 arch/powerpc/mm/mem.c               |    1 +
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index ed75bc9..c0a2fc1 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -33,6 +33,7 @@
 #include <asm/kvm_asm.h>
 #include <asm/processor.h>
 #include <asm/page.h>
+#include <asm/cacheflush.h>
 
 #define KVM_MAX_VCPUS		NR_CPUS
 #define KVM_MAX_VCORES		NR_CPUS
@@ -550,5 +551,14 @@ struct kvm_vcpu_arch {
 #define KVM_MMIO_REG_FQPR	0x0060
 
 #define __KVM_HAVE_ARCH_WQP
+#define __KVM_HAVE_ARCH_MAP_PAGE
+static inline void kvm_arch_map_page(struct page *page)
+{
+	/* Need to invalidate the icache for new pages */
+	if (!test_bit(PG_arch_1, &page->flags)) {
+		flush_dcache_icache_page(page);
+		set_bit(PG_arch_1, &page->flags);
+	}
+}
 
 #endif /* __POWERPC_KVM_HOST_H__ */
diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index baaafde..fbdad0e 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -469,6 +469,7 @@ void flush_dcache_icache_page(struct page *page)
 	__flush_dcache_icache_phys(page_to_pfn(page) << PAGE_SHIFT);
 #endif
 }
+EXPORT_SYMBOL(flush_dcache_icache_page);
 
 void clear_user_page(void *page, unsigned long vaddr, struct page *pg)
 {
-- 
1.6.0.2

Re: [PATCH 8/8] KVM: PPC: Add cache flush on page map

[Scott Wood]

On 08/07/2012 05:57 AM, Alexander Graf wrote:
> When we map a page that wasn't icache cleared before, do so when first
> mapping it in KVM using the same information bits as the Linux mapping
> logic. That way we are 100% sure that any page we map does not have stale
> entries in the icache.
> 
> What we really would need is a method to flush the icache only when we
> actually map a page executable for the guest. But with the current
> abstraction that is hard to do, and this way we don't have a huge performance
> penalty, so better be safe now and micro optimize things later.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/powerpc/include/asm/kvm_host.h |   10 ++++++++++
>  arch/powerpc/mm/mem.c               |    1 +
>  2 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
> index ed75bc9..c0a2fc1 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -33,6 +33,7 @@
>  #include <asm/kvm_asm.h>
>  #include <asm/processor.h>
>  #include <asm/page.h>
> +#include <asm/cacheflush.h>
>  
>  #define KVM_MAX_VCPUS		NR_CPUS
>  #define KVM_MAX_VCORES		NR_CPUS
> @@ -550,5 +551,14 @@ struct kvm_vcpu_arch {
>  #define KVM_MMIO_REG_FQPR	0x0060
>  
>  #define __KVM_HAVE_ARCH_WQP
> +#define __KVM_HAVE_ARCH_MAP_PAGE
> +static inline void kvm_arch_map_page(struct page *page)
> +{
> +	/* Need to invalidate the icache for new pages */
> +	if (!test_bit(PG_arch_1, &page->flags)) {
> +		flush_dcache_icache_page(page);
> +		set_bit(PG_arch_1, &page->flags);
> +	}
> +}

Shouldn't this test CPU_FTR_COHERENT_ICACHE?

-Scott

Re: [PATCH 8/8] KVM: PPC: Add cache flush on page map

[Alexander Graf]

On 07.08.2012, at 23:01, Scott Wood <scottwood@freescale.com> wrote:

> On 08/07/2012 05:57 AM, Alexander Graf wrote:
>> When we map a page that wasn't icache cleared before, do so when first
>> mapping it in KVM using the same information bits as the Linux mapping
>> logic. That way we are 100% sure that any page we map does not have stale
>> entries in the icache.
>> 
>> What we really would need is a method to flush the icache only when we
>> actually map a page executable for the guest. But with the current
>> abstraction that is hard to do, and this way we don't have a huge performance
>> penalty, so better be safe now and micro optimize things later.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> arch/powerpc/include/asm/kvm_host.h |   10 ++++++++++
>> arch/powerpc/mm/mem.c               |    1 +
>> 2 files changed, 11 insertions(+), 0 deletions(-)
>> 
>> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
>> index ed75bc9..c0a2fc1 100644
>> --- a/arch/powerpc/include/asm/kvm_host.h
>> +++ b/arch/powerpc/include/asm/kvm_host.h
>> @@ -33,6 +33,7 @@
>> #include <asm/kvm_asm.h>
>> #include <asm/processor.h>
>> #include <asm/page.h>
>> +#include <asm/cacheflush.h>
>> 
>> #define KVM_MAX_VCPUS        NR_CPUS
>> #define KVM_MAX_VCORES        NR_CPUS
>> @@ -550,5 +551,14 @@ struct kvm_vcpu_arch {
>> #define KVM_MMIO_REG_FQPR    0x0060
>> 
>> #define __KVM_HAVE_ARCH_WQP
>> +#define __KVM_HAVE_ARCH_MAP_PAGE
>> +static inline void kvm_arch_map_page(struct page *page)
>> +{
>> +    /* Need to invalidate the icache for new pages */
>> +    if (!test_bit(PG_arch_1, &page->flags)) {
>> +        flush_dcache_icache_page(page);
>> +        set_bit(PG_arch_1, &page->flags);
>> +    }
>> +}
> 
> Shouldn't this test CPU_FTR_COHERENT_ICACHE?

flush_icache_range checks for it a bit further down the code stream. I mostly modeled things the same way as set_pre_filter.


Alex

Re: [PATCH 0/8] KVM: PPC: E500: Implement MMU Notifiers

[Alexander Graf]

On 07.08.2012, at 12:57, Alexander Graf wrote:

> This patch set adds a very simple implementation of MMU notifiers for the
> e500 target. Along the way, I stumbled over a few other things that I've
> put into the same patch set, namely:
> 
>  * e500 prerequisites
>  * icache flushing on page map (probably also hits ARM!)
>  * exit trace point for e500
> 
> Alexander Graf (8):
>  KVM: PPC: BookE: Expose remote TLB flushes in debugfs
>  KVM: PPC: E500: Fix clear_tlb_refs
>  KVM: PPC: PR: Use generic tracepoint for guest exit
>  KVM: PPC: Expose SYNC cap based on mmu notifiers

I applied the above 4 patches to kvm-ppc-next.


Alex