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

[David Hildenbrand]

>>
>>> +    CPUS390XState *env;
>>>  
>>>      /* get the reset parameters, reset them once done */
>>>      s390_ipl_get_reset_request(&cs, &reset_type);
>>> @@ -327,9 +411,16 @@ static void s390_machine_reset(MachineState *machine)
>>>      /* all CPUs are paused and synchronized at this point */
>>>      s390_cmma_reset();
>>>  
>>> +    cpu = S390_CPU(cs);
>>> +    env = &cpu->env;
>>
>> Can you just pass "cpu" to s390_pv_prepare_reset() and handle it in there?
> 
> Wouldn't it make more sense to test the machine state here anyway?

Whatever you prefer. Just saying, that introducing "CPUS390XState *env"
in this function can be avoided.

> 
>>
>>> +
>>>      switch (reset_type) {
>>>      case S390_RESET_EXTERNAL:
>>>      case S390_RESET_REIPL:
>>> +        if (ms->pv) {
>>> +            s390_machine_unprotect(ms);
>>> +        }
>>> +
>>>          qemu_devices_reset();
>>>          s390_crypto_reset();
>>>  
>>> @@ -337,22 +428,52 @@ static void s390_machine_reset(MachineState *machine)
>>>          run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
>>>          break;
>>>      case S390_RESET_MODIFIED_CLEAR:
>>> +        /*
>>> +         * Susbsystem reset needs to be done before we unshare memory
>>> +         * and loose access to VIRTIO structures in guest memory.
>>> +         */
>>> +        subsystem_reset();
>>> +        s390_crypto_reset();
>>> +        s390_pv_prepare_reset(env);
>>>          CPU_FOREACH(t) {
>>>              run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
>>>          }
>>> -        subsystem_reset();
>>> -        s390_crypto_reset();
>>>          run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>>>          break;
>>>      case S390_RESET_LOAD_NORMAL:
>>> +        /*
>>> +         * Susbsystem reset needs to be done before we unshare memory
>>> +         * and loose access to VIRTIO structures in guest memory.
>>> +         */
>>> +        subsystem_reset();
>>> +        s390_pv_prepare_reset(env);
>>>          CPU_FOREACH(t) {
>>>              if (t == cs) {
>>>                  continue;
>>>              }
>>>              run_on_cpu(t, s390_do_cpu_reset, RUN_ON_CPU_NULL);
>>>          }
>>> -        subsystem_reset();
>>>          run_on_cpu(cs, s390_do_cpu_initial_reset, RUN_ON_CPU_NULL);
>>> +        run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>>> +        break;
>>> +    case S390_RESET_PV: /* Subcode 10 */
>>> +        subsystem_reset();
>>> +        s390_crypto_reset();
>>> +
>>> +        CPU_FOREACH(t) {
>>> +            if (t == cs) {
>>> +                continue;
>>> +            }
>>> +            run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
>>> +        }
>>> +        run_on_cpu(cs, s390_do_cpu_reset, RUN_ON_CPU_NULL);
>>> +
>>> +        if (s390_machine_protect(ms)) {
>>> +            s390_machine_inject_pv_error(cs);
>>> +            s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>>> +            return;
>>> +        }
>>> +
>>>          run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>>
>> [...]
>>
>>>  
>>> +#if !defined(CONFIG_USER_ONLY)
>>> +static bool machine_is_pv(MachineState *ms)
>>> +{
>>> +    Object *obj;
>>> +
>>> +    /* we have to bail out for the "none" machine */
>>> +    obj = object_dynamic_cast(OBJECT(ms), TYPE_S390_CCW_MACHINE);
>>> +     if (!obj) {
>>> +        return false;
>>> +    }
>>> +    return S390_CCW_MACHINE(obj)->pv;
>>
>> Maybe you want to cache the machine, so you can avoid the
>> lookup+conversion on every new CPU.
> 
> Christian has provided me with a fix to this code, I'll squash it.
> 
>>
>>> +}
>>> +#endif
>>
>> [...]
>>>  static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t 
>>> addr,
>>>                                uintptr_t ra, bool write)
>>>  {
>>> +    /* Handled by the Ultravisor */
>>> +    if (env->pv) {
>>> +        return 0;
>>> +    }
>>>      if ((r1 & 1) || (addr & ~TARGET_PAGE_MASK)) {
>>>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>>>          return -1;
>>> @@ -93,6 +101,11 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, 
>>> uint64_t r3, uintptr_t ra)
>>>          return;
>>>      }
>>>  
>>> +    if (subcode > 7 && !s390_has_feat(S390_FEAT_UNPACK)) {
>>
>>> = DIAG308_PV_SET
> 
> ?

Instead of the magic number seven, check against "subcode >= DIAG308_PV_SET"

-- 
Thanks,

David / dhildenb