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

[Janosch Frank]

On 3/10/20 10:00 AM, David Hildenbrand wrote:
> On 10.03.20 09:32, Janosch Frank wrote:
>> The unpack facility provides the means to setup a protected guest. A
>> protected guest can not be introspected by the hypervisor or any
>> user/administrator of the machine it is running on.
>>
>> Protected guests are encrypted at rest and need a special boot
>> mechanism via diag308 subcode 8 and 10.
>>
>> Code 8 sets the PV specific IPLB which is retained seperately from
>> those set via code 5.
>>
>> Code 10 is used to unpack the VM into protected memory, verify its
>> integrity and start it.
>>
>> Signed-off-by: Janosch Frank <address@hidden>
>> Co-developed-by: Christian Borntraeger <address@hidden> [Changes
>> to machine]
> 
> [...]
> 
> 
>> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
>> new file mode 100644
>> index 0000000000..ba6409246e
>> --- /dev/null
>> +++ b/hw/s390x/pv.c
>> @@ -0,0 +1,104 @@
>> +/*
>> + * Secure execution functions
> 
> Protected virtualization ;)

Ack

> 
>> + *
>> + * Copyright IBM Corp. 2020
>> + * Author(s):
>> + *  Janosch Frank <address@hidden>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>> + * your option) any later version. See the COPYING file in the top-level
>> + * directory.
>> + */
> 
> [...]
> 
>>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>>  {
>> @@ -238,9 +240,11 @@ static void s390_create_sclpconsole(const char *type, 
>> Chardev *chardev)
>>  static void ccw_init(MachineState *machine)
>>  {
>>      int ret;
>> +    S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
>>      VirtualCssBus *css_bus;
>>      DeviceState *dev;
>>  
>> +    ms->pv = false;
> 
> As discussed, not needed.

Ack

> 
>>      s390_sclp_init();
>>      /* init memory + setup max page size. Required for the CPU model */
>>      s390_memory_init(machine->ram);
>> @@ -316,10 +320,88 @@ static inline void s390_do_cpu_ipl(CPUState *cs, 
>> run_on_cpu_data arg)
>>      s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>>  }
>>  
>> +static void s390_machine_unprotect(S390CcwMachineState *ms)
>> +{
>> +    CPUState *t;
>> +
>> +    s390_pv_vm_disable();
>> +    CPU_FOREACH(t) {
>> +        S390_CPU(t)->env.pv = false;
>> +    }
> 
> See below, would be great if we can get rid of env.pv IMHO.
> 
> [...]

I'll have a look

> 
>> +    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

> 
>> +        }
>> +
>>          run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>>          break;
>>      default:
> 
> [...]
> 
>>  
>> +#if !defined(CONFIG_USER_ONLY)
>> +static bool machine_is_pv(MachineState *ms)
>> +{
>> +    static S390CcwMachineState *ccw;
>> +    Object *obj;
>> +
>> +    if (ccw)
>> +        return ccw->pv;
> 
> missing {}
> 
>> +
>> +    /* we have to bail out for the "none" machine */
>> +    obj = object_dynamic_cast(OBJECT(ms), TYPE_S390_CCW_MACHINE);
>> +     if (!obj) {
>> +        return false;
>> +    }
>> +    ccw = S390_CCW_MACHINE(obj);
>> +    return ccw->pv;
>> +}
>> +#endif
> 
> Now that we talked about cached values, what about
> 
> #if !defined(CONFIG_USER_ONLY)
> static bool s390_is_pv(void)
> {
>     static S390CcwMachineState *ccw;
>     Object *obj;
> 
>     if (ccw) {
>         return ccw->pv;
>     }
> 
>     /* we have to bail out for the "none" machine */
>     obj = object_dynamic_cast(qdev_get_machine(),
>                               TYPE_S390_CCW_MACHINE);
>     if (!obj) {
>         return false;
>     }
>     ccw = S390_CCW_MACHINE(obj);
>     return ccw->pv;
> }
> #endif
> 
> and drop all env->pv checks, replacing them by s390_is_pv(). (sorry,
> should have recommended that earlier)

Fine by me.
@Christian: Opinions?

> 
>> +
>>  static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>>  {
>>      CPUState *cs = CPU(dev);
>> @@ -205,6 +226,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
>> **errp)
>>          goto out;
>>      }
>>  
>> +    cpu->env.pv = machine_is_pv(ms);
>>      /* sync cs->cpu_index and env->core_id. The latter is needed for TCG. */
>>      cs->cpu_index = cpu->env.core_id;
>>  #endif
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index 1d17709d6e..7e4d9d267c 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -114,6 +114,7 @@ struct CPUS390XState {
>>  
>>      /* Fields up to this point are cleared by a CPU reset */
>>      struct {} end_reset_fields;
>> +    bool pv; /* protected virtualization */
>>  
>>  #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.

> 
> 
> [...]
> 
>> @@ -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.

> 
>> +    case DIAG308_PV_START:
>> +        iplb = s390_ipl_get_iplb_pv();
>> +        if (!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;
>>
> 
>

signature.asc
Description: OpenPGP digital signature