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);
}
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;
--
2.20.1
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())
--
Thanks,
David / dhildenb
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.
--
Thanks,
David / dhildenb
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 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...
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.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.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..ad6e38c876 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, r);
}
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;
--
2.20.1
On 29.11.19 10:17, 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.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.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..ad6e38c876 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, r);
> }
>
> 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;
>
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
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>
© 2016 - 2026 Red Hat, Inc.