Re: [PATCH v8 2/2] s390x: protvirt: Support unpack facility

[David Hildenbrand]

>>> +    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);
>>> +            /*
>>> +             * Continue after the diag308 so the guest knows something
>>> +             * went wrong.
>>> +             */
>>> +            s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>>> +            return;
>>
>> Didn't you want to squash in that hunk from the other patch? (I remember
>> seeing a goto)
> 
> I chose to leave it this way so we don't jump around with the goto

Fine with me as long as the other patch won't touch this code anymore ;)

>>>  #if !defined(CONFIG_USER_ONLY)
>>>      uint32_t core_id; /* PoP "CPU address", same as cpu_index */
>>> diff --git a/target/s390x/cpu_features_def.inc.h 
>>> b/target/s390x/cpu_features_def.inc.h
>>> index 31dff0d84e..60db28351d 100644
>>> --- a/target/s390x/cpu_features_def.inc.h
>>> +++ b/target/s390x/cpu_features_def.inc.h
>>> @@ -107,6 +107,7 @@ DEF_FEAT(DEFLATE_BASE, "deflate-base", STFL, 151, 
>>> "Deflate-conversion facility (
>>>  DEF_FEAT(VECTOR_PACKED_DECIMAL_ENH, "vxpdeh", STFL, 152, 
>>> "Vector-Packed-Decimal-Enhancement Facility")
>>>  DEF_FEAT(MSA_EXT_9, "msa9-base", STFL, 155, 
>>> "Message-security-assist-extension-9 facility (excluding subfunctions)")
>>>  DEF_FEAT(ETOKEN, "etoken", STFL, 156, "Etoken facility")
>>> +DEF_FEAT(UNPACK, "unpack", STFL, 161, "Unpack facility")
>>
>> Random thought: The naming of that facility could have been improved to
>> something that gives users/readers an idea what it's actually doing.
> 
> That's the name from our specifications and with those feature bits we
> normally use the facility names from those docs, no?
> Something like "protected guest boot facility" would certainly have been
> better to understand.

Yeah, I was not talking about bad naming from the HW/FW folks for that
facility. Not your fault.


>> [...]
>>
>>> @@ -128,17 +142,31 @@ 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_pv();
>>> +        } 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;
>>>          } else {
>>>              env->regs[r1 + 1] = DIAG_308_RC_NO_CONF;
>>>          }
>>> -        return;
>>> +        break;
>>
>> return->break is unrelated, but we do have a wild mixture already.
> 
> I removed it here and in the move diag structures over SIDA patch.
> After this has been merged I can do a cleanup patch if wanted.


Whatever you prefer.


-- 
Thanks,

David / dhildenb