diff mbox

[GIT,PULL,04/23] KVM: s390: rewrite vcpu_post_run and drop out early

Message ID 1449054384-76374-5-git-send-email-borntraeger@de.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Christian Borntraeger Dec. 2, 2015, 11:06 a.m. UTC 
From: David Hildenbrand <dahi@linux.vnet.ibm.com>

Let's rewrite this function to better reflect how we actually handle
exit_code. By dropping out early we can save a few cycles. This
especially speeds up sie exits caused by host irqs.

Also, let's move the special -EOPNOTSUPP for intercepts to
the place where it belongs and convert it to -EREMOTE.

Reviewed-by: Dominik Dingel <dingel@linux.vnet.ibm.com>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/intercept.c |  7 +++---
 arch/s390/kvm/kvm-s390.c  | 59 +++++++++++++++++------------------------------
 2 files changed, 24 insertions(+), 42 deletions(-)

Comments

Paolo Bonzini Dec. 2, 2015, 12:20 p.m. UTC | #1 
On 02/12/2015 12:06, Christian Borntraeger wrote:
> +	memcpy(&vcpu->run->s.regs.gprs[14], &vcpu->arch.sie_block->gg14, 16);

This is preexisting but... boy it's ugly. :)

Do you gain much over the simpler

	vcpu->run->s.regs.gprs[14] = vcpu->arch.sie_block->gg14;
	vcpu->run->s.regs.gprs[15] = vcpu->arch.sie_block->gg15;

?

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christian Borntraeger Dec. 2, 2015, 1:04 p.m. UTC | #2 
On 12/02/2015 01:20 PM, Paolo Bonzini wrote:
> 
> 
> On 02/12/2015 12:06, Christian Borntraeger wrote:
>> +	memcpy(&vcpu->run->s.regs.gprs[14], &vcpu->arch.sie_block->gg14, 16);
> 
> This is preexisting but... boy it's ugly. :)
> 
> Do you gain much over the simpler
> 
> 	vcpu->run->s.regs.gprs[14] = vcpu->arch.sie_block->gg14;
> 	vcpu->run->s.regs.gprs[15] = vcpu->arch.sie_block->gg15;
> 

Its just legacy code from the old days.
There is a difference, but it seems to a missed opportunity from gcc

        vcpu->arch.sie_block->gg14 = vcpu->run->s.regs.gprs[14];
    839c:       e3 30 f0 b8 00 04       lg      %r3,184(%r15)
    83a2:       e3 10 32 40 00 04       lg      %r1,576(%r3)
    83a8:       e3 20 30 80 00 04       lg      %r2,128(%r3)
    83ae:       e3 20 21 b8 00 04       lg      %r2,440(%r2)
    83b4:       e3 20 10 a0 00 24       stg     %r2,160(%r1)
        vcpu->arch.sie_block->gg15 = vcpu->run->s.regs.gprs[15];
    83ba:       e3 10 32 40 00 04       lg      %r1,576(%r3)
    83c0:       e3 20 30 80 00 04       lg      %r2,128(%r3)
    83c6:       e3 20 21 c0 00 04       lg      %r2,448(%r2)
    83cc:       e3 20 10 a8 00 24       stg     %r2,168(%r1)

gcc seems to reuse and reload %r2 and %r3, maybe register pressure.



the memcpy gives

    memcpy(&vcpu->arch.sie_block->gg14, &vcpu->run->s.regs.gprs[14], 16);
    839c:       e3 30 f0 b8 00 04       lg      %r3,184(%r15)
    83a2:       e3 10 32 40 00 04       lg      %r1,576(%r3)
    83a8:       e3 20 30 80 00 04       lg      %r2,128(%r3)
    83ae:       d2 0f 10 a0 21 b8       mvc     160(16,%r1),440(%r2)

I will prepare a patch and do my usual micro benchmark. Unless
things get much worse I will schedule this for the next pull.

Christian


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paolo Bonzini Dec. 2, 2015, 1:05 p.m. UTC | #3 
On 02/12/2015 14:04, Christian Borntraeger wrote:
>> > Do you gain much over the simpler
>> > 
>> > 	vcpu->run->s.regs.gprs[14] = vcpu->arch.sie_block->gg14;
>> > 	vcpu->run->s.regs.gprs[15] = vcpu->arch.sie_block->gg15;
>> > 
> Its just legacy code from the old days.
> There is a difference, but it seems to a missed opportunity from gcc
> 
>         vcpu->arch.sie_block->gg14 = vcpu->run->s.regs.gprs[14];
>     839c:       e3 30 f0 b8 00 04       lg      %r3,184(%r15)
>     83a2:       e3 10 32 40 00 04       lg      %r1,576(%r3)
>     83a8:       e3 20 30 80 00 04       lg      %r2,128(%r3)
>     83ae:       e3 20 21 b8 00 04       lg      %r2,440(%r2)
>     83b4:       e3 20 10 a0 00 24       stg     %r2,160(%r1)
>         vcpu->arch.sie_block->gg15 = vcpu->run->s.regs.gprs[15];
>     83ba:       e3 10 32 40 00 04       lg      %r1,576(%r3)
>     83c0:       e3 20 30 80 00 04       lg      %r2,128(%r3)
>     83c6:       e3 20 21 c0 00 04       lg      %r2,448(%r2)
>     83cc:       e3 20 10 a8 00 24       stg     %r2,168(%r1)
> 
> gcc seems to reuse and reload %r2 and %r3, maybe register pressure.

More likely to be -fno-strict-aliasing. :(

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christian Borntraeger Dec. 2, 2015, 2 p.m. UTC | #4 
On 12/02/2015 02:05 PM, Paolo Bonzini wrote:
> 
> 
> On 02/12/2015 14:04, Christian Borntraeger wrote:
>>>> Do you gain much over the simpler
>>>>
>>>> 	vcpu->run->s.regs.gprs[14] = vcpu->arch.sie_block->gg14;
>>>> 	vcpu->run->s.regs.gprs[15] = vcpu->arch.sie_block->gg15;
>>>>
>> Its just legacy code from the old days.
>> There is a difference, but it seems to a missed opportunity from gcc
>>
>>         vcpu->arch.sie_block->gg14 = vcpu->run->s.regs.gprs[14];
>>     839c:       e3 30 f0 b8 00 04       lg      %r3,184(%r15)
>>     83a2:       e3 10 32 40 00 04       lg      %r1,576(%r3)
>>     83a8:       e3 20 30 80 00 04       lg      %r2,128(%r3)
>>     83ae:       e3 20 21 b8 00 04       lg      %r2,440(%r2)
>>     83b4:       e3 20 10 a0 00 24       stg     %r2,160(%r1)
>>         vcpu->arch.sie_block->gg15 = vcpu->run->s.regs.gprs[15];
>>     83ba:       e3 10 32 40 00 04       lg      %r1,576(%r3)
>>     83c0:       e3 20 30 80 00 04       lg      %r2,128(%r3)
>>     83c6:       e3 20 21 c0 00 04       lg      %r2,448(%r2)
>>     83cc:       e3 20 10 a8 00 24       stg     %r2,168(%r1)
>>
>> gcc seems to reuse and reload %r2 and %r3, maybe register pressure.
> 
> More likely to be -fno-strict-aliasing. :(

Yes its indeed the aliasing. 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
index b4a5aa1..d53c107 100644
--- a/arch/s390/kvm/intercept.c
+++ b/arch/s390/kvm/intercept.c
@@ -54,9 +54,6 @@  void kvm_s390_rewind_psw(struct kvm_vcpu *vcpu, int ilc)
 static int handle_noop(struct kvm_vcpu *vcpu)
 {
 	switch (vcpu->arch.sie_block->icptcode) {
-	case 0x0:
-		vcpu->stat.exit_null++;
-		break;
 	case 0x10:
 		vcpu->stat.exit_external_request++;
 		break;
@@ -338,8 +335,10 @@  static int handle_partial_execution(struct kvm_vcpu *vcpu)
 
 int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)
 {
+	if (kvm_is_ucontrol(vcpu->kvm))
+		return -EOPNOTSUPP;
+
 	switch (vcpu->arch.sie_block->icptcode) {
-	case 0x00:
 	case 0x10:
 	case 0x18:
 		return handle_noop(vcpu);
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 8465892..5c36c8e 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2071,8 +2071,6 @@  static int vcpu_post_run_fault_in_sie(struct kvm_vcpu *vcpu)
 
 static int vcpu_post_run(struct kvm_vcpu *vcpu, int exit_reason)
 {
-	int rc = -1;
-
 	VCPU_EVENT(vcpu, 6, "exit sie icptcode %d",
 		   vcpu->arch.sie_block->icptcode);
 	trace_kvm_s390_sie_exit(vcpu, vcpu->arch.sie_block->icptcode);
@@ -2080,40 +2078,35 @@  static int vcpu_post_run(struct kvm_vcpu *vcpu, int exit_reason)
 	if (guestdbg_enabled(vcpu))
 		kvm_s390_restore_guest_per_regs(vcpu);
 
-	if (exit_reason >= 0) {
-		rc = 0;
+	memcpy(&vcpu->run->s.regs.gprs[14], &vcpu->arch.sie_block->gg14, 16);
+
+	if (vcpu->arch.sie_block->icptcode > 0) {
+		int rc = kvm_handle_sie_intercept(vcpu);
+
+		if (rc != -EOPNOTSUPP)
+			return rc;
+		vcpu->run->exit_reason = KVM_EXIT_S390_SIEIC;
+		vcpu->run->s390_sieic.icptcode = vcpu->arch.sie_block->icptcode;
+		vcpu->run->s390_sieic.ipa = vcpu->arch.sie_block->ipa;
+		vcpu->run->s390_sieic.ipb = vcpu->arch.sie_block->ipb;
+		return -EREMOTE;
+	} else if (exit_reason != -EFAULT) {
+		vcpu->stat.exit_null++;
+		return 0;
 	} else if (kvm_is_ucontrol(vcpu->kvm)) {
 		vcpu->run->exit_reason = KVM_EXIT_S390_UCONTROL;
 		vcpu->run->s390_ucontrol.trans_exc_code =
 						current->thread.gmap_addr;
 		vcpu->run->s390_ucontrol.pgm_code = 0x10;
-		rc = -EREMOTE;
-
+		return -EREMOTE;
 	} else if (current->thread.gmap_pfault) {
 		trace_kvm_s390_major_guest_pfault(vcpu);
 		current->thread.gmap_pfault = 0;
-		if (kvm_arch_setup_async_pf(vcpu)) {
-			rc = 0;
-		} else {
-			gpa_t gpa = current->thread.gmap_addr;
-			rc = kvm_arch_fault_in_page(vcpu, gpa, 1);
-		}
+		if (kvm_arch_setup_async_pf(vcpu))
+			return 0;
+		return kvm_arch_fault_in_page(vcpu, current->thread.gmap_addr, 1);
 	}
-
-	if (rc == -1)
-		rc = vcpu_post_run_fault_in_sie(vcpu);
-
-	memcpy(&vcpu->run->s.regs.gprs[14], &vcpu->arch.sie_block->gg14, 16);
-
-	if (rc == 0) {
-		if (kvm_is_ucontrol(vcpu->kvm))
-			/* Don't exit for host interrupts. */
-			rc = vcpu->arch.sie_block->icptcode ? -EOPNOTSUPP : 0;
-		else
-			rc = kvm_handle_sie_intercept(vcpu);
-	}
-
-	return rc;
+	return vcpu_post_run_fault_in_sie(vcpu);
 }
 
 static int __vcpu_run(struct kvm_vcpu *vcpu)
@@ -2233,18 +2226,8 @@  int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 		rc = 0;
 	}
 
-	if (rc == -EOPNOTSUPP) {
-		/* intercept cannot be handled in-kernel, prepare kvm-run */
-		kvm_run->exit_reason         = KVM_EXIT_S390_SIEIC;
-		kvm_run->s390_sieic.icptcode = vcpu->arch.sie_block->icptcode;
-		kvm_run->s390_sieic.ipa      = vcpu->arch.sie_block->ipa;
-		kvm_run->s390_sieic.ipb      = vcpu->arch.sie_block->ipb;
-		rc = 0;
-	}
-
 	if (rc == -EREMOTE) {
-		/* intercept was handled, but userspace support is needed
-		 * kvm_run has been prepared by the handler */
+		/* userspace support is needed, kvm_run has been prepared */
 		rc = 0;
 	}