Patchwork [14/23] KVM: PPC: Book3S PR: Delay disabling relocation-on interrupts

login
register
mail settings
Submitter Paul Mackerras
Date Aug. 6, 2013, 4:23 a.m.
Message ID <20130806042337.GT19254@iris.ozlabs.ibm.com>
Download mbox | patch
Permalink /patch/264865/
State New
Headers show

Comments

Paul Mackerras - Aug. 6, 2013, 4:23 a.m.
When we are running a PR KVM guest on POWER8, we have to disable the
new POWER8 feature of taking interrupts with relocation on, that is,
of taking interrupts without disabling the MMU, because the SLB does
not contain the normal kernel SLB entries while in the guest.
Currently we disable relocation-on interrupts when a PR guest is
created, and leave it disabled until there are no more PR guests in
existence.

This defers the disabling of relocation-on interrupts until the first
time a PR KVM guest vcpu is run.  The reason is that in future we will
support both PR and HV guests in the same kernel, and this will avoid
disabling relocation-on interrupts unnecessarily for guests which turn
out to be HV guests, as we will not know at VM creation time whether
it will be a PR or a HV guest.

Signed-off-by: Paul Mackerras <paulus@samba.org>
---
 arch/powerpc/include/asm/kvm_host.h |  1 +
 arch/powerpc/kvm/book3s_pr.c        | 71 ++++++++++++++++++++++++++-----------
 2 files changed, 52 insertions(+), 20 deletions(-)
Alexander Graf - Aug. 30, 2013, 4:30 p.m.
On 06.08.2013, at 06:23, Paul Mackerras wrote:

> When we are running a PR KVM guest on POWER8, we have to disable the
> new POWER8 feature of taking interrupts with relocation on, that is,
> of taking interrupts without disabling the MMU, because the SLB does
> not contain the normal kernel SLB entries while in the guest.
> Currently we disable relocation-on interrupts when a PR guest is
> created, and leave it disabled until there are no more PR guests in
> existence.
> 
> This defers the disabling of relocation-on interrupts until the first

It would've been nice to see the original patch on kvm-ppc@vger.

> time a PR KVM guest vcpu is run.  The reason is that in future we will
> support both PR and HV guests in the same kernel, and this will avoid
> disabling relocation-on interrupts unnecessarily for guests which turn
> out to be HV guests, as we will not know at VM creation time whether
> it will be a PR or a HV guest.
> 
> Signed-off-by: Paul Mackerras <paulus@samba.org>
> ---
> arch/powerpc/include/asm/kvm_host.h |  1 +
> arch/powerpc/kvm/book3s_pr.c        | 71 ++++++++++++++++++++++++++-----------
> 2 files changed, 52 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
> index 4d83972..c012db2 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -264,6 +264,7 @@ struct kvm_arch {
> #endif /* CONFIG_KVM_BOOK3S_64_HV */
> #ifdef CONFIG_KVM_BOOK3S_PR
> 	struct mutex hpt_mutex;
> +	bool relon_disabled;
> #endif
> #ifdef CONFIG_PPC_BOOK3S_64
> 	struct list_head spapr_tce_tables;
> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
> index 5b06a70..2759ddc 100644
> --- a/arch/powerpc/kvm/book3s_pr.c
> +++ b/arch/powerpc/kvm/book3s_pr.c
> @@ -1197,6 +1197,47 @@ void kvmppc_core_vcpu_free(struct kvm_vcpu *vcpu)
> 	kmem_cache_free(kvm_vcpu_cache, vcpu);
> }
> 
> +/*
> + * On POWER8, we have to disable relocation-on interrupts while
> + * we are in the guest, since the guest doesn't have the normal
> + * kernel SLB contents.  Since disabling relocation-on interrupts
> + * is a fairly heavy-weight operation, we do it once when starting
> + * the first guest vcpu and leave it disabled until the last guest
> + * has been destroyed.
> + */
> +static unsigned int kvm_global_user_count = 0;
> +static DEFINE_SPINLOCK(kvm_global_user_count_lock);
> +
> +static void disable_relon_interrupts(struct kvm *kvm)
> +{
> +	mutex_lock(&kvm->lock);
> +	if (!kvm->arch.relon_disabled) {
> +		if (firmware_has_feature(FW_FEATURE_SET_MODE)) {

Is this the same as the endianness setting rtas call? If so, would a PR guest in an HV guest that provides only endianness setting but no relocation-on setting confuse any of this code?


Alex

> +			spin_lock(&kvm_global_user_count_lock);
> +			if (++kvm_global_user_count == 1)
> +				pSeries_disable_reloc_on_exc();
> +			spin_unlock(&kvm_global_user_count_lock);
> +		}
> +		/* order disabling above with setting relon_disabled */
> +		smp_mb();
> +		kvm->arch.relon_disabled = true;
> +	}
> +	mutex_unlock(&kvm->lock);
> +}
> +
> +static void enable_relon_interrupts(struct kvm *kvm)
> +{
> +	if (kvm->arch.relon_disabled &&
> +	    firmware_has_feature(FW_FEATURE_SET_MODE)) {
> +		spin_lock(&kvm_global_user_count_lock);
> +		BUG_ON(kvm_global_user_count == 0);
> +		if (--kvm_global_user_count == 0)
> +			pSeries_enable_reloc_on_exc();
> +		spin_unlock(&kvm_global_user_count_lock);
> +	}
> +	kvm->arch.relon_disabled = false;
> +}
> +
> int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
> {
> 	int ret;
> @@ -1234,6 +1275,9 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
> 		goto out;
> 	}
> 
> +	if (!vcpu->kvm->arch.relon_disabled)
> +		disable_relon_interrupts(vcpu->kvm);
> +
> 	/* Save FPU state in stack */
> 	if (current->thread.regs->msr & MSR_FP)
> 		giveup_fpu(current);
> @@ -1400,9 +1444,6 @@ void kvmppc_core_flush_memslot(struct kvm *kvm, struct kvm_memory_slot *memslot)
> {
> }
> 
> -static unsigned int kvm_global_user_count = 0;
> -static DEFINE_SPINLOCK(kvm_global_user_count_lock);
> -
> int kvmppc_core_init_vm(struct kvm *kvm)
> {
> #ifdef CONFIG_PPC64
> @@ -1411,28 +1452,18 @@ int kvmppc_core_init_vm(struct kvm *kvm)
> #endif
> 	mutex_init(&kvm->arch.hpt_mutex);
> 
> -	if (firmware_has_feature(FW_FEATURE_SET_MODE)) {
> -		spin_lock(&kvm_global_user_count_lock);
> -		if (++kvm_global_user_count == 1)
> -			pSeries_disable_reloc_on_exc();
> -		spin_unlock(&kvm_global_user_count_lock);
> -	}
> +	/*
> +	 * If we don't have relocation-on interrupts at all,
> +	 * then we can consider them to be already disabled.
> +	 */
> +	kvm->arch.relon_disabled = !firmware_has_feature(FW_FEATURE_SET_MODE);
> +
> 	return 0;
> }
> 
> void kvmppc_core_destroy_vm(struct kvm *kvm)
> {
> -#ifdef CONFIG_PPC64
> -	WARN_ON(!list_empty(&kvm->arch.spapr_tce_tables));
> -#endif
> -
> -	if (firmware_has_feature(FW_FEATURE_SET_MODE)) {
> -		spin_lock(&kvm_global_user_count_lock);
> -		BUG_ON(kvm_global_user_count == 0);
> -		if (--kvm_global_user_count == 0)
> -			pSeries_enable_reloc_on_exc();
> -		spin_unlock(&kvm_global_user_count_lock);
> -	}
> +	enable_relon_interrupts(kvm);
> }
> 
> static int kvmppc_book3s_init(void)
> -- 
> 1.8.3.1
> 

--
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
Paul Mackerras - Aug. 30, 2013, 10:55 p.m.
On Fri, Aug 30, 2013 at 06:30:50PM +0200, Alexander Graf wrote:
> 
> On 06.08.2013, at 06:23, Paul Mackerras wrote:
> 
> > When we are running a PR KVM guest on POWER8, we have to disable the
> > new POWER8 feature of taking interrupts with relocation on, that is,
> > of taking interrupts without disabling the MMU, because the SLB does
> > not contain the normal kernel SLB entries while in the guest.
> > Currently we disable relocation-on interrupts when a PR guest is
> > created, and leave it disabled until there are no more PR guests in
> > existence.
> > 
> > This defers the disabling of relocation-on interrupts until the first
> 
> It would've been nice to see the original patch on kvm-ppc@vger.

Here are the headers from my copy of the original mail:

> Date: Tue, 6 Aug 2013 14:23:37 +1000
> From: Paul Mackerras <paulus@samba.org>
> To: Alexander Graf <agraf@suse.de>, Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: kvm-ppc@vger.kernel.org, kvm@vger.kernel.org
> Subject: [PATCH 14/23] KVM: PPC: Book3S PR: Delay disabling relocation-on interrupts

So as far as I can see, I *did* cc it to kvm-ppc@vger.

> > +	if (!kvm->arch.relon_disabled) {
> > +		if (firmware_has_feature(FW_FEATURE_SET_MODE)) {
> 
> Is this the same as the endianness setting rtas call? If so, would a PR guest in an HV guest that provides only endianness setting but no relocation-on setting confuse any of this code?

It is the same hcall, but since the interrupts-with-relocation-on
function was defined in the first PAPR version that has H_SET_MODE,
we shouldn't ever hit that situation.  In any case, if we did happen
to run under a (non PAPR-compliant) hypervisor that implemented
H_SET_MODE but not the relocation-on setting, then we couldn't have
enabled relocation-on interrupts in the first place, so it wouldn't
matter.

Paul.
--
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
Alexander Graf - Aug. 30, 2013, 11:13 p.m.
On 31.08.2013, at 00:55, Paul Mackerras wrote:

> On Fri, Aug 30, 2013 at 06:30:50PM +0200, Alexander Graf wrote:
>> 
>> On 06.08.2013, at 06:23, Paul Mackerras wrote:
>> 
>>> When we are running a PR KVM guest on POWER8, we have to disable the
>>> new POWER8 feature of taking interrupts with relocation on, that is,
>>> of taking interrupts without disabling the MMU, because the SLB does
>>> not contain the normal kernel SLB entries while in the guest.
>>> Currently we disable relocation-on interrupts when a PR guest is
>>> created, and leave it disabled until there are no more PR guests in
>>> existence.
>>> 
>>> This defers the disabling of relocation-on interrupts until the first
>> 
>> It would've been nice to see the original patch on kvm-ppc@vger.
> 
> Here are the headers from my copy of the original mail:
> 
>> Date: Tue, 6 Aug 2013 14:23:37 +1000
>> From: Paul Mackerras <paulus@samba.org>
>> To: Alexander Graf <agraf@suse.de>, Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: kvm-ppc@vger.kernel.org, kvm@vger.kernel.org
>> Subject: [PATCH 14/23] KVM: PPC: Book3S PR: Delay disabling relocation-on interrupts
> 
> So as far as I can see, I *did* cc it to kvm-ppc@vger.

Oh, sorry to not be more explicit here. I meant the one that actually introduced the relocation-on handling:

  https://lists.ozlabs.org/pipermail/linuxppc-dev/2012-December/102355.html

I can't find any trace of that in my inbox, even though it clearly touches KVM PPC code.

> 
>>> +	if (!kvm->arch.relon_disabled) {
>>> +		if (firmware_has_feature(FW_FEATURE_SET_MODE)) {
>> 
>> Is this the same as the endianness setting rtas call? If so, would a PR guest in an HV guest that provides only endianness setting but no relocation-on setting confuse any of this code?
> 
> It is the same hcall, but since the interrupts-with-relocation-on
> function was defined in the first PAPR version that has H_SET_MODE,
> we shouldn't ever hit that situation.  In any case, if we did happen
> to run under a (non PAPR-compliant) hypervisor that implemented
> H_SET_MODE but not the relocation-on setting, then we couldn't have
> enabled relocation-on interrupts in the first place, so it wouldn't
> matter.

Well, I think Anton's patches do exactly that:

  https://lists.nongnu.org/archive/html/qemu-ppc/2013-08/msg00253.html

I really just want to double-check that we're not shooting ourselves in the foot here.


Alex

--
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
Paul Mackerras - Aug. 31, 2013, 5:42 a.m.
On Sat, Aug 31, 2013 at 01:13:07AM +0200, Alexander Graf wrote:
> 
> Oh, sorry to not be more explicit here. I meant the one that actually introduced the relocation-on handling:
> 
>   https://lists.ozlabs.org/pipermail/linuxppc-dev/2012-December/102355.html
> 
> I can't find any trace of that in my inbox, even though it clearly touches KVM PPC code.

True, Ian should have cc'd it to kvm-ppc@vger, I'll mention it to him.

> 
> > 
> >>> +	if (!kvm->arch.relon_disabled) {
> >>> +		if (firmware_has_feature(FW_FEATURE_SET_MODE)) {
> >> 
> >> Is this the same as the endianness setting rtas call? If so, would a PR guest in an HV guest that provides only endianness setting but no relocation-on setting confuse any of this code?
> > 
> > It is the same hcall, but since the interrupts-with-relocation-on
> > function was defined in the first PAPR version that has H_SET_MODE,
> > we shouldn't ever hit that situation.  In any case, if we did happen
> > to run under a (non PAPR-compliant) hypervisor that implemented
> > H_SET_MODE but not the relocation-on setting, then we couldn't have
> > enabled relocation-on interrupts in the first place, so it wouldn't
> > matter.
> 
> Well, I think Anton's patches do exactly that:
> 
>   https://lists.nongnu.org/archive/html/qemu-ppc/2013-08/msg00253.html
> 
> I really just want to double-check that we're not shooting ourselves in the foot here.

I still think there's no real problem, since there would be no other
way to enable relocation-on interrupts other than H_SET_MODE.  So if
H_SET_MODE can't control that setting, then it must be disabled
already.

However, we should also make sure that H_SET_MODE supports changing
the relocation-on setting when it first goes in.  I'm going to want
that soon anyway since I'm working on POWER8 KVM support at the
moment.

Paul.
--
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

Patch

diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index 4d83972..c012db2 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -264,6 +264,7 @@  struct kvm_arch {
 #endif /* CONFIG_KVM_BOOK3S_64_HV */
 #ifdef CONFIG_KVM_BOOK3S_PR
 	struct mutex hpt_mutex;
+	bool relon_disabled;
 #endif
 #ifdef CONFIG_PPC_BOOK3S_64
 	struct list_head spapr_tce_tables;
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index 5b06a70..2759ddc 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -1197,6 +1197,47 @@  void kvmppc_core_vcpu_free(struct kvm_vcpu *vcpu)
 	kmem_cache_free(kvm_vcpu_cache, vcpu);
 }
 
+/*
+ * On POWER8, we have to disable relocation-on interrupts while
+ * we are in the guest, since the guest doesn't have the normal
+ * kernel SLB contents.  Since disabling relocation-on interrupts
+ * is a fairly heavy-weight operation, we do it once when starting
+ * the first guest vcpu and leave it disabled until the last guest
+ * has been destroyed.
+ */
+static unsigned int kvm_global_user_count = 0;
+static DEFINE_SPINLOCK(kvm_global_user_count_lock);
+
+static void disable_relon_interrupts(struct kvm *kvm)
+{
+	mutex_lock(&kvm->lock);
+	if (!kvm->arch.relon_disabled) {
+		if (firmware_has_feature(FW_FEATURE_SET_MODE)) {
+			spin_lock(&kvm_global_user_count_lock);
+			if (++kvm_global_user_count == 1)
+				pSeries_disable_reloc_on_exc();
+			spin_unlock(&kvm_global_user_count_lock);
+		}
+		/* order disabling above with setting relon_disabled */
+		smp_mb();
+		kvm->arch.relon_disabled = true;
+	}
+	mutex_unlock(&kvm->lock);
+}
+
+static void enable_relon_interrupts(struct kvm *kvm)
+{
+	if (kvm->arch.relon_disabled &&
+	    firmware_has_feature(FW_FEATURE_SET_MODE)) {
+		spin_lock(&kvm_global_user_count_lock);
+		BUG_ON(kvm_global_user_count == 0);
+		if (--kvm_global_user_count == 0)
+			pSeries_enable_reloc_on_exc();
+		spin_unlock(&kvm_global_user_count_lock);
+	}
+	kvm->arch.relon_disabled = false;
+}
+
 int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 {
 	int ret;
@@ -1234,6 +1275,9 @@  int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 		goto out;
 	}
 
+	if (!vcpu->kvm->arch.relon_disabled)
+		disable_relon_interrupts(vcpu->kvm);
+
 	/* Save FPU state in stack */
 	if (current->thread.regs->msr & MSR_FP)
 		giveup_fpu(current);
@@ -1400,9 +1444,6 @@  void kvmppc_core_flush_memslot(struct kvm *kvm, struct kvm_memory_slot *memslot)
 {
 }
 
-static unsigned int kvm_global_user_count = 0;
-static DEFINE_SPINLOCK(kvm_global_user_count_lock);
-
 int kvmppc_core_init_vm(struct kvm *kvm)
 {
 #ifdef CONFIG_PPC64
@@ -1411,28 +1452,18 @@  int kvmppc_core_init_vm(struct kvm *kvm)
 #endif
 	mutex_init(&kvm->arch.hpt_mutex);
 
-	if (firmware_has_feature(FW_FEATURE_SET_MODE)) {
-		spin_lock(&kvm_global_user_count_lock);
-		if (++kvm_global_user_count == 1)
-			pSeries_disable_reloc_on_exc();
-		spin_unlock(&kvm_global_user_count_lock);
-	}
+	/*
+	 * If we don't have relocation-on interrupts at all,
+	 * then we can consider them to be already disabled.
+	 */
+	kvm->arch.relon_disabled = !firmware_has_feature(FW_FEATURE_SET_MODE);
+
 	return 0;
 }
 
 void kvmppc_core_destroy_vm(struct kvm *kvm)
 {
-#ifdef CONFIG_PPC64
-	WARN_ON(!list_empty(&kvm->arch.spapr_tce_tables));
-#endif
-
-	if (firmware_has_feature(FW_FEATURE_SET_MODE)) {
-		spin_lock(&kvm_global_user_count_lock);
-		BUG_ON(kvm_global_user_count == 0);
-		if (--kvm_global_user_count == 0)
-			pSeries_enable_reloc_on_exc();
-		spin_unlock(&kvm_global_user_count_lock);
-	}
+	enable_relon_interrupts(kvm);
 }
 
 static int kvmppc_book3s_init(void)