| Message ID | 20191122075218.23935-4-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: Reset cleanup | expand |
On 22.11.19 08:52, 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> > Reviewed-by: Cornelia Huck <cohuck@redhat.com> > Reviewed-by: Thomas Huth <thuth@redhat.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..18d33c8492 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_RESET_MOD_CLR 0 > +#define DIAG308_RESET_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 -1; > + } > + if (!address_space_access_valid(&address_space_memory, addr, > + sizeof(IplParameterBlock), write, > + MEMTXATTRS_UNSPECIFIED)) { > + s390_program_interrupt(env, PGM_ADDRESSING, ra); > + return -1; > + } > + 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_RESET_MOD_CLR: > s390_ipl_reset_request(cs, S390_RESET_MODIFIED_CLEAR); > break; > - case 1: > + case DIAG308_RESET_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>
diff --git a/target/s390x/diag.c b/target/s390x/diag.c index 53c2f81f2a..18d33c8492 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_RESET_MOD_CLR 0 +#define DIAG308_RESET_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 -1; + } + if (!address_space_access_valid(&address_space_memory, addr, + sizeof(IplParameterBlock), write, + MEMTXATTRS_UNSPECIFIED)) { + s390_program_interrupt(env, PGM_ADDRESSING, ra); + return -1; + } + 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_RESET_MOD_CLR: s390_ipl_reset_request(cs, S390_RESET_MODIFIED_CLEAR); break; - case 1: + case DIAG308_RESET_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();