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

[Christian Borntraeger]

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: