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

[David Hildenbrand]

On 10.03.20 14:39, 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

"cannot"

> 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

"separately"

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

[...]

> +static inline bool ipl_valid_pv_components(IplParameterBlock *iplb)
> +{
> +    IPLBlockPV *ipib_pv = &iplb->pv;
> +    int i;
> +
> +    if (ipib_pv->num_comp == 0) {
> +        return false;
> +    }
> +
> +    for (i = 0; i < ipib_pv->num_comp; i++) {
> +        /* Addr must be 4k aligned */
> +        if (ipib_pv->components[i].addr & ~TARGET_PAGE_MASK) {

I usually find

QEMU_IS_ALIGNED(ipib_pv->components[i].addr, TARGET_PAGE_MASK)

nicer

[...]

> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
> new file mode 100644
> index 0000000000..1ba8bc7242
> --- /dev/null
> +++ b/hw/s390x/pv.c
> @@ -0,0 +1,104 @@
> +/*
> + * Protected Virtualization functions
> + *
> + * 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.
> + */
> +#include "qemu/osdep.h"
> +
> +#include <linux/kvm.h>
> +
> +#include "qemu/error-report.h"
> +#include "sysemu/kvm.h"
> +#include "pv.h"
> +
> +const char *cmd_names[] = {
> +    "VM_ENABLE",
> +    "VM_DISABLE",
> +    "VM_SET_SEC_PARAMS",
> +    "VM_UNPACK",
> +    "VM_VERIFY",
> +    "VM_PREP_RESET",
> +    "VM_UNSHARE_ALL",
> +};
> +
> +static int s390_pv_cmd(uint32_t cmd, void *data)
> +{
> +    int rc;

reverse x... :)

> +    struct kvm_pv_cmd pv_cmd = {
> +        .cmd = cmd,
> +        .data = (uint64_t)data,
> +    };
> +

and then maybe

int rc =  ...

> +    rc = kvm_vm_ioctl(kvm_state, KVM_S390_PV_COMMAND, &pv_cmd);
> +    if (rc) {
> +        error_report("KVM PV command %d (%s) failed: header rc %x rrc %x "
> +                     "IOCTL rc: %d", cmd, cmd_names[cmd], pv_cmd.rc, 
> pv_cmd.rrc,
> +                     rc);
> +    }
> +    return rc;
> +}
> +
> +static void s390_pv_cmd_exit(uint32_t cmd, void *data)
> +{
> +    int rc;
> +
> +    rc = s390_pv_cmd(cmd, data);

int rc = ...

> +    if (rc) {
> +        exit(1);
> +    }
> +}
> +

[...]

> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 895498cca6..455ad31718 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -41,6 +41,8 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/s390x/tod.h"
>  #include "sysemu/sysemu.h"
> +#include "hw/s390x/pv.h"
> +#include <linux/kvm.h>
>  
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>  {
> @@ -316,10 +318,80 @@ 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)
> +{
> +

superfluous empty line

> +    s390_pv_vm_disable();
> +    ms->pv = false;
> +}
> +

[...]

> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 3dd396e870..bcb9e47767 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -37,6 +37,8 @@
>  #include "sysemu/hw_accel.h"
>  #include "hw/qdev-properties.h"
>  #ifndef CONFIG_USER_ONLY
> +#include "hw/s390x/s390-virtio-ccw.h"
> +#include "hw/s390x/pv.h"

Do you need that include here? I don't think so.

>  #include "hw/boards.h"
>  #include "sysemu/arch_init.h"
>  #include "sysemu/sysemu.h"
> @@ -174,6 +176,27 @@ static void s390_cpu_disas_set_info(CPUState *cpu, 
> disassemble_info *info)
>      info->print_insn = print_insn_s390;
>  }
>  
> +#if !defined(CONFIG_USER_ONLY)
> +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
> +

Nit: I *think* this would be better placed in hw/s390x/pv.h

>  static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 1d17709d6e..8fa25d9ed5 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -762,6 +762,7 @@ static inline void s390_do_cpu_load_normal(CPUState *cs, 
> run_on_cpu_data arg)
>  
>  
>  /* cpu.c */
> +bool s390_is_pv(void);

Wonder if this will compile with CONFIG_USER_ONLY (maybe we need a
"return false" dummy). I assume you test-compiled that with all variants
(CONFIG_TCG, CONFIG_USER_ONLY, ...)


.. we're almost there ... :)

-- 
Thanks,

David / dhildenb