[RFC v6 08/13] target/s390x: split cpu-dump from helper.c

Cho, Yu-Chen posted 13 patches 4 years, 7 months ago
Maintainers: David Hildenbrand <david@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Cornelia Huck <cohuck@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Jason Herne <jjherne@linux.ibm.com>, Halil Pasic <pasic@linux.ibm.com>, Tony Krowiak <akrowiak@linux.ibm.com>, Christian Borntraeger <borntraeger@de.ibm.com>, Thomas Huth <thuth@redhat.com>, Alex Williamson <alex.williamson@redhat.com>
There is a newer version of this series
[RFC v6 08/13] target/s390x: split cpu-dump from helper.c
Posted by Cho, Yu-Chen 4 years, 7 months ago
Splitting this functionality also allows us to make helper.c sysemu-only.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
Signed-off-by: Cho, Yu-Chen <acho@suse.com>
Acked-by: Cornelia Huck <cohuck@redhat.com>
---
 target/s390x/cpu-dump.c  | 176 +++++++++++++++++++++++++++++++++++++++
 target/s390x/helper.c    | 151 ---------------------------------
 target/s390x/meson.build |   1 +
 3 files changed, 177 insertions(+), 151 deletions(-)
 create mode 100644 target/s390x/cpu-dump.c

diff --git a/target/s390x/cpu-dump.c b/target/s390x/cpu-dump.c
new file mode 100644
index 0000000000..6f559c1913
--- /dev/null
+++ b/target/s390x/cpu-dump.c
@@ -0,0 +1,176 @@
+/*
+ * S/390 CPU dump to FILE
+ *
+ *  Copyright (c) 2009 Ulrich Hecht
+ *  Copyright (c) 2011 Alexander Graf
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "cpu.h"
+#include "s390x-internal.h"
+#include "qemu/qemu-print.h"
+#include "sysemu/tcg.h"
+
+void s390_cpu_set_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
+{
+#ifndef CONFIG_USER_ONLY
+    uint64_t old_mask = env->psw.mask;
+#endif
+
+    env->psw.addr = addr;
+    env->psw.mask = mask;
+
+    /* KVM will handle all WAITs and trigger a WAIT exit on disabled_wait */
+    if (!tcg_enabled()) {
+        return;
+    }
+    env->cc_op = (mask >> 44) & 3;
+
+#ifndef CONFIG_USER_ONLY
+    if ((old_mask ^ mask) & PSW_MASK_PER) {
+        s390_cpu_recompute_watchpoints(env_cpu(env));
+    }
+
+    if (mask & PSW_MASK_WAIT) {
+        s390_handle_wait(env_archcpu(env));
+    }
+#endif
+}
+
+uint64_t s390_cpu_get_psw_mask(CPUS390XState *env)
+{
+    uint64_t r = env->psw.mask;
+
+    if (tcg_enabled()) {
+        uint64_t cc = calc_cc(env, env->cc_op, env->cc_src,
+                              env->cc_dst, env->cc_vr);
+
+        assert(cc <= 3);
+        r &= ~PSW_MASK_CC;
+        r |= cc << 44;
+    }
+
+    return r;
+}
+
+void s390_cpu_dump_state(CPUState *cs, FILE *f, int flags)
+{
+    S390CPU *cpu = S390_CPU(cs);
+    CPUS390XState *env = &cpu->env;
+    int i;
+
+    qemu_fprintf(f, "PSW=mask %016" PRIx64 " addr %016" PRIx64,
+                 s390_cpu_get_psw_mask(env), env->psw.addr);
+    if (!tcg_enabled()) {
+        qemu_fprintf(f, "\n");
+    } else if (env->cc_op > 3) {
+        qemu_fprintf(f, " cc %15s\n", cc_name(env->cc_op));
+    } else {
+        qemu_fprintf(f, " cc %02x\n", env->cc_op);
+    }
+
+    for (i = 0; i < 16; i++) {
+        qemu_fprintf(f, "R%02d=%016" PRIx64, i, env->regs[i]);
+        if ((i % 4) == 3) {
+            qemu_fprintf(f, "\n");
+        } else {
+            qemu_fprintf(f, " ");
+        }
+    }
+
+    if (flags & CPU_DUMP_FPU) {
+        if (s390_has_feat(S390_FEAT_VECTOR)) {
+            for (i = 0; i < 32; i++) {
+                qemu_fprintf(f, "V%02d=%016" PRIx64 "%016" PRIx64 "%c",
+                             i, env->vregs[i][0], env->vregs[i][1],
+                             i % 2 ? '\n' : ' ');
+            }
+        } else {
+            for (i = 0; i < 16; i++) {
+                qemu_fprintf(f, "F%02d=%016" PRIx64 "%c",
+                             i, *get_freg(env, i),
+                             (i % 4) == 3 ? '\n' : ' ');
+            }
+        }
+    }
+
+#ifndef CONFIG_USER_ONLY
+    for (i = 0; i < 16; i++) {
+        qemu_fprintf(f, "C%02d=%016" PRIx64, i, env->cregs[i]);
+        if ((i % 4) == 3) {
+            qemu_fprintf(f, "\n");
+        } else {
+            qemu_fprintf(f, " ");
+        }
+    }
+#endif
+
+#ifdef DEBUG_INLINE_BRANCHES
+    for (i = 0; i < CC_OP_MAX; i++) {
+        qemu_fprintf(f, "  %15s = %10ld\t%10ld\n", cc_name(i),
+                     inline_branch_miss[i], inline_branch_hit[i]);
+    }
+#endif
+
+    qemu_fprintf(f, "\n");
+}
+
+const char *cc_name(enum cc_op cc_op)
+{
+    static const char * const cc_names[] = {
+        [CC_OP_CONST0]    = "CC_OP_CONST0",
+        [CC_OP_CONST1]    = "CC_OP_CONST1",
+        [CC_OP_CONST2]    = "CC_OP_CONST2",
+        [CC_OP_CONST3]    = "CC_OP_CONST3",
+        [CC_OP_DYNAMIC]   = "CC_OP_DYNAMIC",
+        [CC_OP_STATIC]    = "CC_OP_STATIC",
+        [CC_OP_NZ]        = "CC_OP_NZ",
+        [CC_OP_ADDU]      = "CC_OP_ADDU",
+        [CC_OP_SUBU]      = "CC_OP_SUBU",
+        [CC_OP_LTGT_32]   = "CC_OP_LTGT_32",
+        [CC_OP_LTGT_64]   = "CC_OP_LTGT_64",
+        [CC_OP_LTUGTU_32] = "CC_OP_LTUGTU_32",
+        [CC_OP_LTUGTU_64] = "CC_OP_LTUGTU_64",
+        [CC_OP_LTGT0_32]  = "CC_OP_LTGT0_32",
+        [CC_OP_LTGT0_64]  = "CC_OP_LTGT0_64",
+        [CC_OP_ADD_64]    = "CC_OP_ADD_64",
+        [CC_OP_SUB_64]    = "CC_OP_SUB_64",
+        [CC_OP_ABS_64]    = "CC_OP_ABS_64",
+        [CC_OP_NABS_64]   = "CC_OP_NABS_64",
+        [CC_OP_ADD_32]    = "CC_OP_ADD_32",
+        [CC_OP_SUB_32]    = "CC_OP_SUB_32",
+        [CC_OP_ABS_32]    = "CC_OP_ABS_32",
+        [CC_OP_NABS_32]   = "CC_OP_NABS_32",
+        [CC_OP_COMP_32]   = "CC_OP_COMP_32",
+        [CC_OP_COMP_64]   = "CC_OP_COMP_64",
+        [CC_OP_TM_32]     = "CC_OP_TM_32",
+        [CC_OP_TM_64]     = "CC_OP_TM_64",
+        [CC_OP_NZ_F32]    = "CC_OP_NZ_F32",
+        [CC_OP_NZ_F64]    = "CC_OP_NZ_F64",
+        [CC_OP_NZ_F128]   = "CC_OP_NZ_F128",
+        [CC_OP_ICM]       = "CC_OP_ICM",
+        [CC_OP_SLA_32]    = "CC_OP_SLA_32",
+        [CC_OP_SLA_64]    = "CC_OP_SLA_64",
+        [CC_OP_FLOGR]     = "CC_OP_FLOGR",
+        [CC_OP_LCBB]      = "CC_OP_LCBB",
+        [CC_OP_VC]        = "CC_OP_VC",
+        [CC_OP_MULS_32]   = "CC_OP_MULS_32",
+        [CC_OP_MULS_64]   = "CC_OP_MULS_64",
+    };
+
+    return cc_names[cc_op];
+}
diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index 8015c4e3d1..c72e990f4d 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -23,7 +23,6 @@
 #include "s390x-internal.h"
 #include "exec/gdbstub.h"
 #include "qemu/timer.h"
-#include "qemu/qemu-print.h"
 #include "hw/s390x/ioinst.h"
 #include "hw/s390x/pv.h"
 #include "sysemu/hw_accel.h"
@@ -289,153 +288,3 @@ int s390_store_adtl_status(S390CPU *cpu, hwaddr addr, hwaddr len)
 /* For user-only, tcg is always enabled. */
 #define tcg_enabled() true
 #endif /* CONFIG_USER_ONLY */
-
-void s390_cpu_set_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
-{
-#ifndef CONFIG_USER_ONLY
-    uint64_t old_mask = env->psw.mask;
-#endif
-
-    env->psw.addr = addr;
-    env->psw.mask = mask;
-
-    /* KVM will handle all WAITs and trigger a WAIT exit on disabled_wait */
-    if (!tcg_enabled()) {
-        return;
-    }
-    env->cc_op = (mask >> 44) & 3;
-
-#ifndef CONFIG_USER_ONLY
-    if ((old_mask ^ mask) & PSW_MASK_PER) {
-        s390_cpu_recompute_watchpoints(env_cpu(env));
-    }
-
-    if (mask & PSW_MASK_WAIT) {
-        s390_handle_wait(env_archcpu(env));
-    }
-#endif
-}
-
-uint64_t s390_cpu_get_psw_mask(CPUS390XState *env)
-{
-    uint64_t r = env->psw.mask;
-
-    if (tcg_enabled()) {
-        uint64_t cc = calc_cc(env, env->cc_op, env->cc_src,
-                              env->cc_dst, env->cc_vr);
-
-        assert(cc <= 3);
-        r &= ~PSW_MASK_CC;
-        r |= cc << 44;
-    }
-
-    return r;
-}
-
-void s390_cpu_dump_state(CPUState *cs, FILE *f, int flags)
-{
-    S390CPU *cpu = S390_CPU(cs);
-    CPUS390XState *env = &cpu->env;
-    int i;
-
-    qemu_fprintf(f, "PSW=mask %016" PRIx64 " addr %016" PRIx64,
-                 s390_cpu_get_psw_mask(env), env->psw.addr);
-    if (!tcg_enabled()) {
-        qemu_fprintf(f, "\n");
-    } else if (env->cc_op > 3) {
-        qemu_fprintf(f, " cc %15s\n", cc_name(env->cc_op));
-    } else {
-        qemu_fprintf(f, " cc %02x\n", env->cc_op);
-    }
-
-    for (i = 0; i < 16; i++) {
-        qemu_fprintf(f, "R%02d=%016" PRIx64, i, env->regs[i]);
-        if ((i % 4) == 3) {
-            qemu_fprintf(f, "\n");
-        } else {
-            qemu_fprintf(f, " ");
-        }
-    }
-
-    if (flags & CPU_DUMP_FPU) {
-        if (s390_has_feat(S390_FEAT_VECTOR)) {
-            for (i = 0; i < 32; i++) {
-                qemu_fprintf(f, "V%02d=%016" PRIx64 "%016" PRIx64 "%c",
-                             i, env->vregs[i][0], env->vregs[i][1],
-                             i % 2 ? '\n' : ' ');
-            }
-        } else {
-            for (i = 0; i < 16; i++) {
-                qemu_fprintf(f, "F%02d=%016" PRIx64 "%c",
-                             i, *get_freg(env, i),
-                             (i % 4) == 3 ? '\n' : ' ');
-            }
-        }
-    }
-
-#ifndef CONFIG_USER_ONLY
-    for (i = 0; i < 16; i++) {
-        qemu_fprintf(f, "C%02d=%016" PRIx64, i, env->cregs[i]);
-        if ((i % 4) == 3) {
-            qemu_fprintf(f, "\n");
-        } else {
-            qemu_fprintf(f, " ");
-        }
-    }
-#endif
-
-#ifdef DEBUG_INLINE_BRANCHES
-    for (i = 0; i < CC_OP_MAX; i++) {
-        qemu_fprintf(f, "  %15s = %10ld\t%10ld\n", cc_name(i),
-                     inline_branch_miss[i], inline_branch_hit[i]);
-    }
-#endif
-
-    qemu_fprintf(f, "\n");
-}
-
-const char *cc_name(enum cc_op cc_op)
-{
-    static const char * const cc_names[] = {
-        [CC_OP_CONST0]    = "CC_OP_CONST0",
-        [CC_OP_CONST1]    = "CC_OP_CONST1",
-        [CC_OP_CONST2]    = "CC_OP_CONST2",
-        [CC_OP_CONST3]    = "CC_OP_CONST3",
-        [CC_OP_DYNAMIC]   = "CC_OP_DYNAMIC",
-        [CC_OP_STATIC]    = "CC_OP_STATIC",
-        [CC_OP_NZ]        = "CC_OP_NZ",
-        [CC_OP_ADDU]      = "CC_OP_ADDU",
-        [CC_OP_SUBU]      = "CC_OP_SUBU",
-        [CC_OP_LTGT_32]   = "CC_OP_LTGT_32",
-        [CC_OP_LTGT_64]   = "CC_OP_LTGT_64",
-        [CC_OP_LTUGTU_32] = "CC_OP_LTUGTU_32",
-        [CC_OP_LTUGTU_64] = "CC_OP_LTUGTU_64",
-        [CC_OP_LTGT0_32]  = "CC_OP_LTGT0_32",
-        [CC_OP_LTGT0_64]  = "CC_OP_LTGT0_64",
-        [CC_OP_ADD_64]    = "CC_OP_ADD_64",
-        [CC_OP_SUB_64]    = "CC_OP_SUB_64",
-        [CC_OP_ABS_64]    = "CC_OP_ABS_64",
-        [CC_OP_NABS_64]   = "CC_OP_NABS_64",
-        [CC_OP_ADD_32]    = "CC_OP_ADD_32",
-        [CC_OP_SUB_32]    = "CC_OP_SUB_32",
-        [CC_OP_ABS_32]    = "CC_OP_ABS_32",
-        [CC_OP_NABS_32]   = "CC_OP_NABS_32",
-        [CC_OP_COMP_32]   = "CC_OP_COMP_32",
-        [CC_OP_COMP_64]   = "CC_OP_COMP_64",
-        [CC_OP_TM_32]     = "CC_OP_TM_32",
-        [CC_OP_TM_64]     = "CC_OP_TM_64",
-        [CC_OP_NZ_F32]    = "CC_OP_NZ_F32",
-        [CC_OP_NZ_F64]    = "CC_OP_NZ_F64",
-        [CC_OP_NZ_F128]   = "CC_OP_NZ_F128",
-        [CC_OP_ICM]       = "CC_OP_ICM",
-        [CC_OP_SLA_32]    = "CC_OP_SLA_32",
-        [CC_OP_SLA_64]    = "CC_OP_SLA_64",
-        [CC_OP_FLOGR]     = "CC_OP_FLOGR",
-        [CC_OP_LCBB]      = "CC_OP_LCBB",
-        [CC_OP_VC]        = "CC_OP_VC",
-        [CC_OP_MULS_32]   = "CC_OP_MULS_32",
-        [CC_OP_MULS_64]   = "CC_OP_MULS_64",
-    };
-
-    return cc_names[cc_op];
-}
diff --git a/target/s390x/meson.build b/target/s390x/meson.build
index a73bae3dc5..6e1aa3b0cd 100644
--- a/target/s390x/meson.build
+++ b/target/s390x/meson.build
@@ -6,6 +6,7 @@ s390x_ss.add(files(
   'gdbstub.c',
   'helper.c',
   'interrupt.c',
+  'cpu-dump.c',
 ))
 
 s390x_ss.add(when: 'CONFIG_KVM', if_true: files('kvm.c'), if_false: files('kvm-stub.c'))
-- 
2.32.0


Re: [RFC v6 08/13] target/s390x: split cpu-dump from helper.c
Posted by Thomas Huth 4 years, 7 months ago
On 29/06/2021 16.19, Cho, Yu-Chen wrote:
> Splitting this functionality also allows us to make helper.c sysemu-only.
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> Signed-off-by: Cho, Yu-Chen <acho@suse.com>
> Acked-by: Cornelia Huck <cohuck@redhat.com>
> ---
>   target/s390x/cpu-dump.c  | 176 +++++++++++++++++++++++++++++++++++++++

Apart from the dump() function, the other functions here are are used in 
other contexts, too, so maybe the name is not very appropriate here... What 
about naming it "cpu-state.c" instead? Or include the functions in cpu.c 
directly?

  Thomas


Re: [RFC v6 08/13] target/s390x: split cpu-dump from helper.c
Posted by Al Cho 4 years, 7 months ago
On Thu, 2021-07-01 at 14:35 +0200, Thomas Huth wrote:
> On 29/06/2021 16.19, Cho, Yu-Chen wrote:
> > Splitting this functionality also allows us to make helper.c sysemu-
> > only.
> > 
> > Signed-off-by: Claudio Fontana <cfontana@suse.de>
> > Signed-off-by: Cho, Yu-Chen <acho@suse.com>
> > Acked-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >   target/s390x/cpu-dump.c  | 176
> > +++++++++++++++++++++++++++++++++++++++
> 
> Apart from the dump() function, the other functions here are are used
> in 
> other contexts, too, so maybe the name is not very appropriate here...
> What 
> about naming it "cpu-state.c" instead? Or include the functions in
> cpu.c 
> directly?
> 

ok, I think naming it "cpu-state.c" would make more sense.

Thanks,
            AL

Re: [RFC v6 08/13] target/s390x: split cpu-dump from helper.c
Posted by Claudio Fontana 4 years, 7 months ago
On 7/2/21 9:25 AM, Al Cho wrote:
> On Thu, 2021-07-01 at 14:35 +0200, Thomas Huth wrote:
>> On 29/06/2021 16.19, Cho, Yu-Chen wrote:
>>> Splitting this functionality also allows us to make helper.c sysemu-
>>> only.
>>>
>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>> Signed-off-by: Cho, Yu-Chen <acho@suse.com>
>>> Acked-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>   target/s390x/cpu-dump.c  | 176
>>> +++++++++++++++++++++++++++++++++++++++
>>
>> Apart from the dump() function, the other functions here are are used
>> in 
>> other contexts, too, so maybe the name is not very appropriate here...
>> What 
>> about naming it "cpu-state.c" instead? Or include the functions in
>> cpu.c 
>> directly?
>>
> 
> ok, I think naming it "cpu-state.c" would make more sense.
> 
> Thanks,
>             AL
> 

For context, cpu-dump.c mimics how this is done on x86,

so rather than coming up with creative new names for each architecture,
I'd rather either put the code into cpu.c, or just keep the existing "cpu-dump.c" as in the initially proposed patch, which looks like the best option to me.

Thanks

Claudio

Re: [RFC v6 08/13] target/s390x: split cpu-dump from helper.c
Posted by Al Cho 4 years, 7 months ago
On Mon, 2021-07-05 at 08:25 +0200, Claudio Fontana wrote:
> On 7/2/21 9:25 AM, Al Cho wrote:
> > On Thu, 2021-07-01 at 14:35 +0200, Thomas Huth wrote:
> > > On 29/06/2021 16.19, Cho, Yu-Chen wrote:
> > > > Splitting this functionality also allows us to make helper.c
> > > > sysemu-
> > > > only.
> > > > 
> > > > Signed-off-by: Claudio Fontana <cfontana@suse.de>
> > > > Signed-off-by: Cho, Yu-Chen <acho@suse.com>
> > > > Acked-by: Cornelia Huck <cohuck@redhat.com>
> > > > ---
> > > >   target/s390x/cpu-dump.c  | 176
> > > > +++++++++++++++++++++++++++++++++++++++
> > > 
> > > Apart from the dump() function, the other functions here are are
> > > used
> > > in 
> > > other contexts, too, so maybe the name is not very appropriate
> > > here...
> > > What 
> > > about naming it "cpu-state.c" instead? Or include the functions
> > > in
> > > cpu.c 
> > > directly?
> > > 
> > 
> > ok, I think naming it "cpu-state.c" would make more sense.
> > 
> > Thanks,
> >             AL
> > 
> 
> For context, cpu-dump.c mimics how this is done on x86,
> 
> so rather than coming up with creative new names for each
> architecture,

I think Claudio is right, I didn't recognize it before. sorry.

> I'd rather either put the code into cpu.c, or just keep the existing
> "cpu-dump.c" as in the initially proposed patch, which looks like the
> best option to me.
> 

For me just keep the existing "cpu-dump.c" as in the initially proposed
patch would be the better one option.
But it's also good to me if we keep the dump() function in cpu-dump.c
and put other functions into cpu.c.

Cheers,
      AL
Re: [RFC v6 08/13] target/s390x: split cpu-dump from helper.c
Posted by Thomas Huth 4 years, 7 months ago
On 06/07/2021 10.47, Al Cho wrote:
> On Mon, 2021-07-05 at 08:25 +0200, Claudio Fontana wrote:
>> On 7/2/21 9:25 AM, Al Cho wrote:
>>> On Thu, 2021-07-01 at 14:35 +0200, Thomas Huth wrote:
>>>> On 29/06/2021 16.19, Cho, Yu-Chen wrote:
>>>>> Splitting this functionality also allows us to make helper.c
>>>>> sysemu-
>>>>> only.
>>>>>
>>>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>>>> Signed-off-by: Cho, Yu-Chen <acho@suse.com>
>>>>> Acked-by: Cornelia Huck <cohuck@redhat.com>
>>>>> ---
>>>>>    target/s390x/cpu-dump.c  | 176
>>>>> +++++++++++++++++++++++++++++++++++++++
>>>>
>>>> Apart from the dump() function, the other functions here are are
>>>> used
>>>> in
>>>> other contexts, too, so maybe the name is not very appropriate
>>>> here...
>>>> What
>>>> about naming it "cpu-state.c" instead? Or include the functions
>>>> in
>>>> cpu.c
>>>> directly?
>>>>
>>>
>>> ok, I think naming it "cpu-state.c" would make more sense.
>>>
>>> Thanks,
>>>              AL
>>>
>>
>> For context, cpu-dump.c mimics how this is done on x86,
>>
>> so rather than coming up with creative new names for each
>> architecture,
> 
> I think Claudio is right, I didn't recognize it before. sorry.
> 
>> I'd rather either put the code into cpu.c, or just keep the existing
>> "cpu-dump.c" as in the initially proposed patch, which looks like the
>> best option to me.
>>
> 
> For me just keep the existing "cpu-dump.c" as in the initially proposed
> patch would be the better one option.
> But it's also good to me if we keep the dump() function in cpu-dump.c
> and put other functions into cpu.c.

FWIW, if you don't like cpu-state.c, I'd vote for putting the dump() 
function into cpu-dump.c and put the other functions into cpu.c instead.

  Thomas


Re: [RFC v6 08/13] target/s390x: split cpu-dump from helper.c
Posted by Claudio Fontana 4 years, 7 months ago
On 7/6/21 11:06 AM, Thomas Huth wrote:
> On 06/07/2021 10.47, Al Cho wrote:
>> On Mon, 2021-07-05 at 08:25 +0200, Claudio Fontana wrote:
>>> On 7/2/21 9:25 AM, Al Cho wrote:
>>>> On Thu, 2021-07-01 at 14:35 +0200, Thomas Huth wrote:
>>>>> On 29/06/2021 16.19, Cho, Yu-Chen wrote:
>>>>>> Splitting this functionality also allows us to make helper.c
>>>>>> sysemu-
>>>>>> only.
>>>>>>
>>>>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>>>>> Signed-off-by: Cho, Yu-Chen <acho@suse.com>
>>>>>> Acked-by: Cornelia Huck <cohuck@redhat.com>
>>>>>> ---
>>>>>>    target/s390x/cpu-dump.c  | 176
>>>>>> +++++++++++++++++++++++++++++++++++++++
>>>>>
>>>>> Apart from the dump() function, the other functions here are are
>>>>> used
>>>>> in
>>>>> other contexts, too, so maybe the name is not very appropriate
>>>>> here...
>>>>> What
>>>>> about naming it "cpu-state.c" instead? Or include the functions
>>>>> in
>>>>> cpu.c
>>>>> directly?
>>>>>
>>>>
>>>> ok, I think naming it "cpu-state.c" would make more sense.
>>>>
>>>> Thanks,
>>>>              AL
>>>>
>>>
>>> For context, cpu-dump.c mimics how this is done on x86,
>>>
>>> so rather than coming up with creative new names for each
>>> architecture,
>>
>> I think Claudio is right, I didn't recognize it before. sorry.
>>
>>> I'd rather either put the code into cpu.c, or just keep the existing
>>> "cpu-dump.c" as in the initially proposed patch, which looks like the
>>> best option to me.
>>>
>>
>> For me just keep the existing "cpu-dump.c" as in the initially proposed
>> patch would be the better one option.
>> But it's also good to me if we keep the dump() function in cpu-dump.c
>> and put other functions into cpu.c.
> 
> FWIW, if you don't like cpu-state.c, I'd vote for putting the dump() 
> function into cpu-dump.c and put the other functions into cpu.c instead.
> 
>   Thomas
> 

Ah I see the issue now, the patch currently includes functions in cpu-dump.c that are not really cpu dump functions,
and should go back into cpu.c .

Agreed, this seems to be the next step.

Thanks,

Claudio