Re: [PATCH 02/15] s390x: Beautify diag308 handling

[Thomas Huth]

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 <address@hidden>
> ---
>  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 <address@hidden>