On Fri, 13 Mar 2020 05:52:32 -0400 Janosch Frank <fran...@linux.ibm.com> wrote:
> Signed-off-by: Janosch Frank <fran...@linux.ibm.com> > --- > hw/s390x/ipl.h | 11 +++++++---- > target/s390x/diag.c | 2 +- > 2 files changed, 8 insertions(+), 5 deletions(-) > > diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h > index 95e3183c9cccf8b6..f799f7cfcf4763b1 100644 > --- a/hw/s390x/ipl.h > +++ b/hw/s390x/ipl.h > @@ -261,15 +261,18 @@ static inline bool > ipl_valid_pv_header(IplParameterBlock *iplb) > return true; > } > > -static inline bool iplb_valid(IplParameterBlock *iplb) > +static inline bool iplb_valid(IplParameterBlock *iplb, uint64_t subcode) > { > switch (iplb->pbt) { > case S390_IPL_TYPE_FCP: > - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN; > + return (subcode == DIAG308_SET && > + be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN); > case S390_IPL_TYPE_CCW: > - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN; > + return (subcode == DIAG308_SET && > + be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN); > case S390_IPL_TYPE_PV: > - if (be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN) { > + if (subcode != DIAG308_PV_SET || > + be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN) { > return false; I'm not sure I like passing the subcode here... > } > if (!ipl_valid_pv_header(iplb)) { > diff --git a/target/s390x/diag.c b/target/s390x/diag.c > index b1ca81633b83bbdc..d4f33db5c23c818d 100644 > --- a/target/s390x/diag.c > +++ b/target/s390x/diag.c > @@ -118,7 +118,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, > uint64_t r3, uintptr_t ra) > > cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len)); > > - if (!iplb_valid(iplb)) { > + if (!iplb_valid(iplb, subcode)) { > env->regs[r1 + 1] = DIAG_308_RC_INVALID; > goto out; > } ...because you're basically checking whether you either have a valid normal iplb, or a valid pv iplb, with the two being mutually exclusive, IIUC. So what about introducing iplb_valid_pv and calling that for the pv case? Would be a bit nicer to read, I think, and also matches what you do for the STORE case.