Re: [PATCH v9] fixup! Fix subcode/pbt

[Cornelia Huck <cohuck@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1543 bytes --]

On Mon, 16 Mar 2020 15:47:41 +0100
Janosch Frank <frankja@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 <frankja@linux.ibm.com> wrote:
> >   
> >> Signed-off-by: Janosch Frank <frankja@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.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]