[v4,6/6] s390x: kvm: Make kvm_sclp_service_call void
diff mbox series

Message ID 20191127175046.4911-7-frankja@linux.ibm.com
State New
Headers show
Series
  • s390x: Cleanup
Related show

Commit Message

Janosch Frank Nov. 27, 2019, 5:50 p.m. UTC
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(-)

Comments

David Hildenbrand Nov. 27, 2019, 6:07 p.m. UTC | #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())
David Hildenbrand Nov. 27, 2019, 6:17 p.m. UTC | #2
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.
Janosch Frank Nov. 27, 2019, 6:25 p.m. UTC | #3
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.
Janosch Frank Nov. 27, 2019, 6:38 p.m. UTC | #4
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.
Thomas Huth Nov. 28, 2019, 7:48 a.m. UTC | #5
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>
Cornelia Huck Nov. 28, 2019, 5:34 p.m. UTC | #6
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.  
> 
>
Janosch Frank Nov. 29, 2019, 8:16 a.m. UTC | #7
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.  
>>
>>
>
Cornelia Huck Nov. 29, 2019, 9:01 a.m. UTC | #8
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...

Patch
diff mbox series

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;