From: Thomas Huth <thuth@redhat.com>
To: Janosch Frank <frankja@linux.ibm.com>, qemu-devel@nongnu.org
Cc: pmorel@linux.ibm.com, david@redhat.com, cohuck@redhat.com,
borntraeger@de.ibm.com, qemu-s390x@nongnu.org,
mihajlov@linux.ibm.com
Subject: Re: [PATCH 01/15] s390x: Cleanup cpu resets
Date: Thu, 21 Nov 2019 13:53:46 +0100 [thread overview]
Message-ID: <32e994b4-6ae2-8de2-4ae0-fc61a21f5f7a@redhat.com> (raw)
In-Reply-To: <20191120114334.2287-2-frankja@linux.ibm.com>
On 20/11/2019 12.43, Janosch Frank wrote:
> Let's move the resets into one function and switch by type, so we can
> use fallthroughs for shared reset actions.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> hw/s390x/s390-virtio-ccw.c | 3 +
> target/s390x/cpu.c | 111 ++++++++++++++++---------------------
> 2 files changed, 52 insertions(+), 62 deletions(-)
[...]
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 3abe7e80fd..10d5b915d8 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -82,67 +82,53 @@ static void s390_cpu_load_normal(CPUState *s)
> }
> #endif
>
> -/* S390CPUClass::cpu_reset() */
> -static void s390_cpu_reset(CPUState *s)
> +enum {
> + S390_CPU_RESET_NORMAL,
> + S390_CPU_RESET_INITIAL,
> + S390_CPU_RESET_CLEAR,
> +};
> +
> +static void s390_cpu_reset(CPUState *s, uint8_t type)
Please give the enum a name and use that instead of uint8_t for "type".
Or at least make it an "int". uint8_t is not really appropriate here.
> {
> S390CPU *cpu = S390_CPU(s);
> S390CPUClass *scc = S390_CPU_GET_CLASS(cpu);
> CPUS390XState *env = &cpu->env;
>
> - env->pfault_token = -1UL;
> - env->bpbc = false;
> scc->parent_reset(s);
> cpu->env.sigp_order = 0;
> s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
> -}
>
> -/* S390CPUClass::initial_reset() */
> -static void s390_cpu_initial_reset(CPUState *s)
> -{
> - S390CPU *cpu = S390_CPU(s);
> - CPUS390XState *env = &cpu->env;
> + /* Set initial values after clearing */
> + switch (type) {
> + case S390_CPU_RESET_CLEAR:
> + /* Fallthrough will clear the rest */
I think you could drop the above comment, since /* Fallthrough */ two
lines later should be enough.
> + memset(env, 0, offsetof(CPUS390XState, start_initial_reset_fields));
> + /* Fallthrough */
> + case S390_CPU_RESET_INITIAL:
> + memset(&env->start_initial_reset_fields, 0,
> + offsetof(CPUS390XState, end_reset_fields) -
> + offsetof(CPUS390XState, start_initial_reset_fields));
> + /* architectured initial values for CR 0 and 14 */
> + env->cregs[0] = CR0_RESET;
> + env->cregs[14] = CR14_RESET;
>
> - s390_cpu_reset(s);
> - /* initial reset does not clear everything! */
> - memset(&env->start_initial_reset_fields, 0,
> - offsetof(CPUS390XState, end_reset_fields) -
> - offsetof(CPUS390XState, start_initial_reset_fields));
> -
> - /* architectured initial values for CR 0 and 14 */
> - env->cregs[0] = CR0_RESET;
> - env->cregs[14] = CR14_RESET;
> -
> - /* architectured initial value for Breaking-Event-Address register */
> - env->gbea = 1;
> -
> - env->pfault_token = -1UL;
> -
> - /* tininess for underflow is detected before rounding */
> - set_float_detect_tininess(float_tininess_before_rounding,
> - &env->fpu_status);
> + /* architectured initial value for Breaking-Event-Address register */
> + env->gbea = 1;
> + /* tininess for underflow is detected before rounding */
> + set_float_detect_tininess(float_tininess_before_rounding,
> + &env->fpu_status);
> + /* Fallthrough */
> + case S390_CPU_RESET_NORMAL:
> + env->pfault_token = -1UL;
> + env->bpbc = false;
> + break;
> + }
>
> /* Reset state inside the kernel that we cannot access yet from QEMU. */
> - if (kvm_enabled()) {
> - kvm_s390_reset_vcpu(cpu);
> + if (kvm_enabled() && (type == S390_CPU_RESET_CLEAR ||
> + type == S390_CPU_RESET_INITIAL)) {
> + kvm_s390_reset_vcpu(cpu);
> }
Why don't you simply move that into the switch-case statement, too?
[...]
Anyway, re-using code is of course a good idea, but I wonder whether it
would be nicer to keep most things in place, and then simply chain the
functions like this:
static void s390_cpu_reset_normal(CPUState *s)
{
...
}
static void s390_cpu_reset_initial(CPUState *s)
{
...
s390_cpu_reset_normal(s);
...
}
static void s390_cpu_reset_clear(CPUState *s)
{
...
s390_cpu_reset_initial()
...
}
Just my 0.02 €, but at least for me, that's easier to understand than a
switch-case statement with fallthroughs inbetween.
Thomas
next prev parent reply other threads:[~2019-11-21 12:54 UTC|newest]
Thread overview: 90+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-20 11:43 [PATCH 00/15] s390x: Protected Virtualization support Janosch Frank
2019-11-20 11:43 ` [PATCH 01/15] s390x: Cleanup cpu resets Janosch Frank
2019-11-21 11:10 ` Cornelia Huck
2019-11-21 11:32 ` Janosch Frank
2019-11-21 12:18 ` Cornelia Huck
2019-11-21 12:53 ` Thomas Huth [this message]
2019-11-21 13:11 ` Janosch Frank
2019-11-21 13:17 ` Thomas Huth
2019-11-20 11:43 ` [PATCH 02/15] s390x: Beautify diag308 handling Janosch Frank
2019-11-21 11:17 ` Cornelia Huck
2019-11-21 11:27 ` Janosch Frank
2019-11-21 11:21 ` David Hildenbrand
2019-11-21 11:28 ` Janosch Frank
2019-11-21 13:12 ` Thomas Huth
2019-11-21 13:20 ` Thomas Huth
2019-11-21 13:53 ` Janosch Frank
2019-11-20 11:43 ` [PATCH 03/15] s390x: protvirt: Add diag308 subcodes 8 - 10 Janosch Frank
2019-11-21 12:47 ` Cornelia Huck
2019-11-21 14:36 ` Thomas Huth
2020-02-07 7:56 ` Janosch Frank
2019-11-20 11:43 ` [PATCH 04/15] Header sync protvirt Janosch Frank
2019-11-21 12:59 ` Cornelia Huck
2019-11-21 13:12 ` Janosch Frank
2019-11-21 13:17 ` Cornelia Huck
2019-11-20 11:43 ` [PATCH 05/15] s390x: protvirt: Sync PV state Janosch Frank
2019-11-21 13:25 ` Cornelia Huck
2019-11-21 13:43 ` Janosch Frank
2019-11-21 14:43 ` Thomas Huth
2019-11-20 11:43 ` [PATCH 06/15] s390x: protvirt: Support unpack facility Janosch Frank
2019-11-20 13:43 ` Cornelia Huck
2019-11-21 11:33 ` Janosch Frank
2019-11-21 11:27 ` David Hildenbrand
2019-11-21 14:25 ` Janosch Frank
2019-11-21 14:28 ` David Hildenbrand
2019-11-21 14:31 ` Christian Borntraeger
2019-11-21 14:32 ` Janosch Frank
2019-11-22 13:39 ` Cornelia Huck
2019-11-22 13:49 ` Janosch Frank
2019-11-28 14:07 ` Thomas Huth
2019-11-28 14:20 ` Janosch Frank
2019-11-20 11:43 ` [PATCH 07/15] s390x: protvirt: Handle diag 308 subcodes 0,1,3,4 Janosch Frank
2019-11-21 13:50 ` Cornelia Huck
2019-11-21 14:00 ` Janosch Frank
2019-11-21 14:04 ` Janosch Frank
2019-11-21 14:17 ` Cornelia Huck
2019-11-21 14:23 ` Janosch Frank
2019-11-20 11:43 ` [PATCH 08/15] s390x: protvirt: KVM intercept changes Janosch Frank
2019-11-21 14:07 ` Cornelia Huck
2019-11-21 14:29 ` Janosch Frank
2019-11-21 15:11 ` Thomas Huth
2019-11-28 16:38 ` Janosch Frank
2019-11-28 16:45 ` Cornelia Huck
2019-11-28 16:54 ` Janosch Frank
2019-11-20 11:43 ` [PATCH 09/15] s390x: protvirt: SCLP interpretation Janosch Frank
2019-11-21 14:11 ` Cornelia Huck
2019-11-21 14:24 ` Janosch Frank
2019-11-22 13:48 ` Pierre Morel
2019-11-20 11:43 ` [PATCH 10/15] s390x: protvirt: Add new VCPU reset functions Janosch Frank
2019-11-20 11:43 ` [PATCH 11/15] RFC: s390x: Exit on vcpu reset error Janosch Frank
2019-11-21 12:14 ` David Hildenbrand
2019-11-21 12:19 ` Janosch Frank
2019-11-21 12:22 ` David Hildenbrand
2019-11-20 11:43 ` [PATCH 12/15] s390x: protvirt: Set guest IPL PSW Janosch Frank
2019-11-28 14:30 ` Thomas Huth
2019-11-28 15:39 ` Janosch Frank
2019-11-20 11:43 ` [PATCH 13/15] s390x: protvirt: Move diag 308 data over SIDAD Janosch Frank
2019-11-28 14:40 ` Thomas Huth
2019-11-28 16:08 ` Janosch Frank
2019-11-28 16:14 ` David Hildenbrand
2019-11-20 11:43 ` [PATCH 14/15] s390x: protvirt: Disable address checks for PV guest IO emulation Janosch Frank
2019-11-28 15:28 ` Thomas Huth
2019-11-28 15:36 ` Janosch Frank
2019-11-28 16:10 ` Janosch Frank
2019-11-28 16:18 ` Cornelia Huck
2019-11-28 16:24 ` Janosch Frank
2019-11-28 20:08 ` Thomas Huth
2019-11-20 11:43 ` [PATCH 15/15] s390x: protvirt: Handle SIGP store status correctly Janosch Frank
2019-11-21 11:24 ` David Hildenbrand
2019-11-21 11:29 ` Janosch Frank
2019-11-28 15:30 ` Thomas Huth
2019-11-20 13:26 ` [PATCH 00/15] s390x: Protected Virtualization support Cornelia Huck
2019-11-20 13:33 ` Janosch Frank
2019-11-21 9:13 ` Janosch Frank
2019-11-21 9:39 ` Cornelia Huck
2019-11-29 11:08 ` Daniel P. Berrangé
2019-11-29 12:14 ` Janosch Frank
2019-11-29 12:35 ` Daniel P. Berrangé
2019-11-29 14:02 ` Janosch Frank
2019-11-29 14:30 ` Viktor Mihajlovski
2019-12-03 10:49 ` Daniel P. Berrangé
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=32e994b4-6ae2-8de2-4ae0-fc61a21f5f7a@redhat.com \
--to=thuth@redhat.com \
--cc=borntraeger@de.ibm.com \
--cc=cohuck@redhat.com \
--cc=david@redhat.com \
--cc=frankja@linux.ibm.com \
--cc=mihajlov@linux.ibm.com \
--cc=pmorel@linux.ibm.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-s390x@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).