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 <[email protected]> >> --- >> 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 <[email protected]>
>
signature.asc
Description: OpenPGP digital signature
