[v2,8/8] s390x: factor out common ioinst handler logic

Commit Message

Halil Pasic Oct. 4, 2017, 3:41 p.m. UTC

Some of ioinst the handlers look very much the same: they basically
delegate the work to the appropriate css function (doing some always the
same stuff before and after the call to the appropriate css function).
Let us create a template and get rid of some code.

Signed-off-by: Halil Pasic <[e-mail]>
Suggested-by: Marc Hartmayer <[e-mail]>
---
 target/s390x/ioinst.c | 59 ++++++++++++---------------------------------------
 1 file changed, 14 insertions(+), 45 deletions(-)

Comments

Cornelia Huck Oct. 10, 2017, 1:10 p.m. UTC | #1

On Wed,  4 Oct 2017 17:41:44 +0200
Halil Pasic <[e-mail]> wrote:

> Some of ioinst the handlers look very much the same: they basically
> delegate the work to the appropriate css function (doing some always the
> same stuff before and after the call to the appropriate css function).
> Let us create a template and get rid of some code.
> 
> Signed-off-by: Halil Pasic <[e-mail]>
> Suggested-by: Marc Hartmayer <[e-mail]>
> ---
>  target/s390x/ioinst.c | 59 ++++++++++++---------------------------------------
>  1 file changed, 14 insertions(+), 45 deletions(-)

Staring at this patch, I'm not sure I like it, although I can't quite
put a finger on *what* I don't like... maybe the whole 'template'
approach.

Halil Pasic Oct. 10, 2017, 2:37 p.m. UTC | #2

On 10/10/2017 03:10 PM, Cornelia Huck wrote:
> On Wed,  4 Oct 2017 17:41:44 +0200
> Halil Pasic <[e-mail]> wrote:
> 
>> Some of ioinst the handlers look very much the same: they basically
>> delegate the work to the appropriate css function (doing some always the
>> same stuff before and after the call to the appropriate css function).
>> Let us create a template and get rid of some code.
>>
>> Signed-off-by: Halil Pasic <[e-mail]>
>> Suggested-by: Marc Hartmayer <[e-mail]>
>> ---
>>  target/s390x/ioinst.c | 59 ++++++++++++---------------------------------------
>>  1 file changed, 14 insertions(+), 45 deletions(-)
> 
> Staring at this patch, I'm not sure I like it, although I can't quite
> put a finger on *what* I don't like... maybe the whole 'template'
> approach.
> 

Well that's why I was careful to make it a separate patch.
I'm also a bit ambivalent, and did not want to include it in
this series along the lines can be done later if somebody wants,
but Marc convinced me -- kind of. We can just forget about it.

Halil

Patch

diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
index 0c256baa70..9c7d6a222c 100644
--- a/target/s390x/ioinst.c
+++ b/target/s390x/ioinst.c
@@ -38,7 +38,10 @@  int ioinst_disassemble_sch_ident(uint32_t value, int *m, int *cssid, int *ssid,
     return 0;
 }
 
-void ioinst_handle_xsch(S390CPU *cpu, uint64_t reg1)
+/* many ionst handlers look the same: they just delegate to a some css func */
+static void ioinst_handler_template_sch(S390CPU *cpu, uint64_t reg1,
+                                      const char *iname,
+                                      IOInstEnding (*handler_css)(SubchDev *))
 {
     int cssid, ssid, schid, m;
     SubchDev *sch;
@@ -47,49 +50,28 @@  void ioinst_handle_xsch(S390CPU *cpu, uint64_t reg1)
         program_interrupt(&cpu->env, PGM_OPERAND, 4);
         return;
     }
-    trace_ioinst_sch_id("xsch", cssid, ssid, schid);
+    trace_ioinst_sch_id(iname, cssid, ssid, schid);
     sch = css_find_subch(m, cssid, ssid, schid);
     if (!sch || !css_subch_visible(sch)) {
         setcc(cpu, 3);
         return;
     }
-    setcc(cpu, css_do_xsch(sch).cc);
+    setcc(cpu, handler_css(sch).cc);
 }
 
-void ioinst_handle_csch(S390CPU *cpu, uint64_t reg1)
+void ioinst_handle_xsch(S390CPU *cpu, uint64_t reg1)
 {
-    int cssid, ssid, schid, m;
-    SubchDev *sch;
+    ioinst_handler_template_sch(cpu, reg1, "xsch", css_do_xsch);
+}
 
-    if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
-        program_interrupt(&cpu->env, PGM_OPERAND, 4);
-        return;
-    }
-    trace_ioinst_sch_id("csch", cssid, ssid, schid);
-    sch = css_find_subch(m, cssid, ssid, schid);
-    if (!sch || !css_subch_visible(sch)) {
-        setcc(cpu, 3);
-        return;
-    }
-    setcc(cpu, css_do_csch(sch).cc);
+void ioinst_handle_csch(S390CPU *cpu, uint64_t reg1)
+{
+    ioinst_handler_template_sch(cpu, reg1, "csch", css_do_csch);
 }
 
 void ioinst_handle_hsch(S390CPU *cpu, uint64_t reg1)
 {
-    int cssid, ssid, schid, m;
-    SubchDev *sch;
-
-    if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
-        program_interrupt(&cpu->env, PGM_OPERAND, 4);
-        return;
-    }
-    trace_ioinst_sch_id("hsch", cssid, ssid, schid);
-    sch = css_find_subch(m, cssid, ssid, schid);
-    if (!sch || !css_subch_visible(sch)) {
-        setcc(cpu, 3);
-        return;
-    }
-    setcc(cpu, css_do_hsch(sch).cc);
+    ioinst_handler_template_sch(cpu, reg1, "hsch", css_do_hsch);
 }
 
 static int ioinst_schib_valid(SCHIB *schib)
@@ -707,20 +689,7 @@  void ioinst_handle_schm(S390CPU *cpu, uint64_t reg1, uint64_t reg2,
 
 void ioinst_handle_rsch(S390CPU *cpu, uint64_t reg1)
 {
-    int cssid, ssid, schid, m;
-    SubchDev *sch;
-
-    if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
-        program_interrupt(&cpu->env, PGM_OPERAND, 4);
-        return;
-    }
-    trace_ioinst_sch_id("rsch", cssid, ssid, schid);
-    sch = css_find_subch(m, cssid, ssid, schid);
-    if (!sch || !css_subch_visible(sch)) {
-        setcc(cpu, 3);
-        return;
-    }
-    setcc(cpu, css_do_rsch(sch).cc);
+   ioinst_handler_template_sch(cpu, reg1, "rsch", css_do_rsch);
 }
 
 #define RCHP_REG1_RES(_reg) (_reg & 0x00000000ff00ff00)