qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Janosch Frank
Subject: Re: [PATCH v9] fixup! Fix subcode/pbt
Date: Mon, 16 Mar 2020 16:05:03 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2

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;
}

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]