Re: [PATCH v9 02/15] s390x: protvirt: Support unpack facility

[Janosch Frank]

On 3/12/20 9:33 AM, Christian Borntraeger wrote:
> 
> 
> On 11.03.20 14:21, Janosch Frank wrote:
> [...]
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index b81942e1e6f9002e..98df89e62c25f583 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -27,6 +27,7 @@
>>  #include "hw/s390x/vfio-ccw.h"
>>  #include "hw/s390x/css.h"
>>  #include "hw/s390x/ebcdic.h"
>> +#include "hw/s390x/pv.h"
>>  #include "ipl.h"
>>  #include "qemu/error-report.h"
>>  #include "qemu/config-file.h"
>> @@ -566,12 +567,31 @@ void s390_ipl_update_diag308(IplParameterBlock *iplb)
>>  {
>>      S390IPLState *ipl = get_ipl_device();
>>  
>> -    ipl->iplb = *iplb;
>> -    ipl->iplb_valid = true;
>> +    /*
>> +     * The IPLB set and retrieved by subcodes 8/9 is completely
>> +     * separate from the one managed via subcodes 5/6.
>> +     */
>> +    if (iplb->pbt == S390_IPL_TYPE_PV) {
>> +        ipl->iplb_pv = *iplb;
>> +        ipl->iplb_valid_pv = true;
>> +    } else {
>> +        ipl->iplb = *iplb;
>> +        ipl->iplb_valid = true;
>> +    }
> 
> We call this for DIAG308_SET and DIAG308_PV_SET in diag.c (see below).
> Doesnt this allow to set S390_IPL_TYPE_PV via subcode 5 and an CCW type
> via subcode 8. It is certainly not an issue security-wise, but it seems to 
> violate
> the architecture.
> Shouldnt we add a check in diag.c?

That would make sense, I'll add it

> 
> [...]
>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> [..]
>> @@ -93,6 +102,11 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, 
>> uint64_t r3, uintptr_t ra)
>>          return;
>>      }
>>  
>> +    if (subcode >= DIAG308_PV_SET && !s390_has_feat(S390_FEAT_UNPACK)) {
>> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>> +        return;
>> +    }
>> +
>>      switch (subcode) {
>>      case DIAG308_RESET_MOD_CLR:
>>          s390_ipl_reset_request(cs, S390_RESET_MODIFIED_CLEAR);
>> @@ -105,6 +119,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, 
>> uint64_t r3, uintptr_t ra)
>>          s390_ipl_reset_request(cs, S390_RESET_REIPL);
>>          break;
>>      case DIAG308_SET:
>> +    case DIAG308_PV_SET:
> 
> somewhere here after we have loaded the block. 
> 
> 
> 
> Other than that this looks good.
> 
>

signature.asc
Description: OpenPGP digital signature