| Message ID | 20191120114334.2287-3-frankja@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: Protected Virtualization support | expand |
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>
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>
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!
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!
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
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>
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> >
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();
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(-)