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

[Christian Borntraeger]

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.