Re: [PATCH v2 01/13] s390x: protvirt: Add diag308 subcodes 8 - 10

[Janosch Frank]

On 11/29/19 1:40 PM, Thomas Huth wrote:
> On 29/11/2019 10.47, Janosch Frank wrote:
> [...]
>> Subcodes 8-10 are not valid in protected mode, we have to do a subcode
>> 3 and then the 8 and 10 combination for a protected reboot.
> 
> So if 8-10 are not valid in protected mode...
> 
>> @@ -59,6 +61,9 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, 
>> uint64_t r3)
>>  #define DIAG308_LOAD_NORMAL_DUMP    4
>>  #define DIAG308_SET                 5
>>  #define DIAG308_STORE               6
>> +#define DIAG308_PV_SET              8
>> +#define DIAG308_PV_STORE            9
>> +#define DIAG308_PV_START            10
>>  
>>  static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t 
>> addr,
>>                                uintptr_t ra, bool write)
>> @@ -105,6 +110,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:
> 
> ... should you maybe add a check here (and the other cases) to make sure
> that the guest is currently not running in PV mode? Or is this taken
> care of by the Ultravisor already?

The Ultravisor takes care of that.

> 
>>          if (diag308_parm_check(env, r1, addr, ra, false)) {
>>              return;
>>          }
>> @@ -117,7 +123,8 @@ 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_ccw(iplb) && !iplb_valid_fcp(iplb)) {
>> +        if (!iplb_valid_ccw(iplb) && !iplb_valid_fcp(iplb) &&
>> +            !(iplb_valid_se(iplb) && s390_ipl_pv_check_comp(iplb) >= 0)) {
>>              env->regs[r1 + 1] = DIAG_308_RC_INVALID;
>>              goto out;
>>          }
>> @@ -128,10 +135,15 @@ out:
>>          g_free(iplb);
>>          return;
>>      case DIAG308_STORE:
>> +    case DIAG308_PV_STORE:
>>          if (diag308_parm_check(env, r1, addr, ra, true)) {
>>              return;
>>          }
>> -        iplb = s390_ipl_get_iplb();
>> +        if (subcode == DIAG308_PV_STORE) {
>> +            iplb = s390_ipl_get_iplb_secure();
>> +        } else {
>> +            iplb = s390_ipl_get_iplb();
>> +        }
>>          if (iplb) {
>>              cpu_physical_memory_write(addr, iplb, be32_to_cpu(iplb->len));
>>              env->regs[r1 + 1] = DIAG_308_RC_OK;
>> @@ -139,6 +151,16 @@ out:
>>              env->regs[r1 + 1] = DIAG_308_RC_NO_CONF;
>>          }
>>          return;
>> +        break;
> 
> Please remove the break. Or the return. But let's not do both.

Right, I forgot to remove that...

> 
>> +    case DIAG308_PV_START:
>> +        iplb = s390_ipl_get_iplb_secure();
>> +        if (!iplb_valid_se(iplb)) {
>> +            env->regs[r1 + 1] = DIAG_308_RC_NO_PV_CONF;
>> +            return;
>> +        }
>> +
>> +        s390_ipl_reset_request(cs, S390_RESET_PV);
>> +        break;
>>      default:
>>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>>          break;
>>
> 
>  Thomas
>

signature.asc
Description: OpenPGP digital signature