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

[Cornelia Huck]

On Mon, 16 Mar 2020 20:42:33 +0100
Christian Borntraeger <address@hidden> wrote:

> On 16.03.20 18:57, Cornelia Huck wrote:
> > On Mon, 16 Mar 2020 16:04:00 +0100
> > Christian Borntraeger <address@hidden> wrote:
> >   
> >> On 16.03.20 15:54, 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.
> >>>>>       
> >>>>
> >>>> 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.    
> >>
> >> So you suggest to split these case statements?
> >> case DIAG308_STORE:
> >> case DIAG308_PV_STORE:  
> > 
> > Why? Those cases are already done in the way I suggest for these here
> > as well (i.e. keep common checks, just split the iplb handling.)  
> 
> This was more of a question. I was not sure what your suggestion was.

Sorry if I wasn't clear enough.

For the store case, you have

        if (subcode == DIAG308_PV_STORE) {
            iplb = s390_ipl_get_iplb_pv();
        } else {
            iplb = s390_ipl_get_iplb();
        }

with the rest of the handling being identical. My suggestion was to use
something like

        valid = subcode == DIAG308_PV_SET ? iplb_valid_pv(iplb) : 
iplb_valid(iplb);
        if (!valid) {
             env->regs[r1 + 1] = DIAG_308_RC_INVALID;
             goto out;
         }