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

[Cornelia Huck]

On Mon, 16 Mar 2020 16:05:03 +0100
Janosch Frank <address@hidden> wrote:

> On 3/16/20 3:54 PM, Cornelia Huck wrote:
> > On Mon, 16 Mar 2020 15:47:41 +0100
> > Janosch Frank <address@hidden> wrote:
> >   
> >> On 3/16/20 3:27 PM, Cornelia Huck wrote:  
> >>> On Fri, 13 Mar 2020 05:52:32 -0400
> >>> Janosch Frank <address@hidden> wrote:
> >>>     
> >>>> Signed-off-by: Janosch Frank <address@hidden>
> >>>> ---
> >>>>  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;
> }
> 

Sorry, you now managed to confuse me... where is that supposed to be
called?

pgptayspJuzuV.pgp
Description: OpenPGP digital signature