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

[Janosch Frank]

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 <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 ? ;-)

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

signature.asc
Description: OpenPGP digital signature