| Message ID | 20191127175046.4911-7-frankja@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: Cleanup | expand |
On 27.11.19 18:50, Janosch Frank wrote: > It defaults to returning 0 anyway and that return value is not > necessary, as 0 is also the default rc that the caller would return. > > While doing that we can simplify the logic a bit and return early if > we inject a PGM exception. Also we always set a 0 cc, so let's not > base it on the rc of the sclp emulation functions. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > target/s390x/kvm.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index 0c9d14b4b1..08bb1edca0 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -1159,13 +1159,13 @@ void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t te_code) > kvm_s390_vcpu_interrupt(cpu, &irq); > } > > -static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run, > +static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run, > uint16_t ipbh0) > { > CPUS390XState *env = &cpu->env; > uint64_t sccb; > uint32_t code; > - int r = 0; > + int r; > > sccb = env->regs[ipbh0 & 0xf]; > code = env->regs[(ipbh0 & 0xf0) >> 4]; > @@ -1173,11 +1173,9 @@ static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run, > r = sclp_service_call(env, sccb, code); > if (r < 0) { > kvm_s390_program_interrupt(cpu, -r); > - } else { > - setcc(cpu, r); > + return; > } > - > - return 0; > + setcc(cpu, 0); For now, sclp_service_call will return <= 0 ... but don't we actually have the option to return a cc? What does the spec say? Always set to 0? At least also the TCG implementation sets the CC to whatever is returned here .... and Claudio's unit tests have code to handle cc != 0 ... and the kernel as well (drivers/s390/char/sclp.h:sclp_service_call())
On 27.11.19 19:07, David Hildenbrand wrote: > On 27.11.19 18:50, Janosch Frank wrote: >> It defaults to returning 0 anyway and that return value is not >> necessary, as 0 is also the default rc that the caller would return. >> >> While doing that we can simplify the logic a bit and return early if >> we inject a PGM exception. Also we always set a 0 cc, so let's not >> base it on the rc of the sclp emulation functions. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> target/s390x/kvm.c | 12 +++++------- >> 1 file changed, 5 insertions(+), 7 deletions(-) >> >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >> index 0c9d14b4b1..08bb1edca0 100644 >> --- a/target/s390x/kvm.c >> +++ b/target/s390x/kvm.c >> @@ -1159,13 +1159,13 @@ void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t te_code) >> kvm_s390_vcpu_interrupt(cpu, &irq); >> } >> >> -static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run, >> +static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run, >> uint16_t ipbh0) >> { >> CPUS390XState *env = &cpu->env; >> uint64_t sccb; >> uint32_t code; >> - int r = 0; >> + int r; >> >> sccb = env->regs[ipbh0 & 0xf]; >> code = env->regs[(ipbh0 & 0xf0) >> 4]; >> @@ -1173,11 +1173,9 @@ static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run, >> r = sclp_service_call(env, sccb, code); >> if (r < 0) { >> kvm_s390_program_interrupt(cpu, -r); >> - } else { >> - setcc(cpu, r); >> + return; >> } >> - >> - return 0; >> + setcc(cpu, 0); > > For now, sclp_service_call will return <= 0 ... but don't we actually > have the option to return a cc? What does the spec say? Always set to 0? > > At least also the TCG implementation sets the CC to whatever is returned > here .... and Claudio's unit tests have code to handle cc != 0 ... and > the kernel as well (drivers/s390/char/sclp.h:sclp_service_call()) > FWIW, I'd keep this as setcc(cpu, r); if we ever have to set a cc != 0.
On 11/27/19 7:07 PM, David Hildenbrand wrote: > On 27.11.19 18:50, Janosch Frank wrote: >> It defaults to returning 0 anyway and that return value is not >> necessary, as 0 is also the default rc that the caller would return. >> >> While doing that we can simplify the logic a bit and return early if >> we inject a PGM exception. Also we always set a 0 cc, so let's not >> base it on the rc of the sclp emulation functions. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> target/s390x/kvm.c | 12 +++++------- >> 1 file changed, 5 insertions(+), 7 deletions(-) >> >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >> index 0c9d14b4b1..08bb1edca0 100644 >> --- a/target/s390x/kvm.c >> +++ b/target/s390x/kvm.c >> @@ -1159,13 +1159,13 @@ void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t te_code) >> kvm_s390_vcpu_interrupt(cpu, &irq); >> } >> >> -static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run, >> +static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run, >> uint16_t ipbh0) >> { >> CPUS390XState *env = &cpu->env; >> uint64_t sccb; >> uint32_t code; >> - int r = 0; >> + int r; >> >> sccb = env->regs[ipbh0 & 0xf]; >> code = env->regs[(ipbh0 & 0xf0) >> 4]; >> @@ -1173,11 +1173,9 @@ static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run, >> r = sclp_service_call(env, sccb, code); >> if (r < 0) { >> kvm_s390_program_interrupt(cpu, -r); >> - } else { >> - setcc(cpu, r); >> + return; >> } >> - >> - return 0; >> + setcc(cpu, 0); > > For now, sclp_service_call will return <= 0 ... but don't we actually > have the option to return a cc? What does the spec say? Always set to 0? > > At least also the TCG implementation sets the CC to whatever is returned > here .... and Claudio's unit tests have code to handle cc != 0 ... and > the kernel as well (drivers/s390/char/sclp.h:sclp_service_call()) There's 0 (initiated), busy and operational and as far as I know we implement neither. sclp_service_call() returns either 0 or -PGM_CODE, so we don't need to check when we're after the pgm injection code.
On 11/27/19 7:25 PM, Janosch Frank wrote: > > There's 0 (initiated), busy and operational and as far as I know we > implement neither. That came out wrong... s/operational/not operational/ We only implement "command initiated" / cc = 0 We can never have busy, because we handle sclp calls synchronously. The spec does not give any indication when we could return "not operational". I guess that's just a free pass for hypervisors. > sclp_service_call() returns either 0 or -PGM_CODE, so we don't need to > check when we're after the pgm injection code.
On 27/11/2019 18.50, Janosch Frank wrote: > It defaults to returning 0 anyway and that return value is not > necessary, as 0 is also the default rc that the caller would return. > > While doing that we can simplify the logic a bit and return early if > we inject a PGM exception. Also we always set a 0 cc, so let's not > base it on the rc of the sclp emulation functions. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > target/s390x/kvm.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index 0c9d14b4b1..08bb1edca0 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -1159,13 +1159,13 @@ void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t te_code) > kvm_s390_vcpu_interrupt(cpu, &irq); > } > > -static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run, > +static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run, > uint16_t ipbh0) > { > CPUS390XState *env = &cpu->env; > uint64_t sccb; > uint32_t code; > - int r = 0; > + int r; > > sccb = env->regs[ipbh0 & 0xf]; > code = env->regs[(ipbh0 & 0xf0) >> 4]; > @@ -1173,11 +1173,9 @@ static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run, > r = sclp_service_call(env, sccb, code); > if (r < 0) { > kvm_s390_program_interrupt(cpu, -r); > - } else { > - setcc(cpu, r); > + return; > } > - > - return 0; > + setcc(cpu, 0); I think I'd also slightly prefer setcc(cpu, r) here ... but anyway: Reviewed-by: Thomas Huth <thuth@redhat.com>
On Wed, 27 Nov 2019 19:38:06 +0100 Janosch Frank <frankja@linux.ibm.com> wrote: > On 11/27/19 7:25 PM, Janosch Frank wrote: > > > > There's 0 (initiated), busy and operational and as far as I know we > > implement neither. > > That came out wrong... > s/operational/not operational/ > > We only implement "command initiated" / cc = 0 > We can never have busy, because we handle sclp calls synchronously. > The spec does not give any indication when we could return "not > operational". I guess that's just a free pass for hypervisors. Regardless, setcc(cpu, r) also feels a bit cleaner to me... > > > sclp_service_call() returns either 0 or -PGM_CODE, so we don't need to > > check when we're after the pgm injection code. > >
On 11/28/19 6:34 PM, Cornelia Huck wrote: > On Wed, 27 Nov 2019 19:38:06 +0100 > Janosch Frank <frankja@linux.ibm.com> wrote: > >> On 11/27/19 7:25 PM, Janosch Frank wrote: >>> >>> There's 0 (initiated), busy and operational and as far as I know we >>> implement neither. >> >> That came out wrong... >> s/operational/not operational/ >> >> We only implement "command initiated" / cc = 0 >> We can never have busy, because we handle sclp calls synchronously. >> The spec does not give any indication when we could return "not >> operational". I guess that's just a free pass for hypervisors. > > Regardless, setcc(cpu, r) also feels a bit cleaner to me... Ok, do you want to change that while picking the patches up or shall I send a new version? > >> >>> sclp_service_call() returns either 0 or -PGM_CODE, so we don't need to >>> check when we're after the pgm injection code. >> >> >
On Fri, 29 Nov 2019 09:16:21 +0100 Janosch Frank <frankja@linux.ibm.com> wrote: > On 11/28/19 6:34 PM, Cornelia Huck wrote: > > On Wed, 27 Nov 2019 19:38:06 +0100 > > Janosch Frank <frankja@linux.ibm.com> wrote: > > > >> On 11/27/19 7:25 PM, Janosch Frank wrote: > >>> > >>> There's 0 (initiated), busy and operational and as far as I know we > >>> implement neither. > >> > >> That came out wrong... > >> s/operational/not operational/ > >> > >> We only implement "command initiated" / cc = 0 > >> We can never have busy, because we handle sclp calls synchronously. > >> The spec does not give any indication when we could return "not > >> operational". I guess that's just a free pass for hypervisors. > > > > Regardless, setcc(cpu, r) also feels a bit cleaner to me... > > Ok, do you want to change that while picking the patches up or shall I > send a new version? New version (of this patch), please :) Easier for me...
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index 0c9d14b4b1..08bb1edca0 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -1159,13 +1159,13 @@ void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t te_code) kvm_s390_vcpu_interrupt(cpu, &irq); } -static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run, +static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run, uint16_t ipbh0) { CPUS390XState *env = &cpu->env; uint64_t sccb; uint32_t code; - int r = 0; + int r; sccb = env->regs[ipbh0 & 0xf]; code = env->regs[(ipbh0 & 0xf0) >> 4]; @@ -1173,11 +1173,9 @@ static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run, r = sclp_service_call(env, sccb, code); if (r < 0) { kvm_s390_program_interrupt(cpu, -r); - } else { - setcc(cpu, r); + return; } - - return 0; + setcc(cpu, 0); } static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1) @@ -1240,7 +1238,7 @@ static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1) setcc(cpu, 3); break; case PRIV_B2_SCLP_CALL: - rc = kvm_sclp_service_call(cpu, run, ipbh0); + kvm_sclp_service_call(cpu, run, ipbh0); break; default: rc = -1;
It defaults to returning 0 anyway and that return value is not necessary, as 0 is also the default rc that the caller would return. While doing that we can simplify the logic a bit and return early if we inject a PGM exception. Also we always set a 0 cc, so let's not base it on the rc of the sclp emulation functions. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- target/s390x/kvm.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)