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) {
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();
--
2.20.1
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 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 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,
David / dhildenb
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>
>
© 2016 - 2026 Red Hat, Inc.