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 <fran...@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 <th...@redhat.com>
