On 3/16/20 3:54 PM, Cornelia Huck wrote: > On Mon, 16 Mar 2020 15:47:41 +0100 > Janosch Frank <[email protected]> wrote: > >> On 3/16/20 3:27 PM, Cornelia Huck wrote: >>> On Fri, 13 Mar 2020 05:52:32 -0400 >>> Janosch Frank <[email protected]> wrote: >>> >>>> Signed-off-by: Janosch Frank <[email protected]> >>>> --- >>>> hw/s390x/ipl.h | 11 +++++++---- >>>> target/s390x/diag.c | 2 +- >>>> 2 files changed, 8 insertions(+), 5 deletions(-) > > >>>> @@ -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. >>> >>S390_IPL_TYPE_CCW >> The idea was to get rid of all of these ifs and elses and only have one >> iplb_valid function. Your suggestion would defeat hiding that complexity >> behind this function. > > I'd argue that this is a complexity we should not hide; for non-pv, we > can have several formats, for pv, only one, and we cannot use a pv iplb > in a non-pv context and vice versa. >
Ok, then please let me split this out into a new function within diag.c.
Something like:
static bool diag308_pbt_subcode_validity(uint8_t pbt, uint64_t subcode)
{
if (subcode == DIAG308_SET) {
return (pbt == S390_IPL_TYPE_FCP || pbt == S390_IPL_TYPE_CCW)
} else if (subcode == DIAG308_PV_SET && pbt == S390_IPL_TYPE_PV) {
return true;
}
return false;
}
signature.asc
Description: OpenPGP digital signature
