[Qemu-devel] [PATCH 0/2] s390x: Diag 308 improvements

[Qemu-devel] [PATCH 0/2] s390x: Diag 308 improvements

[Janosch Frank]

Let's make it architecture compliant and a bit more readable.

Each non implemented diag 308 supcode should result in a specification
exception. Operating systems use the exceptions to query which codes
are available.

Janosch Frank (2):
  s390x: Diag308 move common parameter checking into function
  s390x: Return specification exception for unimplemented diag 308
    subcodes

 target/s390x/diag.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/2] s390x: Diag308 move common parameter checking into function

[Janosch Frank]

Let's make that switch statement a bit shorter.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 target/s390x/diag.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index acb0f3d4af..cfd7222ddd 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -53,6 +53,22 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
 #define DIAG_308_RC_NO_CONF         0x0102
 #define DIAG_308_RC_INVALID         0x0402
 
+static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
+                              uintptr_t ra)
+{
+    if ((r1 & 1) || (addr & 0x0fffULL)) {
+        s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
+        return -EINVAL;
+    }
+    if (!address_space_access_valid(&address_space_memory, addr,
+                                    sizeof(IplParameterBlock), true,
+                                    MEMTXATTRS_UNSPECIFIED)) {
+        s390_program_interrupt(env, PGM_ADDRESSING, ILEN_AUTO, ra);
+        return -EFAULT;
+    }
+    return 0;
+}
+
 void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
 {
     CPUState *cs = CPU(s390_env_get_cpu(env));
@@ -81,14 +97,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
         s390_ipl_reset_request(cs, S390_RESET_REIPL);
         break;
     case 5:
-        if ((r1 & 1) || (addr & 0x0fffULL)) {
-            s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
-            return;
-        }
-        if (!address_space_access_valid(&address_space_memory, addr,
-                                        sizeof(IplParameterBlock), false,
-                                        MEMTXATTRS_UNSPECIFIED)) {
-            s390_program_interrupt(env, PGM_ADDRESSING, ILEN_AUTO, ra);
+        if (diag308_parm_check(env, r1, addr, ra)) {
             return;
         }
         iplb = g_new0(IplParameterBlock, 1);
@@ -111,14 +120,7 @@ out:
         g_free(iplb);
         return;
     case 6:
-        if ((r1 & 1) || (addr & 0x0fffULL)) {
-            s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
-            return;
-        }
-        if (!address_space_access_valid(&address_space_memory, addr,
-                                        sizeof(IplParameterBlock), true,
-                                        MEMTXATTRS_UNSPECIFIED)) {
-            s390_program_interrupt(env, PGM_ADDRESSING, ILEN_AUTO, ra);
+        if (diag308_parm_check(env, r1, addr, ra)) {
             return;
         }
         iplb = s390_ipl_get_iplb();
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/2] s390x: Diag308 move common parameter checking into function

[David Hildenbrand]

On 11.01.19 12:36, Janosch Frank wrote:
> Let's make that switch statement a bit shorter.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/diag.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index acb0f3d4af..cfd7222ddd 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -53,6 +53,22 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
>  #define DIAG_308_RC_NO_CONF         0x0102
>  #define DIAG_308_RC_INVALID         0x0402
>  
> +static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
> +                              uintptr_t ra)
> +{
> +    if ((r1 & 1) || (addr & 0x0fffULL)) {
> +        s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
> +        return -EINVAL;
> +    }
> +    if (!address_space_access_valid(&address_space_memory, addr,
> +                                    sizeof(IplParameterBlock), true,

This is wrong, you would check for writing although you are only
reading. (true vs. false)

> +                                    MEMTXATTRS_UNSPECIFIED)) {
> +        s390_program_interrupt(env, PGM_ADDRESSING, ILEN_AUTO, ra);
> +        return -EFAULT;
> +    }
> +    return 0;
> +}
> +
>  void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>  {
>      CPUState *cs = CPU(s390_env_get_cpu(env));
> @@ -81,14 +97,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>          s390_ipl_reset_request(cs, S390_RESET_REIPL);
>          break;
>      case 5:
> -        if ((r1 & 1) || (addr & 0x0fffULL)) {
> -            s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
> -            return;
> -        }
> -        if (!address_space_access_valid(&address_space_memory, addr,
> -                                        sizeof(IplParameterBlock), false,
> -                                        MEMTXATTRS_UNSPECIFIED)) {
> -            s390_program_interrupt(env, PGM_ADDRESSING, ILEN_AUTO, ra);
> +        if (diag308_parm_check(env, r1, addr, ra)) {
>              return;
>          }
>          iplb = g_new0(IplParameterBlock, 1);
> @@ -111,14 +120,7 @@ out:
>          g_free(iplb);
>          return;
>      case 6:
> -        if ((r1 & 1) || (addr & 0x0fffULL)) {
> -            s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
> -            return;
> -        }
> -        if (!address_space_access_valid(&address_space_memory, addr,
> -                                        sizeof(IplParameterBlock), true,
> -                                        MEMTXATTRS_UNSPECIFIED)) {
> -            s390_program_interrupt(env, PGM_ADDRESSING, ILEN_AUTO, ra);
> +        if (diag308_parm_check(env, r1, addr, ra)) {
>              return;
>          }
>          iplb = s390_ipl_get_iplb();
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH 1/2] s390x: Diag308 move common parameter checking into function

[Janosch Frank]

[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]

On 11.01.19 16:40, David Hildenbrand wrote:
> On 11.01.19 12:36, Janosch Frank wrote:
>> Let's make that switch statement a bit shorter.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  target/s390x/diag.c | 34 ++++++++++++++++++----------------
>>  1 file changed, 18 insertions(+), 16 deletions(-)
>>
>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
>> index acb0f3d4af..cfd7222ddd 100644
>> --- a/target/s390x/diag.c
>> +++ b/target/s390x/diag.c
>> @@ -53,6 +53,22 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
>>  #define DIAG_308_RC_NO_CONF         0x0102
>>  #define DIAG_308_RC_INVALID         0x0402
>>  
>> +static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
>> +                              uintptr_t ra)
>> +{
>> +    if ((r1 & 1) || (addr & 0x0fffULL)) {
>> +        s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
>> +        return -EINVAL;
>> +    }
>> +    if (!address_space_access_valid(&address_space_memory, addr,
>> +                                    sizeof(IplParameterBlock), true,
> 
> This is wrong, you would check for writing although you are only
> reading. (true vs. false)

Friday morning patches are the best patches :)
Let's drop that one for now, we'll touch it again, if we need to tinker
there.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [PATCH 2/2] s390x: Return specification exception for unimplemented diag 308 subcodes

[Janosch Frank]

The architecture specifies specification exceptions for all
unavailable subcodes.

The presence of subcodes is indicated by checking some query subcode.
For example 6 will indicate that 3-6 are available. So future systems
might call new subcodes to check for new features. This should not
trigger a hw error, instead we return the architectured specification
exception.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Cc: qemu-stable@nongnu.org
---
 target/s390x/diag.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index cfd7222ddd..c28cf1d9f1 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -132,7 +132,7 @@ out:
         }
         return;
     default:
-        hw_error("Unhandled diag308 subcode %" PRIx64, subcode);
+        s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
         break;
     }
 }
-- 
2.14.3

Re: [Qemu-devel] [PATCH 2/2] s390x: Return specification exception for unimplemented diag 308 subcodes

[Christian Borntraeger]

On 11.01.2019 12:36, Janosch Frank wrote:
> The architecture specifies specification exceptions for all
> unavailable subcodes.
> 
> The presence of subcodes is indicated by checking some query subcode.
> For example 6 will indicate that 3-6 are available. So future systems
> might call new subcodes to check for new features. This should not
> trigger a hw error, instead we return the architectured specification
> exception.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Cc: qemu-stable@nongnu.org

Yes, PIC06 is definitively the right thing to do.

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  target/s390x/diag.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index cfd7222ddd..c28cf1d9f1 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -132,7 +132,7 @@ out:
>          }
>          return;
>      default:
> -        hw_error("Unhandled diag308 subcode %" PRIx64, subcode);
> +        s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
>          break;
>      }
>  }
> 

Re: [Qemu-devel] [PATCH 2/2] s390x: Return specification exception for unimplemented diag 308 subcodes

[David Hildenbrand]

On 11.01.19 12:36, Janosch Frank wrote:
> The architecture specifies specification exceptions for all
> unavailable subcodes.
> 
> The presence of subcodes is indicated by checking some query subcode.
> For example 6 will indicate that 3-6 are available. So future systems
> might call new subcodes to check for new features. This should not
> trigger a hw error, instead we return the architectured specification
> exception.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Cc: qemu-stable@nongnu.org
> ---
>  target/s390x/diag.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index cfd7222ddd..c28cf1d9f1 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -132,7 +132,7 @@ out:
>          }
>          return;
>      default:
> -        hw_error("Unhandled diag308 subcode %" PRIx64, subcode);
> +        s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
>          break;
>      }
>  }
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH 2/2] s390x: Return specification exception for unimplemented diag 308 subcodes

[Cornelia Huck]

On Fri, 11 Jan 2019 12:36:57 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> The architecture specifies specification exceptions for all
> unavailable subcodes.
> 
> The presence of subcodes is indicated by checking some query subcode.
> For example 6 will indicate that 3-6 are available. So future systems
> might call new subcodes to check for new features. This should not
> trigger a hw error, instead we return the architectured specification
> exception.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Cc: qemu-stable@nongnu.org
> ---
>  target/s390x/diag.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks, applied.