[02/15] s390x: Beautify diag308 handling
diff mbox series

Message ID 20191120114334.2287-3-frankja@linux.ibm.com
State New
Headers show
Series
  • s390x: Protected Virtualization support
Related show

Commit Message

Janosch Frank Nov. 20, 2019, 11:43 a.m. UTC
Let's improve readability by:
* Using constants for the subcodes
* Moving parameter checking into a function
* Removing subcode > 6 check as the default case catches that

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

Comments

Cornelia Huck Nov. 21, 2019, 11:17 a.m. UTC | #1
On Wed, 20 Nov 2019 06:43:21 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's improve readability by:
> * Using constants for the subcodes
> * Moving parameter checking into a function
> * Removing subcode > 6 check as the default case catches that
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/diag.c | 54 +++++++++++++++++++++++++++------------------
>  1 file changed, 32 insertions(+), 22 deletions(-)
> 
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index 53c2f81f2a..067c667ba7 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -53,6 +53,29 @@ 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
>  
> +#define DIAG308_RES_MOD_CLR		0
> +#define DIAG308_RES_LOAD_NORM		1

Maybe s/RES/RESET/ ? Not that much longer, but clearer IMHO.

> +#define DIAG308_LOAD_CLEAR		3
> +#define DIAG308_LOAD_NORMAL_DUMP	4
> +#define DIAG308_SET			5
> +#define DIAG308_STORE			6

(...)

Really more readable afterwards.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
David Hildenbrand Nov. 21, 2019, 11:21 a.m. UTC | #2
On 20.11.19 12:43, Janosch Frank wrote:
> Let's improve readability by:
> * Using constants for the subcodes
> * Moving parameter checking into a function
> * Removing subcode > 6 check as the default case catches that
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   target/s390x/diag.c | 54 +++++++++++++++++++++++++++------------------
>   1 file changed, 32 insertions(+), 22 deletions(-)
> 
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index 53c2f81f2a..067c667ba7 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -53,6 +53,29 @@ 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
>   
> +#define DIAG308_RES_MOD_CLR		0
> +#define DIAG308_RES_LOAD_NORM		1
> +#define DIAG308_LOAD_CLEAR		3
> +#define DIAG308_LOAD_NORMAL_DUMP	4
> +#define DIAG308_SET			5
> +#define DIAG308_STORE			6
> +
> +static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
> +                              uintptr_t ra, bool write)
> +{
> +    if ((r1 & 1) || (addr & ~TARGET_PAGE_MASK)) {
> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> +        return -EINVAL;
> +    }
> +    if (!address_space_access_valid(&address_space_memory, addr,
> +                                    sizeof(IplParameterBlock), write,
> +                                    MEMTXATTRS_UNSPECIFIED)) {
> +        s390_program_interrupt(env, PGM_ADDRESSING, ra);
> +        return -EINVAL;
> +    }
> +    return 0;
> +}
> +
>   void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>   {
>       CPUState *cs = env_cpu(env);
> @@ -65,30 +88,24 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>           return;
>       }
>   
> -    if ((subcode & ~0x0ffffULL) || (subcode > 6)) {
> +    if (subcode & ~0x0ffffULL) {

Strange, the default case in the switch was basically dead code.

>           s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>           return;
>       }
>   
>       switch (subcode) {
> -    case 0:
> +    case DIAG308_RES_MOD_CLR:
>           s390_ipl_reset_request(cs, S390_RESET_MODIFIED_CLEAR);
>           break;
> -    case 1:
> +    case DIAG308_RES_LOAD_NORM:
>           s390_ipl_reset_request(cs, S390_RESET_LOAD_NORMAL);
>           break;
> -    case 3:
> +    case DIAG308_LOAD_CLEAR:
> +        /* Well we still lack the clearing bit... */
>           s390_ipl_reset_request(cs, S390_RESET_REIPL);
>           break;
> -    case 5:
> -        if ((r1 & 1) || (addr & 0x0fffULL)) {
> -            s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> -            return;
> -        }
> -        if (!address_space_access_valid(&address_space_memory, addr,
> -                                        sizeof(IplParameterBlock), false,
> -                                        MEMTXATTRS_UNSPECIFIED)) {
> -            s390_program_interrupt(env, PGM_ADDRESSING, ra);
> +    case DIAG308_SET:
> +        if (diag308_parm_check(env, r1, addr, ra, false)) {
>               return;
>           }
>           iplb = g_new0(IplParameterBlock, 1);
> @@ -110,15 +127,8 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>   out:
>           g_free(iplb);
>           return;
> -    case 6:
> -        if ((r1 & 1) || (addr & 0x0fffULL)) {
> -            s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> -            return;
> -        }
> -        if (!address_space_access_valid(&address_space_memory, addr,
> -                                        sizeof(IplParameterBlock), true,
> -                                        MEMTXATTRS_UNSPECIFIED)) {
> -            s390_program_interrupt(env, PGM_ADDRESSING, ra);
> +    case DIAG308_STORE:
> +        if (diag308_parm_check(env, r1, addr, ra, true)) {
>               return;
>           }
>           iplb = s390_ipl_get_iplb();
> 

Reviewed-by: David Hildenbrand <david@redhat.com>
Janosch Frank Nov. 21, 2019, 11:27 a.m. UTC | #3
On 11/21/19 12:17 PM, Cornelia Huck wrote:
> On Wed, 20 Nov 2019 06:43:21 -0500
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Let's improve readability by:
>> * Using constants for the subcodes
>> * Moving parameter checking into a function
>> * Removing subcode > 6 check as the default case catches that
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  target/s390x/diag.c | 54 +++++++++++++++++++++++++++------------------
>>  1 file changed, 32 insertions(+), 22 deletions(-)
>>
>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
>> index 53c2f81f2a..067c667ba7 100644
>> --- a/target/s390x/diag.c
>> +++ b/target/s390x/diag.c
>> @@ -53,6 +53,29 @@ 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
>>  
>> +#define DIAG308_RES_MOD_CLR		0
>> +#define DIAG308_RES_LOAD_NORM		1
> 
> Maybe s/RES/RESET/ ? Not that much longer, but clearer IMHO.

Sure

> 
>> +#define DIAG308_LOAD_CLEAR		3
>> +#define DIAG308_LOAD_NORMAL_DUMP	4
>> +#define DIAG308_SET			5
>> +#define DIAG308_STORE			6
> 
> (...)
> 
> Really more readable afterwards.
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Thanks!
Janosch Frank Nov. 21, 2019, 11:28 a.m. UTC | #4
On 11/21/19 12:21 PM, David Hildenbrand wrote:
> On 20.11.19 12:43, Janosch Frank wrote:
>> Let's improve readability by:
>> * Using constants for the subcodes
>> * Moving parameter checking into a function
>> * Removing subcode > 6 check as the default case catches that
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   target/s390x/diag.c | 54 +++++++++++++++++++++++++++------------------
>>   1 file changed, 32 insertions(+), 22 deletions(-)
>>
>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
>> index 53c2f81f2a..067c667ba7 100644
>> --- a/target/s390x/diag.c
>> +++ b/target/s390x/diag.c
>> @@ -53,6 +53,29 @@ 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
>>   
>> +#define DIAG308_RES_MOD_CLR		0
>> +#define DIAG308_RES_LOAD_NORM		1
>> +#define DIAG308_LOAD_CLEAR		3
>> +#define DIAG308_LOAD_NORMAL_DUMP	4
>> +#define DIAG308_SET			5
>> +#define DIAG308_STORE			6
>> +
>> +static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
>> +                              uintptr_t ra, bool write)
>> +{
>> +    if ((r1 & 1) || (addr & ~TARGET_PAGE_MASK)) {
>> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>> +        return -EINVAL;
>> +    }
>> +    if (!address_space_access_valid(&address_space_memory, addr,
>> +                                    sizeof(IplParameterBlock), write,
>> +                                    MEMTXATTRS_UNSPECIFIED)) {
>> +        s390_program_interrupt(env, PGM_ADDRESSING, ra);
>> +        return -EINVAL;
>> +    }
>> +    return 0;
>> +}
>> +
>>   void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>>   {
>>       CPUState *cs = env_cpu(env);
>> @@ -65,30 +88,24 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>>           return;
>>       }
>>   
>> -    if ((subcode & ~0x0ffffULL) || (subcode > 6)) {
>> +    if (subcode & ~0x0ffffULL) {
> 
> Strange, the default case in the switch was basically dead code.
> 
>>           s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>>           return;
>>       }
>>   
>>       switch (subcode) {
>> -    case 0:
>> +    case DIAG308_RES_MOD_CLR:
>>           s390_ipl_reset_request(cs, S390_RESET_MODIFIED_CLEAR);
>>           break;
>> -    case 1:
>> +    case DIAG308_RES_LOAD_NORM:
>>           s390_ipl_reset_request(cs, S390_RESET_LOAD_NORMAL);
>>           break;
>> -    case 3:
>> +    case DIAG308_LOAD_CLEAR:
>> +        /* Well we still lack the clearing bit... */
>>           s390_ipl_reset_request(cs, S390_RESET_REIPL);
>>           break;
>> -    case 5:
>> -        if ((r1 & 1) || (addr & 0x0fffULL)) {
>> -            s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>> -            return;
>> -        }
>> -        if (!address_space_access_valid(&address_space_memory, addr,
>> -                                        sizeof(IplParameterBlock), false,
>> -                                        MEMTXATTRS_UNSPECIFIED)) {
>> -            s390_program_interrupt(env, PGM_ADDRESSING, ra);
>> +    case DIAG308_SET:
>> +        if (diag308_parm_check(env, r1, addr, ra, false)) {
>>               return;
>>           }
>>           iplb = g_new0(IplParameterBlock, 1);
>> @@ -110,15 +127,8 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>>   out:
>>           g_free(iplb);
>>           return;
>> -    case 6:
>> -        if ((r1 & 1) || (addr & 0x0fffULL)) {
>> -            s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>> -            return;
>> -        }
>> -        if (!address_space_access_valid(&address_space_memory, addr,
>> -                                        sizeof(IplParameterBlock), true,
>> -                                        MEMTXATTRS_UNSPECIFIED)) {
>> -            s390_program_interrupt(env, PGM_ADDRESSING, ra);
>> +    case DIAG308_STORE:
>> +        if (diag308_parm_check(env, r1, addr, ra, true)) {
>>               return;
>>           }
>>           iplb = s390_ipl_get_iplb();
>>
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks!
Thomas Huth Nov. 21, 2019, 1:12 p.m. UTC | #5
On 21/11/2019 12.21, David Hildenbrand wrote:
> On 20.11.19 12:43, Janosch Frank wrote:
>> Let's improve readability by:
>> * Using constants for the subcodes
>> * Moving parameter checking into a function
>> * Removing subcode > 6 check as the default case catches that
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   target/s390x/diag.c | 54 +++++++++++++++++++++++++++------------------
>>   1 file changed, 32 insertions(+), 22 deletions(-)
>>
>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
>> index 53c2f81f2a..067c667ba7 100644
>> --- a/target/s390x/diag.c
>> +++ b/target/s390x/diag.c
>> @@ -53,6 +53,29 @@ 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
>>   +#define DIAG308_RES_MOD_CLR        0
>> +#define DIAG308_RES_LOAD_NORM        1
>> +#define DIAG308_LOAD_CLEAR        3
>> +#define DIAG308_LOAD_NORMAL_DUMP    4
>> +#define DIAG308_SET            5
>> +#define DIAG308_STORE            6
>> +
>> +static int diag308_parm_check(CPUS390XState *env, uint64_t r1,
>> uint64_t addr,
>> +                              uintptr_t ra, bool write)
>> +{
>> +    if ((r1 & 1) || (addr & ~TARGET_PAGE_MASK)) {
>> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>> +        return -EINVAL;
>> +    }
>> +    if (!address_space_access_valid(&address_space_memory, addr,
>> +                                    sizeof(IplParameterBlock), write,
>> +                                    MEMTXATTRS_UNSPECIFIED)) {
>> +        s390_program_interrupt(env, PGM_ADDRESSING, ra);
>> +        return -EINVAL;
>> +    }
>> +    return 0;
>> +}
>> +
>>   void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3,
>> uintptr_t ra)
>>   {
>>       CPUState *cs = env_cpu(env);
>> @@ -65,30 +88,24 @@ void handle_diag_308(CPUS390XState *env, uint64_t
>> r1, uint64_t r3, uintptr_t ra)
>>           return;
>>       }
>>   -    if ((subcode & ~0x0ffffULL) || (subcode > 6)) {
>> +    if (subcode & ~0x0ffffULL) {
> 
> Strange, the default case in the switch was basically dead code.

Not really, case 2 and case 4 are not handled there. We even had a funny
bug some months ago, where a guest could terminate QEMU with one of
these codes, see commit 37dbd1f4d4805edcd18d94eb202bb3461b3cd52d.

 Thomas
Thomas Huth Nov. 21, 2019, 1:20 p.m. UTC | #6
On 20/11/2019 12.43, Janosch Frank wrote:
> Let's improve readability by:
> * Using constants for the subcodes
> * Moving parameter checking into a function
> * Removing subcode > 6 check as the default case catches that
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/diag.c | 54 +++++++++++++++++++++++++++------------------
>  1 file changed, 32 insertions(+), 22 deletions(-)
> 
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index 53c2f81f2a..067c667ba7 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -53,6 +53,29 @@ 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
>  
> +#define DIAG308_RES_MOD_CLR		0
> +#define DIAG308_RES_LOAD_NORM		1

I think I'd also prefer RESET instead of RES here.

> +#define DIAG308_LOAD_CLEAR		3
> +#define DIAG308_LOAD_NORMAL_DUMP	4
> +#define DIAG308_SET			5
> +#define DIAG308_STORE			6
> +
> +static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
> +                              uintptr_t ra, bool write)
> +{
> +    if ((r1 & 1) || (addr & ~TARGET_PAGE_MASK)) {
> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> +        return -EINVAL;
> +    }
> +    if (!address_space_access_valid(&address_space_memory, addr,
> +                                    sizeof(IplParameterBlock), write,
> +                                    MEMTXATTRS_UNSPECIFIED)) {
> +        s390_program_interrupt(env, PGM_ADDRESSING, ra);
> +        return -EINVAL;

or maybe -EFAULT ? ;-)

> +    }
> +    return 0;
> +}
> +
>  void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>  {
>      CPUState *cs = env_cpu(env);
> @@ -65,30 +88,24 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>          return;
>      }
>  
> -    if ((subcode & ~0x0ffffULL) || (subcode > 6)) {
> +    if (subcode & ~0x0ffffULL) {
>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>          return;
>      }
>  
>      switch (subcode) {
> -    case 0:
> +    case DIAG308_RES_MOD_CLR:
>          s390_ipl_reset_request(cs, S390_RESET_MODIFIED_CLEAR);
>          break;
> -    case 1:
> +    case DIAG308_RES_LOAD_NORM:
>          s390_ipl_reset_request(cs, S390_RESET_LOAD_NORMAL);
>          break;
> -    case 3:
> +    case DIAG308_LOAD_CLEAR:
> +        /* Well we still lack the clearing bit... */
>          s390_ipl_reset_request(cs, S390_RESET_REIPL);
>          break;
> -    case 5:
> -        if ((r1 & 1) || (addr & 0x0fffULL)) {
> -            s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> -            return;
> -        }
> -        if (!address_space_access_valid(&address_space_memory, addr,
> -                                        sizeof(IplParameterBlock), false,
> -                                        MEMTXATTRS_UNSPECIFIED)) {
> -            s390_program_interrupt(env, PGM_ADDRESSING, ra);
> +    case DIAG308_SET:
> +        if (diag308_parm_check(env, r1, addr, ra, false)) {
>              return;
>          }
>          iplb = g_new0(IplParameterBlock, 1);
> @@ -110,15 +127,8 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>  out:
>          g_free(iplb);
>          return;
> -    case 6:
> -        if ((r1 & 1) || (addr & 0x0fffULL)) {
> -            s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> -            return;
> -        }
> -        if (!address_space_access_valid(&address_space_memory, addr,
> -                                        sizeof(IplParameterBlock), true,
> -                                        MEMTXATTRS_UNSPECIFIED)) {
> -            s390_program_interrupt(env, PGM_ADDRESSING, ra);
> +    case DIAG308_STORE:
> +        if (diag308_parm_check(env, r1, addr, ra, true)) {
>              return;
>          }
>          iplb = s390_ipl_get_iplb();
> 

With RESET instead of RES:

Reviewed-by: Thomas Huth <thuth@redhat.com>
Janosch Frank Nov. 21, 2019, 1:53 p.m. UTC | #7
On 11/21/19 2:20 PM, Thomas Huth wrote:
> On 20/11/2019 12.43, Janosch Frank wrote:
>> Let's improve readability by:
>> * Using constants for the subcodes
>> * Moving parameter checking into a function
>> * Removing subcode > 6 check as the default case catches that
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  target/s390x/diag.c | 54 +++++++++++++++++++++++++++------------------
>>  1 file changed, 32 insertions(+), 22 deletions(-)
>>
>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
>> index 53c2f81f2a..067c667ba7 100644
>> --- a/target/s390x/diag.c
>> +++ b/target/s390x/diag.c
>> @@ -53,6 +53,29 @@ 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
>>  
>> +#define DIAG308_RES_MOD_CLR		0
>> +#define DIAG308_RES_LOAD_NORM		1
> 
> I think I'd also prefer RESET instead of RES here.
> 
>> +#define DIAG308_LOAD_CLEAR		3
>> +#define DIAG308_LOAD_NORMAL_DUMP	4
>> +#define DIAG308_SET			5
>> +#define DIAG308_STORE			6
>> +
>> +static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
>> +                              uintptr_t ra, bool write)
>> +{
>> +    if ((r1 & 1) || (addr & ~TARGET_PAGE_MASK)) {
>> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>> +        return -EINVAL;
>> +    }
>> +    if (!address_space_access_valid(&address_space_memory, addr,
>> +                                    sizeof(IplParameterBlock), write,
>> +                                    MEMTXATTRS_UNSPECIFIED)) {
>> +        s390_program_interrupt(env, PGM_ADDRESSING, ra);
>> +        return -EINVAL;
> 
> or maybe -EFAULT ? ;-)

Honestly, I'm asking myself why I'm even bothering to return specific
error codes when all I check is rc != 0. So I guess I just make it -1
for all errors.

> 
>> +    }
>> +    return 0;
>> +}
>> +
>>  void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>>  {
>>      CPUState *cs = env_cpu(env);
>> @@ -65,30 +88,24 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>>          return;
>>      }
>>  
>> -    if ((subcode & ~0x0ffffULL) || (subcode > 6)) {
>> +    if (subcode & ~0x0ffffULL) {
>>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>>          return;
>>      }
>>  
>>      switch (subcode) {
>> -    case 0:
>> +    case DIAG308_RES_MOD_CLR:
>>          s390_ipl_reset_request(cs, S390_RESET_MODIFIED_CLEAR);
>>          break;
>> -    case 1:
>> +    case DIAG308_RES_LOAD_NORM:
>>          s390_ipl_reset_request(cs, S390_RESET_LOAD_NORMAL);
>>          break;
>> -    case 3:
>> +    case DIAG308_LOAD_CLEAR:
>> +        /* Well we still lack the clearing bit... */
>>          s390_ipl_reset_request(cs, S390_RESET_REIPL);
>>          break;
>> -    case 5:
>> -        if ((r1 & 1) || (addr & 0x0fffULL)) {
>> -            s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>> -            return;
>> -        }
>> -        if (!address_space_access_valid(&address_space_memory, addr,
>> -                                        sizeof(IplParameterBlock), false,
>> -                                        MEMTXATTRS_UNSPECIFIED)) {
>> -            s390_program_interrupt(env, PGM_ADDRESSING, ra);
>> +    case DIAG308_SET:
>> +        if (diag308_parm_check(env, r1, addr, ra, false)) {
>>              return;
>>          }
>>          iplb = g_new0(IplParameterBlock, 1);
>> @@ -110,15 +127,8 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>>  out:
>>          g_free(iplb);
>>          return;
>> -    case 6:
>> -        if ((r1 & 1) || (addr & 0x0fffULL)) {
>> -            s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>> -            return;
>> -        }
>> -        if (!address_space_access_valid(&address_space_memory, addr,
>> -                                        sizeof(IplParameterBlock), true,
>> -                                        MEMTXATTRS_UNSPECIFIED)) {
>> -            s390_program_interrupt(env, PGM_ADDRESSING, ra);
>> +    case DIAG308_STORE:
>> +        if (diag308_parm_check(env, r1, addr, ra, true)) {
>>              return;
>>          }
>>          iplb = s390_ipl_get_iplb();
>>
> 
> With RESET instead of RES:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>

Patch
diff mbox series

diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index 53c2f81f2a..067c667ba7 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -53,6 +53,29 @@  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
 
+#define DIAG308_RES_MOD_CLR		0
+#define DIAG308_RES_LOAD_NORM		1
+#define DIAG308_LOAD_CLEAR		3
+#define DIAG308_LOAD_NORMAL_DUMP	4
+#define DIAG308_SET			5
+#define DIAG308_STORE			6
+
+static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
+                              uintptr_t ra, bool write)
+{
+    if ((r1 & 1) || (addr & ~TARGET_PAGE_MASK)) {
+        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
+        return -EINVAL;
+    }
+    if (!address_space_access_valid(&address_space_memory, addr,
+                                    sizeof(IplParameterBlock), write,
+                                    MEMTXATTRS_UNSPECIFIED)) {
+        s390_program_interrupt(env, PGM_ADDRESSING, ra);
+        return -EINVAL;
+    }
+    return 0;
+}
+
 void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
 {
     CPUState *cs = env_cpu(env);
@@ -65,30 +88,24 @@  void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
         return;
     }
 
-    if ((subcode & ~0x0ffffULL) || (subcode > 6)) {
+    if (subcode & ~0x0ffffULL) {
         s390_program_interrupt(env, PGM_SPECIFICATION, ra);
         return;
     }
 
     switch (subcode) {
-    case 0:
+    case DIAG308_RES_MOD_CLR:
         s390_ipl_reset_request(cs, S390_RESET_MODIFIED_CLEAR);
         break;
-    case 1:
+    case DIAG308_RES_LOAD_NORM:
         s390_ipl_reset_request(cs, S390_RESET_LOAD_NORMAL);
         break;
-    case 3:
+    case DIAG308_LOAD_CLEAR:
+        /* Well we still lack the clearing bit... */
         s390_ipl_reset_request(cs, S390_RESET_REIPL);
         break;
-    case 5:
-        if ((r1 & 1) || (addr & 0x0fffULL)) {
-            s390_program_interrupt(env, PGM_SPECIFICATION, ra);
-            return;
-        }
-        if (!address_space_access_valid(&address_space_memory, addr,
-                                        sizeof(IplParameterBlock), false,
-                                        MEMTXATTRS_UNSPECIFIED)) {
-            s390_program_interrupt(env, PGM_ADDRESSING, ra);
+    case DIAG308_SET:
+        if (diag308_parm_check(env, r1, addr, ra, false)) {
             return;
         }
         iplb = g_new0(IplParameterBlock, 1);
@@ -110,15 +127,8 @@  void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
 out:
         g_free(iplb);
         return;
-    case 6:
-        if ((r1 & 1) || (addr & 0x0fffULL)) {
-            s390_program_interrupt(env, PGM_SPECIFICATION, ra);
-            return;
-        }
-        if (!address_space_access_valid(&address_space_memory, addr,
-                                        sizeof(IplParameterBlock), true,
-                                        MEMTXATTRS_UNSPECIFIED)) {
-            s390_program_interrupt(env, PGM_ADDRESSING, ra);
+    case DIAG308_STORE:
+        if (diag308_parm_check(env, r1, addr, ra, true)) {
             return;
         }
         iplb = s390_ipl_get_iplb();