[PATCH 0/2] KVM: s390: avoid jump tables

[PATCH 0/2] KVM: s390: avoid jump tables

[Christian Borntraeger]

Some old patches refreshed.

Christian Borntraeger (2):
  KVM: s390: use switch vs jump table in priv.c
  KVM: s390: use switch vs jump table in intercept.c

 arch/s390/kvm/intercept.c |  54 +++++++-------
 arch/s390/kvm/kvm-s390.h  |   2 -
 arch/s390/kvm/priv.c      | 183 +++++++++++++++++++++++-----------------------
 3 files changed, 119 insertions(+), 120 deletions(-)

-- 
2.14.3

[PATCH 1/2] KVM: s390: use switch vs jump table in priv.c

[Christian Borntraeger]

instead of having huge jump tables for function selection,
lets use normal switch/case statements for the instruction
handlers in priv.c

bloat-o-meter shows that the saving are even bigger than
just the removed jump tables.

add/remove: 0/11 grow/shrink: 8/0 up/down: 1934/-10246 (-8312)
Function                                     old     new   delta
kvm_s390_handle_b2                            42     958    +916
handle_iske                                  178     558    +380
handle_rrbe                                  178     546    +368
kvm_s390_handle_b9                            42     222    +180
kvm_s390_handle_01                            42      74     +32
kvm_s390_handle_eb                            42      70     +28
handle_sckpf                                 176     204     +28
handle_lctlg                                 628     630      +2
handle_ptff                                   36       -     -36
handle_sckpf.part                             78       -     -78
handle_epsw                                  154       -    -154
handle_stfl                                  316       -    -316
handle_rrbe.part                             470       -    -470
handle_iske.part                             482       -    -482
handle_io_inst                               518       -    -518
x01_handlers                                2048       -   -2048
eb_handlers                                 2048       -   -2048
b9_handlers                                 2048       -   -2048
b2_handlers                                 2048       -   -2048

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/priv.c | 183 +++++++++++++++++++++++++--------------------------
 1 file changed, 91 insertions(+), 92 deletions(-)

diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index c4c4e157c036..a74578cdd3f3 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -795,55 +795,60 @@ static int handle_stsi(struct kvm_vcpu *vcpu)
 	return rc;
 }
 
-static const intercept_handler_t b2_handlers[256] = {
-	[0x02] = handle_stidp,
-	[0x04] = handle_set_clock,
-	[0x10] = handle_set_prefix,
-	[0x11] = handle_store_prefix,
-	[0x12] = handle_store_cpu_address,
-	[0x14] = kvm_s390_handle_vsie,
-	[0x21] = handle_ipte_interlock,
-	[0x29] = handle_iske,
-	[0x2a] = handle_rrbe,
-	[0x2b] = handle_sske,
-	[0x2c] = handle_test_block,
-	[0x30] = handle_io_inst,
-	[0x31] = handle_io_inst,
-	[0x32] = handle_io_inst,
-	[0x33] = handle_io_inst,
-	[0x34] = handle_io_inst,
-	[0x35] = handle_io_inst,
-	[0x36] = handle_io_inst,
-	[0x37] = handle_io_inst,
-	[0x38] = handle_io_inst,
-	[0x39] = handle_io_inst,
-	[0x3a] = handle_io_inst,
-	[0x3b] = handle_io_inst,
-	[0x3c] = handle_io_inst,
-	[0x50] = handle_ipte_interlock,
-	[0x56] = handle_sthyi,
-	[0x5f] = handle_io_inst,
-	[0x74] = handle_io_inst,
-	[0x76] = handle_io_inst,
-	[0x7d] = handle_stsi,
-	[0xb1] = handle_stfl,
-	[0xb2] = handle_lpswe,
-};
-
 int kvm_s390_handle_b2(struct kvm_vcpu *vcpu)
 {
-	intercept_handler_t handler;
-
-	/*
-	 * A lot of B2 instructions are priviledged. Here we check for
-	 * the privileged ones, that we can handle in the kernel.
-	 * Anything else goes to userspace.
-	 */
-	handler = b2_handlers[vcpu->arch.sie_block->ipa & 0x00ff];
-	if (handler)
-		return handler(vcpu);
-
-	return -EOPNOTSUPP;
+	switch (vcpu->arch.sie_block->ipa & 0x00ff) {
+	case 0x02:
+		return handle_stidp(vcpu);
+	case 0x04:
+		return handle_set_clock(vcpu);
+	case 0x10:
+		return handle_set_prefix(vcpu);
+	case 0x11:
+		return handle_store_prefix(vcpu);
+	case 0x12:
+		return handle_store_cpu_address(vcpu);
+	case 0x14:
+		return kvm_s390_handle_vsie(vcpu);
+	case 0x21:
+	case 0x50:
+		return handle_ipte_interlock(vcpu);
+	case 0x29:
+		return handle_iske(vcpu);
+	case 0x2a:
+		return handle_rrbe(vcpu);
+	case 0x2b:
+		return handle_sske(vcpu);
+	case 0x2c:
+		return handle_test_block(vcpu);
+	case 0x30:
+	case 0x31:
+	case 0x32:
+	case 0x33:
+	case 0x34:
+	case 0x35:
+	case 0x36:
+	case 0x37:
+	case 0x38:
+	case 0x39:
+	case 0x3a:
+	case 0x3b:
+	case 0x3c:
+	case 0x5f:
+	case 0x74:
+	case 0x76:
+		return handle_io_inst(vcpu);
+	case 0x56:
+		return handle_sthyi(vcpu);
+	case 0x7d:
+		return handle_stsi(vcpu);
+	case 0xb1:
+		return handle_stfl(vcpu);
+	case 0xb2:
+		return handle_lpswe(vcpu);
+	default:
+		return -EOPNOTSUPP;
+	}
 }
 
 static int handle_epsw(struct kvm_vcpu *vcpu)
@@ -1105,25 +1110,22 @@ static int handle_essa(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
-static const intercept_handler_t b9_handlers[256] = {
-	[0x8a] = handle_ipte_interlock,
-	[0x8d] = handle_epsw,
-	[0x8e] = handle_ipte_interlock,
-	[0x8f] = handle_ipte_interlock,
-	[0xab] = handle_essa,
-	[0xaf] = handle_pfmf,
-};
-
 int kvm_s390_handle_b9(struct kvm_vcpu *vcpu)
 {
-	intercept_handler_t handler;
-
-	/* This is handled just as for the B2 instructions. */
-	handler = b9_handlers[vcpu->arch.sie_block->ipa & 0x00ff];
-	if (handler)
-		return handler(vcpu);
-
-	return -EOPNOTSUPP;
+	switch (vcpu->arch.sie_block->ipa & 0x00ff) {
+	case 0x8a:
+	case 0x8e:
+	case 0x8f:
+		return handle_ipte_interlock(vcpu);
+	case 0x8d:
+		return handle_epsw(vcpu);
+	case 0xab:
+		return handle_essa(vcpu);
+	case 0xaf:
+		return handle_pfmf(vcpu);
+	default:
+		return -EOPNOTSUPP;
+	}
 }
 
 int kvm_s390_handle_lctl(struct kvm_vcpu *vcpu)
@@ -1271,22 +1273,20 @@ static int handle_stctg(struct kvm_vcpu *vcpu)
 	return rc ? kvm_s390_inject_prog_cond(vcpu, rc) : 0;
 }
 
-static const intercept_handler_t eb_handlers[256] = {
-	[0x2f] = handle_lctlg,
-	[0x25] = handle_stctg,
-	[0x60] = handle_ri,
-	[0x61] = handle_ri,
-	[0x62] = handle_ri,
-};
-
 int kvm_s390_handle_eb(struct kvm_vcpu *vcpu)
 {
-	intercept_handler_t handler;
-
-	handler = eb_handlers[vcpu->arch.sie_block->ipb & 0xff];
-	if (handler)
-		return handler(vcpu);
-	return -EOPNOTSUPP;
+	switch (vcpu->arch.sie_block->ipb & 0x000000ff) {
+	case 0x25:
+		return handle_stctg(vcpu);
+	case 0x2f:
+		return handle_lctlg(vcpu);
+	case 0x60:
+	case 0x61:
+	case 0x62:
+		return handle_ri(vcpu);
+	default:
+		return -EOPNOTSUPP;
+	}
 }
 
 static int handle_tprot(struct kvm_vcpu *vcpu)
@@ -1346,10 +1346,12 @@ static int handle_tprot(struct kvm_vcpu *vcpu)
 
 int kvm_s390_handle_e5(struct kvm_vcpu *vcpu)
 {
-	/* For e5xx... instructions we only handle TPROT */
-	if ((vcpu->arch.sie_block->ipa & 0x00ff) == 0x01)
+	switch (vcpu->arch.sie_block->ipa & 0x00ff) {
+	case 0x01:
 		return handle_tprot(vcpu);
-	return -EOPNOTSUPP;
+	default:
+		return -EOPNOTSUPP;
+	}
 }
 
 static int handle_sckpf(struct kvm_vcpu *vcpu)
@@ -1380,17 +1382,14 @@ static int handle_ptff(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
-static const intercept_handler_t x01_handlers[256] = {
-	[0x04] = handle_ptff,
-	[0x07] = handle_sckpf,
-};
-
 int kvm_s390_handle_01(struct kvm_vcpu *vcpu)
 {
-	intercept_handler_t handler;
-
-	handler = x01_handlers[vcpu->arch.sie_block->ipa & 0x00ff];
-	if (handler)
-		return handler(vcpu);
-	return -EOPNOTSUPP;
+	switch (vcpu->arch.sie_block->ipa & 0x00ff) {
+	case 0x04:
+		return handle_ptff(vcpu);
+	case 0x07:
+		return handle_sckpf(vcpu);
+	default:
+		return -EOPNOTSUPP;
+	}
 }
-- 
2.14.3

[PATCH 2/2] KVM: s390: use switch vs jump table in intercept.c

[Christian Borntraeger]

instead of having huge jump tables for function selection,
lets use normal switch/case statements for the instruction
handlers in intercept.c We can now also get rid of
intercept_handler_t.

bloat-o-meter output:
add/remove: 0/1 grow/shrink: 1/0 up/down: 280/-2048 (-1768)
Function                                     old     new   delta
kvm_handle_sie_intercept                    1530    1810    +280
instruction_handlers                        2048       -   -2048
Total: Before=5227, After=3459, chg -33.82%

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/intercept.c | 54 ++++++++++++++++++++++++-----------------------
 arch/s390/kvm/kvm-s390.h  |  2 --
 2 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
index 9c7d70715862..047e711d1fd1 100644
--- a/arch/s390/kvm/intercept.c
+++ b/arch/s390/kvm/intercept.c
@@ -22,22 +22,6 @@
 #include "trace.h"
 #include "trace-s390.h"
 
-
-static const intercept_handler_t instruction_handlers[256] = {
-	[0x01] = kvm_s390_handle_01,
-	[0x82] = kvm_s390_handle_lpsw,
-	[0x83] = kvm_s390_handle_diag,
-	[0xaa] = kvm_s390_handle_aa,
-	[0xae] = kvm_s390_handle_sigp,
-	[0xb2] = kvm_s390_handle_b2,
-	[0xb6] = kvm_s390_handle_stctl,
-	[0xb7] = kvm_s390_handle_lctl,
-	[0xb9] = kvm_s390_handle_b9,
-	[0xe3] = kvm_s390_handle_e3,
-	[0xe5] = kvm_s390_handle_e5,
-	[0xeb] = kvm_s390_handle_eb,
-};
-
 u8 kvm_s390_get_ilen(struct kvm_vcpu *vcpu)
 {
 	struct kvm_s390_sie_block *sie_block = vcpu->arch.sie_block;
@@ -129,16 +113,34 @@ static int handle_validity(struct kvm_vcpu *vcpu)
 
 static int handle_instruction(struct kvm_vcpu *vcpu)
 {
-	intercept_handler_t handler;
-
-	vcpu->stat.exit_instruction++;
-	trace_kvm_s390_intercept_instruction(vcpu,
-					     vcpu->arch.sie_block->ipa,
-					     vcpu->arch.sie_block->ipb);
-	handler = instruction_handlers[vcpu->arch.sie_block->ipa >> 8];
-	if (handler)
-		return handler(vcpu);
-	return -EOPNOTSUPP;
+	switch (vcpu->arch.sie_block->ipa >> 8) {
+	case 0x01:
+		return kvm_s390_handle_01(vcpu);
+	case 0x82:
+		return kvm_s390_handle_lpsw(vcpu);
+	case 0x83:
+		return kvm_s390_handle_diag(vcpu);
+	case 0xaa:
+		return kvm_s390_handle_aa(vcpu);
+	case 0xae:
+		return kvm_s390_handle_sigp(vcpu);
+	case 0xb2:
+		return kvm_s390_handle_b2(vcpu);
+	case 0xb6:
+		return kvm_s390_handle_stctl(vcpu);
+	case 0xb7:
+		return kvm_s390_handle_lctl(vcpu);
+	case 0xb9:
+		return kvm_s390_handle_b9(vcpu);
+	case 0xe3:
+		return kvm_s390_handle_e3(vcpu);
+	case 0xe5:
+		return kvm_s390_handle_e5(vcpu);
+	case 0xeb:
+		return kvm_s390_handle_eb(vcpu);
+	default:
+		return -EOPNOTSUPP;
+	}
 }
 
 static int inject_prog_on_prog_intercept(struct kvm_vcpu *vcpu)
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index bd31b37b0e6f..3c0a975c2477 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -19,8 +19,6 @@
 #include <asm/processor.h>
 #include <asm/sclp.h>
 
-typedef int (*intercept_handler_t)(struct kvm_vcpu *vcpu);
-
 /* Transactional Memory Execution related macros */
 #define IS_TE_ENABLED(vcpu)	((vcpu->arch.sie_block->ecb & ECB_TE))
 #define TDB_FORMAT1		1
-- 
2.14.3

Re: [PATCH 1/2] KVM: s390: use switch vs jump table in priv.c

[Cornelia Huck]

On Tue,  6 Feb 2018 11:21:26 +0000
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> instead of having huge jump tables for function selection,

s/instead/Instead/

> lets use normal switch/case statements for the instruction

s/lets/let's/

> handlers in priv.c
> 
> bloat-o-meter shows that the saving are even bigger than
> just the removed jump tables.
> 
> add/remove: 0/11 grow/shrink: 8/0 up/down: 1934/-10246 (-8312)
> Function                                     old     new   delta
> kvm_s390_handle_b2                            42     958    +916
> handle_iske                                  178     558    +380
> handle_rrbe                                  178     546    +368
> kvm_s390_handle_b9                            42     222    +180
> kvm_s390_handle_01                            42      74     +32
> kvm_s390_handle_eb                            42      70     +28
> handle_sckpf                                 176     204     +28
> handle_lctlg                                 628     630      +2
> handle_ptff                                   36       -     -36
> handle_sckpf.part                             78       -     -78
> handle_epsw                                  154       -    -154
> handle_stfl                                  316       -    -316
> handle_rrbe.part                             470       -    -470
> handle_iske.part                             482       -    -482
> handle_io_inst                               518       -    -518
> x01_handlers                                2048       -   -2048
> eb_handlers                                 2048       -   -2048
> b9_handlers                                 2048       -   -2048
> b2_handlers                                 2048       -   -2048

Neat.

> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/kvm/priv.c | 183 +++++++++++++++++++++++++--------------------------
>  1 file changed, 91 insertions(+), 92 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH 2/2] KVM: s390: use switch vs jump table in intercept.c

[Cornelia Huck]

On Tue,  6 Feb 2018 11:21:27 +0000
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> instead of having huge jump tables for function selection,

s/instead/Instead/

> lets use normal switch/case statements for the instruction

s/lets/let's/

> handlers in intercept.c We can now also get rid of
> intercept_handler_t.
> 
> bloat-o-meter output:
> add/remove: 0/1 grow/shrink: 1/0 up/down: 280/-2048 (-1768)
> Function                                     old     new   delta
> kvm_handle_sie_intercept                    1530    1810    +280
> instruction_handlers                        2048       -   -2048
> Total: Before=5227, After=3459, chg -33.82%
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/kvm/intercept.c | 54 ++++++++++++++++++++++++-----------------------
>  arch/s390/kvm/kvm-s390.h  |  2 --
>  2 files changed, 28 insertions(+), 28 deletions(-)
> 

> @@ -129,16 +113,34 @@ static int handle_validity(struct kvm_vcpu *vcpu)
>  
>  static int handle_instruction(struct kvm_vcpu *vcpu)
>  {
> -	intercept_handler_t handler;
> -
> -	vcpu->stat.exit_instruction++;
> -	trace_kvm_s390_intercept_instruction(vcpu,
> -					     vcpu->arch.sie_block->ipa,
> -					     vcpu->arch.sie_block->ipb);

Is dropping the tracing intentional?

> -	handler = instruction_handlers[vcpu->arch.sie_block->ipa >> 8];
> -	if (handler)
> -		return handler(vcpu);
> -	return -EOPNOTSUPP;
> +	switch (vcpu->arch.sie_block->ipa >> 8) {
> +	case 0x01:
> +		return kvm_s390_handle_01(vcpu);
> +	case 0x82:
> +		return kvm_s390_handle_lpsw(vcpu);
> +	case 0x83:
> +		return kvm_s390_handle_diag(vcpu);
> +	case 0xaa:
> +		return kvm_s390_handle_aa(vcpu);
> +	case 0xae:
> +		return kvm_s390_handle_sigp(vcpu);
> +	case 0xb2:
> +		return kvm_s390_handle_b2(vcpu);
> +	case 0xb6:
> +		return kvm_s390_handle_stctl(vcpu);
> +	case 0xb7:
> +		return kvm_s390_handle_lctl(vcpu);
> +	case 0xb9:
> +		return kvm_s390_handle_b9(vcpu);
> +	case 0xe3:
> +		return kvm_s390_handle_e3(vcpu);
> +	case 0xe5:
> +		return kvm_s390_handle_e5(vcpu);
> +	case 0xeb:
> +		return kvm_s390_handle_eb(vcpu);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
>  }
>  

Else, looks good.

Re: [PATCH 2/2] KVM: s390: use switch vs jump table in intercept.c

[Christian Borntraeger]

On 02/06/2018 01:04 PM, Cornelia Huck wrote:
> On Tue,  6 Feb 2018 11:21:27 +0000
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> instead of having huge jump tables for function selection,
> 
> s/instead/Instead/
> 
>> lets use normal switch/case statements for the instruction
> 
> s/lets/let's/
> 
>> handlers in intercept.c We can now also get rid of
>> intercept_handler_t.
>>
>> bloat-o-meter output:
>> add/remove: 0/1 grow/shrink: 1/0 up/down: 280/-2048 (-1768)
>> Function                                     old     new   delta
>> kvm_handle_sie_intercept                    1530    1810    +280
>> instruction_handlers                        2048       -   -2048
>> Total: Before=5227, After=3459, chg -33.82%
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  arch/s390/kvm/intercept.c | 54 ++++++++++++++++++++++++-----------------------
>>  arch/s390/kvm/kvm-s390.h  |  2 --
>>  2 files changed, 28 insertions(+), 28 deletions(-)
>>
> 
>> @@ -129,16 +113,34 @@ static int handle_validity(struct kvm_vcpu *vcpu)
>>  
>>  static int handle_instruction(struct kvm_vcpu *vcpu)
>>  {
>> -	intercept_handler_t handler;
>> -
>> -	vcpu->stat.exit_instruction++;
>> -	trace_kvm_s390_intercept_instruction(vcpu,
>> -					     vcpu->arch.sie_block->ipa,
>> -					     vcpu->arch.sie_block->ipb);
> 
> Is dropping the tracing intentional?

nope, its a bug :-)

> 
>> -	handler = instruction_handlers[vcpu->arch.sie_block->ipa >> 8];
>> -	if (handler)
>> -		return handler(vcpu);
>> -	return -EOPNOTSUPP;
>> +	switch (vcpu->arch.sie_block->ipa >> 8) {
>> +	case 0x01:
>> +		return kvm_s390_handle_01(vcpu);
>> +	case 0x82:
>> +		return kvm_s390_handle_lpsw(vcpu);
>> +	case 0x83:
>> +		return kvm_s390_handle_diag(vcpu);
>> +	case 0xaa:
>> +		return kvm_s390_handle_aa(vcpu);
>> +	case 0xae:
>> +		return kvm_s390_handle_sigp(vcpu);
>> +	case 0xb2:
>> +		return kvm_s390_handle_b2(vcpu);
>> +	case 0xb6:
>> +		return kvm_s390_handle_stctl(vcpu);
>> +	case 0xb7:
>> +		return kvm_s390_handle_lctl(vcpu);
>> +	case 0xb9:
>> +		return kvm_s390_handle_b9(vcpu);
>> +	case 0xe3:
>> +		return kvm_s390_handle_e3(vcpu);
>> +	case 0xe5:
>> +		return kvm_s390_handle_e5(vcpu);
>> +	case 0xeb:
>> +		return kvm_s390_handle_eb(vcpu);
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>>  }
>>  
> 
> Else, looks good.
> 

Re: [PATCH 0/2] KVM: s390: avoid jump tables

[David Hildenbrand]

On 06.02.2018 12:21, Christian Borntraeger wrote:
> Some old patches refreshed.
> 

Certainly the right thing to do. Especially also interesting due to
retpotline (if we get something like that on s390x). If I remember
correctly, x86 highly benefits by replacing magic function pointer by
switch statements.

Should we go ahead and also rework interrupt delivery?

> Christian Borntraeger (2):
>   KVM: s390: use switch vs jump table in priv.c
>   KVM: s390: use switch vs jump table in intercept.c
> 
>  arch/s390/kvm/intercept.c |  54 +++++++-------
>  arch/s390/kvm/kvm-s390.h  |   2 -
>  arch/s390/kvm/priv.c      | 183 +++++++++++++++++++++++-----------------------
>  3 files changed, 119 insertions(+), 120 deletions(-)
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH 1/2] KVM: s390: use switch vs jump table in priv.c

[David Hildenbrand]

On 06.02.2018 12:21, Christian Borntraeger wrote:
> instead of having huge jump tables for function selection,
> lets use normal switch/case statements for the instruction
> handlers in priv.c
> 
> bloat-o-meter shows that the saving are even bigger than
> just the removed jump tables.
> 
> add/remove: 0/11 grow/shrink: 8/0 up/down: 1934/-10246 (-8312)
> Function                                     old     new   delta
> kvm_s390_handle_b2                            42     958    +916
> handle_iske                                  178     558    +380
> handle_rrbe                                  178     546    +368
> kvm_s390_handle_b9                            42     222    +180
> kvm_s390_handle_01                            42      74     +32
> kvm_s390_handle_eb                            42      70     +28
> handle_sckpf                                 176     204     +28
> handle_lctlg                                 628     630      +2
> handle_ptff                                   36       -     -36
> handle_sckpf.part                             78       -     -78
> handle_epsw                                  154       -    -154
> handle_stfl                                  316       -    -316
> handle_rrbe.part                             470       -    -470
> handle_iske.part                             482       -    -482
> handle_io_inst                               518       -    -518
> x01_handlers                                2048       -   -2048
> eb_handlers                                 2048       -   -2048
> b9_handlers                                 2048       -   -2048
> b2_handlers                                 2048       -   -2048
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/kvm/priv.c | 183 +++++++++++++++++++++++++--------------------------
>  1 file changed, 91 insertions(+), 92 deletions(-)
> 
> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index c4c4e157c036..a74578cdd3f3 100644
> --- a/arch/s390/kvm/priv.c
> +++ b/arch/s390/kvm/priv.c
> @@ -795,55 +795,60 @@ static int handle_stsi(struct kvm_vcpu *vcpu)
>  	return rc;
>  }
>  
> -static const intercept_handler_t b2_handlers[256] = {
> -	[0x02] = handle_stidp,
> -	[0x04] = handle_set_clock,
> -	[0x10] = handle_set_prefix,
> -	[0x11] = handle_store_prefix,
> -	[0x12] = handle_store_cpu_address,
> -	[0x14] = kvm_s390_handle_vsie,
> -	[0x21] = handle_ipte_interlock,
> -	[0x29] = handle_iske,
> -	[0x2a] = handle_rrbe,
> -	[0x2b] = handle_sske,
> -	[0x2c] = handle_test_block,
> -	[0x30] = handle_io_inst,
> -	[0x31] = handle_io_inst,
> -	[0x32] = handle_io_inst,
> -	[0x33] = handle_io_inst,
> -	[0x34] = handle_io_inst,
> -	[0x35] = handle_io_inst,
> -	[0x36] = handle_io_inst,
> -	[0x37] = handle_io_inst,
> -	[0x38] = handle_io_inst,
> -	[0x39] = handle_io_inst,
> -	[0x3a] = handle_io_inst,
> -	[0x3b] = handle_io_inst,
> -	[0x3c] = handle_io_inst,
> -	[0x50] = handle_ipte_interlock,
> -	[0x56] = handle_sthyi,
> -	[0x5f] = handle_io_inst,
> -	[0x74] = handle_io_inst,
> -	[0x76] = handle_io_inst,
> -	[0x7d] = handle_stsi,
> -	[0xb1] = handle_stfl,
> -	[0xb2] = handle_lpswe,
> -};
> -
>  int kvm_s390_handle_b2(struct kvm_vcpu *vcpu)
>  {
> -	intercept_handler_t handler;
> -
> -	/*
> -	 * A lot of B2 instructions are priviledged. Here we check for
> -	 * the privileged ones, that we can handle in the kernel.
> -	 * Anything else goes to userspace.
> -	 */
> -	handler = b2_handlers[vcpu->arch.sie_block->ipa & 0x00ff];
> -	if (handler)
> -		return handler(vcpu);
> -
> -	return -EOPNOTSUPP;
> +	switch (vcpu->arch.sie_block->ipa & 0x00ff) {
> +	case 0x02:
> +		return handle_stidp(vcpu);
> +	case 0x04:
> +		return handle_set_clock(vcpu);
> +	case 0x10:
> +		return handle_set_prefix(vcpu);
> +	case 0x11:
> +		return handle_store_prefix(vcpu);
> +	case 0x12:
> +		return handle_store_cpu_address(vcpu);
> +	case 0x14:
> +		return kvm_s390_handle_vsie(vcpu);
> +	case 0x21:
> +	case 0x50:
> +		return handle_ipte_interlock(vcpu);
> +	case 0x29:
> +		return handle_iske(vcpu);
> +	case 0x2a:
> +		return handle_rrbe(vcpu);
> +	case 0x2b:
> +		return handle_sske(vcpu);
> +	case 0x2c:
> +		return handle_test_block(vcpu);
> +	case 0x30:
> +	case 0x31:
> +	case 0x32:
> +	case 0x33:
> +	case 0x34:
> +	case 0x35:
> +	case 0x36:
> +	case 0x37:
> +	case 0x38:
> +	case 0x39:
> +	case 0x3a:
> +	case 0x3b:
> +	case 0x3c:
> +	case 0x5f:
> +	case 0x74:
> +	case 0x76:
> +		return handle_io_inst(vcpu);
> +	case 0x56:
> +		return handle_sthyi(vcpu);
> +	case 0x7d:
> +		return handle_stsi(vcpu);
> +	case 0xb1:
> +		return handle_stfl(vcpu);
> +	case 0xb2:
> +		return handle_lpswe(vcpu);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
>  }
>  
>  static int handle_epsw(struct kvm_vcpu *vcpu)
> @@ -1105,25 +1110,22 @@ static int handle_essa(struct kvm_vcpu *vcpu)
>  	return 0;
>  }
>  
> -static const intercept_handler_t b9_handlers[256] = {
> -	[0x8a] = handle_ipte_interlock,
> -	[0x8d] = handle_epsw,
> -	[0x8e] = handle_ipte_interlock,
> -	[0x8f] = handle_ipte_interlock,
> -	[0xab] = handle_essa,
> -	[0xaf] = handle_pfmf,
> -};
> -
>  int kvm_s390_handle_b9(struct kvm_vcpu *vcpu)
>  {
> -	intercept_handler_t handler;
> -
> -	/* This is handled just as for the B2 instructions. */
> -	handler = b9_handlers[vcpu->arch.sie_block->ipa & 0x00ff];
> -	if (handler)
> -		return handler(vcpu);
> -
> -	return -EOPNOTSUPP;
> +	switch (vcpu->arch.sie_block->ipa & 0x00ff) {
> +	case 0x8a:
> +	case 0x8e:
> +	case 0x8f:
> +		return handle_ipte_interlock(vcpu);
> +	case 0x8d:
> +		return handle_epsw(vcpu);
> +	case 0xab:
> +		return handle_essa(vcpu);
> +	case 0xaf:
> +		return handle_pfmf(vcpu);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
>  }
>  
>  int kvm_s390_handle_lctl(struct kvm_vcpu *vcpu)
> @@ -1271,22 +1273,20 @@ static int handle_stctg(struct kvm_vcpu *vcpu)
>  	return rc ? kvm_s390_inject_prog_cond(vcpu, rc) : 0;
>  }
>  
> -static const intercept_handler_t eb_handlers[256] = {
> -	[0x2f] = handle_lctlg,
> -	[0x25] = handle_stctg,
> -	[0x60] = handle_ri,
> -	[0x61] = handle_ri,
> -	[0x62] = handle_ri,
> -};
> -
>  int kvm_s390_handle_eb(struct kvm_vcpu *vcpu)
>  {
> -	intercept_handler_t handler;
> -
> -	handler = eb_handlers[vcpu->arch.sie_block->ipb & 0xff];
> -	if (handler)
> -		return handler(vcpu);
> -	return -EOPNOTSUPP;
> +	switch (vcpu->arch.sie_block->ipb & 0x000000ff) {
> +	case 0x25:
> +		return handle_stctg(vcpu);
> +	case 0x2f:
> +		return handle_lctlg(vcpu);
> +	case 0x60:
> +	case 0x61:
> +	case 0x62:
> +		return handle_ri(vcpu);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
>  }
>  
>  static int handle_tprot(struct kvm_vcpu *vcpu)
> @@ -1346,10 +1346,12 @@ static int handle_tprot(struct kvm_vcpu *vcpu)
>  
>  int kvm_s390_handle_e5(struct kvm_vcpu *vcpu)
>  {
> -	/* For e5xx... instructions we only handle TPROT */
> -	if ((vcpu->arch.sie_block->ipa & 0x00ff) == 0x01)
> +	switch (vcpu->arch.sie_block->ipa & 0x00ff) {
> +	case 0x01:
>  		return handle_tprot(vcpu);
> -	return -EOPNOTSUPP;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
>  }
>  
>  static int handle_sckpf(struct kvm_vcpu *vcpu)
> @@ -1380,17 +1382,14 @@ static int handle_ptff(struct kvm_vcpu *vcpu)
>  	return 0;
>  }
>  
> -static const intercept_handler_t x01_handlers[256] = {
> -	[0x04] = handle_ptff,
> -	[0x07] = handle_sckpf,
> -};
> -
>  int kvm_s390_handle_01(struct kvm_vcpu *vcpu)
>  {
> -	intercept_handler_t handler;
> -
> -	handler = x01_handlers[vcpu->arch.sie_block->ipa & 0x00ff];
> -	if (handler)
> -		return handler(vcpu);
> -	return -EOPNOTSUPP;
> +	switch (vcpu->arch.sie_block->ipa & 0x00ff) {
> +	case 0x04:
> +		return handle_ptff(vcpu);
> +	case 0x07:
> +		return handle_sckpf(vcpu);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
>  }
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [PATCH 2/2] KVM: s390: use switch vs jump table in intercept.c

[David Hildenbrand]

On 06.02.2018 12:21, Christian Borntraeger wrote:
> instead of having huge jump tables for function selection,
> lets use normal switch/case statements for the instruction
> handlers in intercept.c We can now also get rid of
> intercept_handler_t.
> 
> bloat-o-meter output:
> add/remove: 0/1 grow/shrink: 1/0 up/down: 280/-2048 (-1768)
> Function                                     old     new   delta
> kvm_handle_sie_intercept                    1530    1810    +280
> instruction_handlers                        2048       -   -2048
> Total: Before=5227, After=3459, chg -33.82%
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/kvm/intercept.c | 54 ++++++++++++++++++++++++-----------------------
>  arch/s390/kvm/kvm-s390.h  |  2 --
>  2 files changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
> index 9c7d70715862..047e711d1fd1 100644
> --- a/arch/s390/kvm/intercept.c
> +++ b/arch/s390/kvm/intercept.c
> @@ -22,22 +22,6 @@
>  #include "trace.h"
>  #include "trace-s390.h"
>  
> -
> -static const intercept_handler_t instruction_handlers[256] = {
> -	[0x01] = kvm_s390_handle_01,
> -	[0x82] = kvm_s390_handle_lpsw,
> -	[0x83] = kvm_s390_handle_diag,
> -	[0xaa] = kvm_s390_handle_aa,
> -	[0xae] = kvm_s390_handle_sigp,
> -	[0xb2] = kvm_s390_handle_b2,
> -	[0xb6] = kvm_s390_handle_stctl,
> -	[0xb7] = kvm_s390_handle_lctl,
> -	[0xb9] = kvm_s390_handle_b9,
> -	[0xe3] = kvm_s390_handle_e3,
> -	[0xe5] = kvm_s390_handle_e5,
> -	[0xeb] = kvm_s390_handle_eb,
> -};
> -
>  u8 kvm_s390_get_ilen(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_s390_sie_block *sie_block = vcpu->arch.sie_block;
> @@ -129,16 +113,34 @@ static int handle_validity(struct kvm_vcpu *vcpu)
>  
>  static int handle_instruction(struct kvm_vcpu *vcpu)
>  {
> -	intercept_handler_t handler;
> -
> -	vcpu->stat.exit_instruction++;
> -	trace_kvm_s390_intercept_instruction(vcpu,
> -					     vcpu->arch.sie_block->ipa,
> -					     vcpu->arch.sie_block->ipb);

With that fixed

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 

Thanks,

David / dhildenb

Re: [PATCH 0/2] KVM: s390: avoid jump tables

[Christian Borntraeger]

On 02/06/2018 01:30 PM, David Hildenbrand wrote:
> On 06.02.2018 12:21, Christian Borntraeger wrote:
>> Some old patches refreshed.
>>
> 
> Certainly the right thing to do. Especially also interesting due to
> retpotline (if we get something like that on s390x). If I remember
> correctly, x86 highly benefits by replacing magic function pointer by
> switch statements.
> 
> Should we go ahead and also rework interrupt delivery?

Yes, that was my plan but I have not started. Do you want to work on that?

> 
>> Christian Borntraeger (2):
>>   KVM: s390: use switch vs jump table in priv.c
>>   KVM: s390: use switch vs jump table in intercept.c
>>
>>  arch/s390/kvm/intercept.c |  54 +++++++-------
>>  arch/s390/kvm/kvm-s390.h  |   2 -
>>  arch/s390/kvm/priv.c      | 183 +++++++++++++++++++++++-----------------------
>>  3 files changed, 119 insertions(+), 120 deletions(-)
>>
> 
> 

Re: [PATCH 0/2] KVM: s390: avoid jump tables

[David Hildenbrand]

On 06.02.2018 13:36, Christian Borntraeger wrote:
> 
> 
> On 02/06/2018 01:30 PM, David Hildenbrand wrote:
>> On 06.02.2018 12:21, Christian Borntraeger wrote:
>>> Some old patches refreshed.
>>>
>>
>> Certainly the right thing to do. Especially also interesting due to
>> retpotline (if we get something like that on s390x). If I remember
>> correctly, x86 highly benefits by replacing magic function pointer by
>> switch statements.
>>
>> Should we go ahead and also rework interrupt delivery?
> 
> Yes, that was my plan but I have not started. Do you want to work on that?

Can do if you don't have time for it!


-- 

Thanks,

David / dhildenb

[PATCH v2 2/2] KVM: s390: use switch vs jump table in intercept.c

[Christian Borntraeger]

Instead of having huge jump tables for function selection,
let's use normal switch/case statements for the instruction
handlers in intercept.c We can now also get rid of
intercept_handler_t.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 arch/s390/kvm/intercept.c | 51 +++++++++++++++++++++++++++--------------------
 arch/s390/kvm/kvm-s390.h  |  2 --
 2 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
index 9c7d70715862..07c6e81163bf 100644
--- a/arch/s390/kvm/intercept.c
+++ b/arch/s390/kvm/intercept.c
@@ -22,22 +22,6 @@
 #include "trace.h"
 #include "trace-s390.h"
 
-
-static const intercept_handler_t instruction_handlers[256] = {
-	[0x01] = kvm_s390_handle_01,
-	[0x82] = kvm_s390_handle_lpsw,
-	[0x83] = kvm_s390_handle_diag,
-	[0xaa] = kvm_s390_handle_aa,
-	[0xae] = kvm_s390_handle_sigp,
-	[0xb2] = kvm_s390_handle_b2,
-	[0xb6] = kvm_s390_handle_stctl,
-	[0xb7] = kvm_s390_handle_lctl,
-	[0xb9] = kvm_s390_handle_b9,
-	[0xe3] = kvm_s390_handle_e3,
-	[0xe5] = kvm_s390_handle_e5,
-	[0xeb] = kvm_s390_handle_eb,
-};
-
 u8 kvm_s390_get_ilen(struct kvm_vcpu *vcpu)
 {
 	struct kvm_s390_sie_block *sie_block = vcpu->arch.sie_block;
@@ -129,16 +113,39 @@ static int handle_validity(struct kvm_vcpu *vcpu)
 
 static int handle_instruction(struct kvm_vcpu *vcpu)
 {
-	intercept_handler_t handler;
-
 	vcpu->stat.exit_instruction++;
 	trace_kvm_s390_intercept_instruction(vcpu,
 					     vcpu->arch.sie_block->ipa,
 					     vcpu->arch.sie_block->ipb);
-	handler = instruction_handlers[vcpu->arch.sie_block->ipa >> 8];
-	if (handler)
-		return handler(vcpu);
-	return -EOPNOTSUPP;
+
+	switch (vcpu->arch.sie_block->ipa >> 8) {
+	case 0x01:
+		return kvm_s390_handle_01(vcpu);
+	case 0x82:
+		return kvm_s390_handle_lpsw(vcpu);
+	case 0x83:
+		return kvm_s390_handle_diag(vcpu);
+	case 0xaa:
+		return kvm_s390_handle_aa(vcpu);
+	case 0xae:
+		return kvm_s390_handle_sigp(vcpu);
+	case 0xb2:
+		return kvm_s390_handle_b2(vcpu);
+	case 0xb6:
+		return kvm_s390_handle_stctl(vcpu);
+	case 0xb7:
+		return kvm_s390_handle_lctl(vcpu);
+	case 0xb9:
+		return kvm_s390_handle_b9(vcpu);
+	case 0xe3:
+		return kvm_s390_handle_e3(vcpu);
+	case 0xe5:
+		return kvm_s390_handle_e5(vcpu);
+	case 0xeb:
+		return kvm_s390_handle_eb(vcpu);
+	default:
+		return -EOPNOTSUPP;
+	}
 }
 
 static int inject_prog_on_prog_intercept(struct kvm_vcpu *vcpu)
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index bd31b37b0e6f..3c0a975c2477 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -19,8 +19,6 @@
 #include <asm/processor.h>
 #include <asm/sclp.h>
 
-typedef int (*intercept_handler_t)(struct kvm_vcpu *vcpu);
-
 /* Transactional Memory Execution related macros */
 #define IS_TE_ENABLED(vcpu)	((vcpu->arch.sie_block->ecb & ECB_TE))
 #define TDB_FORMAT1		1
-- 
2.14.3

Re: [PATCH v2 2/2] KVM: s390: use switch vs jump table in intercept.c

[Cornelia Huck]

On Tue,  6 Feb 2018 12:47:00 +0000
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> Instead of having huge jump tables for function selection,
> let's use normal switch/case statements for the instruction
> handlers in intercept.c We can now also get rid of
> intercept_handler_t.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
>  arch/s390/kvm/intercept.c | 51 +++++++++++++++++++++++++++--------------------
>  arch/s390/kvm/kvm-s390.h  |  2 --
>  2 files changed, 29 insertions(+), 24 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH 1/2] KVM: s390: use switch vs jump table in priv.c

[Heiko Carstens]

On Tue, Feb 06, 2018 at 11:21:26AM +0000, Christian Borntraeger wrote:
> instead of having huge jump tables for function selection,
> lets use normal switch/case statements for the instruction
> handlers in priv.c
> 
> bloat-o-meter shows that the saving are even bigger than
> just the removed jump tables.

Which is not true since bloat-o-meter does not take the rodata section into
account, which grows by 1720 bytes with this commit.

> add/remove: 0/11 grow/shrink: 8/0 up/down: 1934/-10246 (-8312)
> Function                                     old     new   delta
> kvm_s390_handle_b2                            42     958    +916
> handle_iske                                  178     558    +380
> handle_rrbe                                  178     546    +368
> kvm_s390_handle_b9                            42     222    +180
> kvm_s390_handle_01                            42      74     +32
> kvm_s390_handle_eb                            42      70     +28
> handle_sckpf                                 176     204     +28
> handle_lctlg                                 628     630      +2
> handle_ptff                                   36       -     -36
> handle_sckpf.part                             78       -     -78
> handle_epsw                                  154       -    -154
> handle_stfl                                  316       -    -316
> handle_rrbe.part                             470       -    -470
> handle_iske.part                             482       -    -482
> handle_io_inst                               518       -    -518
> x01_handlers                                2048       -   -2048
> eb_handlers                                 2048       -   -2048
> b9_handlers                                 2048       -   -2048
> b2_handlers                                 2048       -   -2048
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

Re: [PATCH v2 2/2] KVM: s390: use switch vs jump table in intercept.c

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 3408 bytes --]

On 06.02.2018 13:47, Christian Borntraeger wrote:
> Instead of having huge jump tables for function selection,
> let's use normal switch/case statements for the instruction
> handlers in intercept.c We can now also get rid of
> intercept_handler_t.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>

Looks good to me and results in less scrolling to find the handlers.

Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>


> ---
>  arch/s390/kvm/intercept.c | 51 +++++++++++++++++++++++++++--------------------
>  arch/s390/kvm/kvm-s390.h  |  2 --
>  2 files changed, 29 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
> index 9c7d70715862..07c6e81163bf 100644
> --- a/arch/s390/kvm/intercept.c
> +++ b/arch/s390/kvm/intercept.c
> @@ -22,22 +22,6 @@
>  #include "trace.h"
>  #include "trace-s390.h"
> 
> -
> -static const intercept_handler_t instruction_handlers[256] = {
> -	[0x01] = kvm_s390_handle_01,
> -	[0x82] = kvm_s390_handle_lpsw,
> -	[0x83] = kvm_s390_handle_diag,
> -	[0xaa] = kvm_s390_handle_aa,
> -	[0xae] = kvm_s390_handle_sigp,
> -	[0xb2] = kvm_s390_handle_b2,
> -	[0xb6] = kvm_s390_handle_stctl,
> -	[0xb7] = kvm_s390_handle_lctl,
> -	[0xb9] = kvm_s390_handle_b9,
> -	[0xe3] = kvm_s390_handle_e3,
> -	[0xe5] = kvm_s390_handle_e5,
> -	[0xeb] = kvm_s390_handle_eb,
> -};
> -
>  u8 kvm_s390_get_ilen(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_s390_sie_block *sie_block = vcpu->arch.sie_block;
> @@ -129,16 +113,39 @@ static int handle_validity(struct kvm_vcpu *vcpu)
> 
>  static int handle_instruction(struct kvm_vcpu *vcpu)
>  {
> -	intercept_handler_t handler;
> -
>  	vcpu->stat.exit_instruction++;
>  	trace_kvm_s390_intercept_instruction(vcpu,
>  					     vcpu->arch.sie_block->ipa,
>  					     vcpu->arch.sie_block->ipb);
> -	handler = instruction_handlers[vcpu->arch.sie_block->ipa >> 8];
> -	if (handler)
> -		return handler(vcpu);
> -	return -EOPNOTSUPP;
> +
> +	switch (vcpu->arch.sie_block->ipa >> 8) {
> +	case 0x01:
> +		return kvm_s390_handle_01(vcpu);
> +	case 0x82:
> +		return kvm_s390_handle_lpsw(vcpu);
> +	case 0x83:
> +		return kvm_s390_handle_diag(vcpu);
> +	case 0xaa:
> +		return kvm_s390_handle_aa(vcpu);
> +	case 0xae:
> +		return kvm_s390_handle_sigp(vcpu);
> +	case 0xb2:
> +		return kvm_s390_handle_b2(vcpu);
> +	case 0xb6:
> +		return kvm_s390_handle_stctl(vcpu);
> +	case 0xb7:
> +		return kvm_s390_handle_lctl(vcpu);
> +	case 0xb9:
> +		return kvm_s390_handle_b9(vcpu);
> +	case 0xe3:
> +		return kvm_s390_handle_e3(vcpu);
> +	case 0xe5:
> +		return kvm_s390_handle_e5(vcpu);
> +	case 0xeb:
> +		return kvm_s390_handle_eb(vcpu);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
>  }
> 
>  static int inject_prog_on_prog_intercept(struct kvm_vcpu *vcpu)
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index bd31b37b0e6f..3c0a975c2477 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -19,8 +19,6 @@
>  #include <asm/processor.h>
>  #include <asm/sclp.h>
> 
> -typedef int (*intercept_handler_t)(struct kvm_vcpu *vcpu);
> -
>  /* Transactional Memory Execution related macros */
>  #define IS_TE_ENABLED(vcpu)	((vcpu->arch.sie_block->ecb & ECB_TE))
>  #define TDB_FORMAT1		1
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/2] KVM: s390: avoid jump tables

[Heiko Carstens]

On Tue, Feb 06, 2018 at 01:30:28PM +0100, David Hildenbrand wrote:
> On 06.02.2018 12:21, Christian Borntraeger wrote:
> > Some old patches refreshed.
> > 
> 
> Certainly the right thing to do. Especially also interesting due to
> retpotline (if we get something like that on s390x). If I remember
> correctly, x86 highly benefits by replacing magic function pointer by
> switch statements.

If you look at the generated code for the first patch: gcc now generates
its own jump table which then jumps (indirectly) to a brasl... So it's two
instead of one branch.
I'm not saying that this patch is not good, but there seem be a wrong
assumptions about the benefit here.

Re: [PATCH 0/2] KVM: s390: avoid jump tables

[Christian Borntraeger]

On 02/08/2018 09:58 AM, Heiko Carstens wrote:
> On Tue, Feb 06, 2018 at 01:30:28PM +0100, David Hildenbrand wrote:
>> On 06.02.2018 12:21, Christian Borntraeger wrote:
>>> Some old patches refreshed.
>>>
>>
>> Certainly the right thing to do. Especially also interesting due to
>> retpotline (if we get something like that on s390x). If I remember
>> correctly, x86 highly benefits by replacing magic function pointer by
>> switch statements.
> 
> If you look at the generated code for the first patch: gcc now generates
> its own jump table which then jumps (indirectly) to a brasl... So it's two
> instead of one branch.
> I'm not saying that this patch is not good, but there seem be a wrong
> assumptions about the benefit here.

Seems to depend on the compiler. The gcc 7.2 from my Fedora 27 seems to do 
the right thing for intercept.o and priv.o.
In the end this will also help the new -mindirect-branch thing as the gcc
support also avoids jump tables if we use thunks.

Re: [PATCH 1/2] KVM: s390: use switch vs jump table in priv.c

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 2380 bytes --]

On 06.02.2018 12:21, Christian Borntraeger wrote:
> instead of having huge jump tables for function selection,
> lets use normal switch/case statements for the instruction
> handlers in priv.c
> 
> bloat-o-meter shows that the saving are even bigger than
> just the removed jump tables.
> 
> add/remove: 0/11 grow/shrink: 8/0 up/down: 1934/-10246 (-8312)
> Function                                     old     new   delta
> kvm_s390_handle_b2                            42     958    +916
> handle_iske                                  178     558    +380
> handle_rrbe                                  178     546    +368
> kvm_s390_handle_b9                            42     222    +180
> kvm_s390_handle_01                            42      74     +32
> kvm_s390_handle_eb                            42      70     +28
> handle_sckpf                                 176     204     +28
> handle_lctlg                                 628     630      +2
> handle_ptff                                   36       -     -36
> handle_sckpf.part                             78       -     -78
> handle_epsw                                  154       -    -154
> handle_stfl                                  316       -    -316
> handle_rrbe.part                             470       -    -470
> handle_iske.part                             482       -    -482
> handle_io_inst                               518       -    -518
> x01_handlers                                2048       -   -2048
> eb_handlers                                 2048       -   -2048
> b9_handlers                                 2048       -   -2048
> b2_handlers                                 2048       -   -2048
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>

> -
>  int kvm_s390_handle_b2(struct kvm_vcpu *vcpu)
>  {
> -	intercept_handler_t handler;
> -
> -	/*
> -	 * A lot of B2 instructions are priviledged. Here we check for
> -	 * the privileged ones, that we can handle in the kernel.
> -	 * Anything else goes to userspace.
> -	 */

Shall we move that to kvm_handle_sie_intercept in a more general way?
Something like:

Here we handle all privileged and performance critical
instructions/interceptions everything else gets handled by userspace via
EOPNOTSUPP.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/2] KVM: s390: avoid jump tables

[Christian Borntraeger]

On 02/08/2018 10:07 AM, Christian Borntraeger wrote:
> 
> 
> On 02/08/2018 09:58 AM, Heiko Carstens wrote:
>> On Tue, Feb 06, 2018 at 01:30:28PM +0100, David Hildenbrand wrote:
>>> On 06.02.2018 12:21, Christian Borntraeger wrote:
>>>> Some old patches refreshed.
>>>>
>>>
>>> Certainly the right thing to do. Especially also interesting due to
>>> retpotline (if we get something like that on s390x). If I remember
>>> correctly, x86 highly benefits by replacing magic function pointer by
>>> switch statements.
>>
>> If you look at the generated code for the first patch: gcc now generates
>> its own jump table which then jumps (indirectly) to a brasl... So it's two
>> instead of one branch.
>> I'm not saying that this patch is not good, but there seem be a wrong
>> assumptions about the benefit here.
> 
> Seems to depend on the compiler. The gcc 7.2 from my Fedora 27 seems to do 
> the right thing for intercept.o and priv.o.
> In the end this will also help the new -mindirect-branch thing as the gcc
> support also avoids jump tables if we use thunks.

I will update the patch description and focus more on the "let us use the compiler
heuristics" and "prepare for thunks".

Re: [PATCH 0/2] KVM: s390: avoid jump tables

[Christian Borntraeger]

On 02/08/2018 09:58 AM, Heiko Carstens wrote:

> If you look at the generated code for the first patch: gcc now generates
> its own jump table which then jumps (indirectly) to a brasl... So it's two
> instead of one branch.
> I'm not saying that this patch is not good, but there seem be a wrong
> assumptions about the benefit here.

I will now use the following patch description.



    KVM: s390: use switch vs jump table in intercept.c
    
    Instead of having huge jump tables for function selection,
    let's use normal switch/case statements for the instruction
    handlers in intercept.c We can now also get rid of
    intercept_handler_t.
    
    This allows the compiler to make the right decision depending
    on the situation (e.g. avoid jump-tables for thunks).
    
    Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
    Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
    Reviewed-by: David Hildenbrand <david@redhat.com>
    Reviewed-by: Cornelia Huck <cohuck@redhat.com>

commit cf392b582009f8ee4ef859f935dd9f67f855ccaf
Author:     Christian Borntraeger <borntraeger@de.ibm.com>
AuthorDate: Fri Apr 8 17:52:39 2016 +0200
Commit:     Christian Borntraeger <borntraeger@de.ibm.com>
CommitDate: Thu Feb 8 10:07:42 2018 +0000

    KVM: s390: use switch vs jump table in priv.c
    
    Instead of having huge jump tables for function selection,
    let's use normal switch/case statements for the instruction
    handlers in priv.c
    
    This allows the compiler to make the right decision depending
    on the situation (e.g. avoid jump-tables for thunks).
    
    Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
    Reviewed-by: Cornelia Huck <cohuck@redhat.com>
    Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
    Reviewed-by: David Hildenbrand <david@redhat.com>

Re: [PATCH 0/2] KVM: s390: avoid jump tables

[Cornelia Huck]

On Thu, 8 Feb 2018 11:09:15 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 02/08/2018 09:58 AM, Heiko Carstens wrote:
> 
> > If you look at the generated code for the first patch: gcc now generates
> > its own jump table which then jumps (indirectly) to a brasl... So it's two
> > instead of one branch.
> > I'm not saying that this patch is not good, but there seem be a wrong
> > assumptions about the benefit here.  
> 
> I will now use the following patch description.
> 
> 
> 
>     KVM: s390: use switch vs jump table in intercept.c
>     
>     Instead of having huge jump tables for function selection,
>     let's use normal switch/case statements for the instruction
>     handlers in intercept.c We can now also get rid of
>     intercept_handler_t.
>     
>     This allows the compiler to make the right decision depending
>     on the situation (e.g. avoid jump-tables for thunks).
>     
>     Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>     Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
>     Reviewed-by: David Hildenbrand <david@redhat.com>
>     Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 
> commit cf392b582009f8ee4ef859f935dd9f67f855ccaf
> Author:     Christian Borntraeger <borntraeger@de.ibm.com>
> AuthorDate: Fri Apr 8 17:52:39 2016 +0200
> Commit:     Christian Borntraeger <borntraeger@de.ibm.com>
> CommitDate: Thu Feb 8 10:07:42 2018 +0000
> 
>     KVM: s390: use switch vs jump table in priv.c
>     
>     Instead of having huge jump tables for function selection,
>     let's use normal switch/case statements for the instruction
>     handlers in priv.c
>     
>     This allows the compiler to make the right decision depending
>     on the situation (e.g. avoid jump-tables for thunks).
>     
>     Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>     Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>     Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
>     Reviewed-by: David Hildenbrand <david@redhat.com>
> 

Fine with me.