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

[Janosch Frank]

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(-)
>>
>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>> index 95e3183c9cccf8b6..f799f7cfcf4763b1 100644
>> --- a/hw/s390x/ipl.h
>> +++ b/hw/s390x/ipl.h
>> @@ -261,15 +261,18 @@ static inline bool 
>> ipl_valid_pv_header(IplParameterBlock *iplb)
>>          return true;
>>  }
>>  
>> -static inline bool iplb_valid(IplParameterBlock *iplb)
>> +static inline bool iplb_valid(IplParameterBlock *iplb, uint64_t subcode)
>>  {
>>      switch (iplb->pbt) {
>>      case S390_IPL_TYPE_FCP:
>> -        return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN;
>> +        return (subcode == DIAG308_SET &&
>> +                be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN);
>>      case S390_IPL_TYPE_CCW:
>> -        return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN;
>> +        return (subcode == DIAG308_SET &&
>> +                be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN);
>>      case S390_IPL_TYPE_PV:
>> -        if (be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN) {
>> +        if (subcode != DIAG308_PV_SET ||
>> +            be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN) {
>>              return false;
> 
> I'm not sure I like passing the subcode here...

I could move this to diag.c and call it iplb_valid_for_subcode()

> 
>>          }
>>          if (!ipl_valid_pv_header(iplb)) {
>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
>> index b1ca81633b83bbdc..d4f33db5c23c818d 100644
>> --- a/target/s390x/diag.c
>> +++ b/target/s390x/diag.c
>> @@ -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.

signature.asc
Description: OpenPGP digital signature