On Mon, 16 Mar 2020 15:47:41 +0100 Janosch Frank <fran...@linux.ibm.com> wrote:
> On 3/16/20 3:27 PM, Cornelia Huck wrote: > > 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(-) > >> @@ -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. > > > > 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.
pgpo5viM3JqK4.pgp
Description: OpenPGP digital signature