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

[Janosch Frank]

On 3/10/20 4:41 PM, Christian Borntraeger wrote:
> 
> 
> 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
>> 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]
>> ---
>>  hw/s390x/Makefile.objs              |   1 +
>>  hw/s390x/ipl.c                      |  56 ++++++++++++-
>>  hw/s390x/ipl.h                      |  79 +++++++++++++++++++
>>  hw/s390x/pv.c                       | 104 ++++++++++++++++++++++++
>>  hw/s390x/pv.h                       |  33 ++++++++
>>  hw/s390x/s390-virtio-ccw.c          | 118 +++++++++++++++++++++++++++-
>>  include/hw/s390x/s390-virtio-ccw.h  |   1 +
>>  target/s390x/cpu.c                  |  23 ++++++
>>  target/s390x/cpu.h                  |   1 +
>>  target/s390x/cpu_features_def.inc.h |   1 +
>>  target/s390x/diag.c                 |  30 ++++++-
>>  11 files changed, 441 insertions(+), 6 deletions(-)
>>  create mode 100644 hw/s390x/pv.c
>>  create mode 100644 hw/s390x/pv.h
>>
>> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
>> index e02ed80b68..a46a1c7894 100644
>> --- a/hw/s390x/Makefile.objs
>> +++ b/hw/s390x/Makefile.objs
>> @@ -31,6 +31,7 @@ obj-y += tod-qemu.o
>>  obj-$(CONFIG_KVM) += tod-kvm.o
>>  obj-$(CONFIG_KVM) += s390-skeys-kvm.o
>>  obj-$(CONFIG_KVM) += s390-stattrib-kvm.o
>> +obj-$(CONFIG_KVM) += pv.o
>>  obj-y += s390-ccw.o
>>  obj-y += ap-device.o
>>  obj-y += ap-bridge.o
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index b81942e1e6..b88b8ff346 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -33,6 +33,7 @@
>>  #include "qemu/cutils.h"
>>  #include "qemu/option.h"
>>  #include "exec/exec-all.h"
>> +#include "pv.h"
>>  
>>  #define KERN_IMAGE_START                0x010000UL
>>  #define LINUX_MAGIC_ADDR                0x010008UL
>> @@ -566,12 +567,31 @@ void s390_ipl_update_diag308(IplParameterBlock *iplb)
>>  {
>>      S390IPLState *ipl = get_ipl_device();
>>  
>> -    ipl->iplb = *iplb;
>> -    ipl->iplb_valid = true;
>> +    /*
>> +     * The IPLB set and retrieved by subcodes 8/9 is completely
>> +     * separate from the one managed via subcodes 5/6.
>> +     */
>> +    if (iplb->pbt == S390_IPL_TYPE_PV) {
>> +        ipl->iplb_pv = *iplb;
>> +        ipl->iplb_valid_pv = true;
>> +    } else {
>> +        ipl->iplb = *iplb;
>> +        ipl->iplb_valid = true;
>> +    }
>>      ipl->netboot = is_virtio_net_device(iplb);
>>      update_machine_ipl_properties(iplb);
>>  }
>>  
>> +IplParameterBlock *s390_ipl_get_iplb_pv(void)
>> +{
>> +    S390IPLState *ipl = get_ipl_device();
>> +
>> +    if (!ipl->iplb_valid_pv) {
>> +        return NULL;
>> +    }
>> +    return &ipl->iplb_pv;
>> +}
>> +
>>  IplParameterBlock *s390_ipl_get_iplb(void)
>>  {
>>      S390IPLState *ipl = get_ipl_device();
>> @@ -660,6 +680,38 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu)
>>      cpu_physical_memory_unmap(addr, len, 1, len);
>>  }
>>  
>> +int s390_ipl_prepare_pv_header(void)
>> +{
>> +    IplParameterBlock *ipib = s390_ipl_get_iplb_pv();
>> +    IPLBlockPV *ipib_pv = &ipib->pv;
>> +    void *hdr = g_malloc(ipib_pv->pv_header_len);
>> +    int rc;
>> +
>> +    cpu_physical_memory_read(ipib_pv->pv_header_addr, hdr,
>> +                             ipib_pv->pv_header_len);
>> +    rc = s390_pv_set_sec_parms((uint64_t)hdr,
>> +                               ipib_pv->pv_header_len);
>> +    g_free(hdr);
>> +    return rc;
>> +}
>> +
>> +int s390_ipl_pv_unpack(void)
>> +{
>> +    IplParameterBlock *ipib = s390_ipl_get_iplb_pv();
>> +    IPLBlockPV *ipib_pv = &ipib->pv;
>> +    int i, rc = 0;
>> +
>> +    for (i = 0; i < ipib_pv->num_comp; i++) {
>> +        rc = s390_pv_unpack(ipib_pv->components[i].addr,
>> +                            TARGET_PAGE_ALIGN(ipib_pv->components[i].size),
>> +                            ipib_pv->components[i].tweak_pref);
>> +        if (rc) {
>> +            break;
>> +        }
>> +    }
>> +    return rc;
>> +}
>> +
>>  void s390_ipl_prepare_cpu(S390CPU *cpu)
>>  {
>>      S390IPLState *ipl = get_ipl_device();
>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>> index 3e44abe1c6..919f9e6913 100644
>> --- a/hw/s390x/ipl.h
>> +++ b/hw/s390x/ipl.h
>> @@ -15,6 +15,24 @@
>>  #include "cpu.h"
>>  #include "hw/qdev-core.h"
>>  
>> +struct IPLBlockPVComp {
>> +    uint64_t tweak_pref;
>> +    uint64_t addr;
>> +    uint64_t size;
>> +} QEMU_PACKED;
>> +typedef struct IPLBlockPVComp IPLBlockPVComp;
>> +
>> +struct IPLBlockPV {
>> +    uint8_t  reserved18[87];    /* 0x18 */
>> +    uint8_t  version;           /* 0x6f */
>> +    uint32_t reserved70;        /* 0x70 */
>> +    uint32_t num_comp;          /* 0x74 */
>> +    uint64_t pv_header_addr;    /* 0x78 */
>> +    uint64_t pv_header_len;     /* 0x80 */
>> +    struct IPLBlockPVComp components[];
>> +} QEMU_PACKED;
>> +typedef struct IPLBlockPV IPLBlockPV;
>> +
>>  struct IplBlockCcw {
>>      uint8_t  reserved0[85];
>>      uint8_t  ssid;
>> @@ -71,6 +89,7 @@ union IplParameterBlock {
>>          union {
>>              IplBlockCcw ccw;
>>              IplBlockFcp fcp;
>> +            IPLBlockPV pv;
>>              IplBlockQemuScsi scsi;
>>          };
>>      } QEMU_PACKED;
>> @@ -85,8 +104,11 @@ typedef union IplParameterBlock IplParameterBlock;
>>  
>>  int s390_ipl_set_loadparm(uint8_t *loadparm);
>>  void s390_ipl_update_diag308(IplParameterBlock *iplb);
>> +int s390_ipl_prepare_pv_header(void);
>> +int s390_ipl_pv_unpack(void);
>>  void s390_ipl_prepare_cpu(S390CPU *cpu);
>>  IplParameterBlock *s390_ipl_get_iplb(void);
>> +IplParameterBlock *s390_ipl_get_iplb_pv(void);
>>  
>>  enum s390_reset {
>>      /* default is a reset not triggered by a CPU e.g. issued by QMP */
>> @@ -94,6 +116,7 @@ enum s390_reset {
>>      S390_RESET_REIPL,
>>      S390_RESET_MODIFIED_CLEAR,
>>      S390_RESET_LOAD_NORMAL,
>> +    S390_RESET_PV,
>>  };
>>  void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type);
>>  void s390_ipl_get_reset_request(CPUState **cs, enum s390_reset *reset_type);
>> @@ -133,6 +156,7 @@ struct S390IPLState {
>>      /*< private >*/
>>      DeviceState parent_obj;
>>      IplParameterBlock iplb;
>> +    IplParameterBlock iplb_pv;
>>      QemuIplParameters qipl;
>>      uint64_t start_addr;
>>      uint64_t compat_start_addr;
>> @@ -140,6 +164,7 @@ struct S390IPLState {
>>      uint64_t compat_bios_start_addr;
>>      bool enforce_bios;
>>      bool iplb_valid;
>> +    bool iplb_valid_pv;
>>      bool netboot;
>>      /* reset related properties don't have to be migrated or reset */
>>      enum s390_reset reset_type;
>> @@ -161,9 +186,11 @@ QEMU_BUILD_BUG_MSG(offsetof(S390IPLState, iplb) & 3, 
>> "alignment of iplb wrong");
>>  
>>  #define S390_IPL_TYPE_FCP 0x00
>>  #define S390_IPL_TYPE_CCW 0x02
>> +#define S390_IPL_TYPE_PV 0x05
>>  #define S390_IPL_TYPE_QEMU_SCSI 0xff
>>  
>>  #define S390_IPLB_HEADER_LEN 8
>> +#define S390_IPLB_MIN_PV_LEN 148
>>  #define S390_IPLB_MIN_CCW_LEN 200
>>  #define S390_IPLB_MIN_FCP_LEN 384
>>  #define S390_IPLB_MIN_QEMU_SCSI_LEN 200
>> @@ -173,6 +200,50 @@ static inline bool iplb_valid_len(IplParameterBlock 
>> *iplb)
>>      return be32_to_cpu(iplb->len) <= sizeof(IplParameterBlock);
>>  }
>>  
>> +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) {
>> +            return false;
>> +        }
>> +
>> +        /* Tweak prefix is monotonically increasing with each component */
>> +        if (i < ipib_pv->num_comp - 1 &&
>> +            ipib_pv->components[i].tweak_pref >=
>> +            ipib_pv->components[i + 1].tweak_pref) {
>> +            return false;
>> +        }
>> +    }
>> +    return true;
>> +}
>> +
>> +static inline bool ipl_valid_pv_header(IplParameterBlock *iplb)
>> +{
>> +        IPLBlockPV *ipib_pv = &iplb->pv;
>> +
>> +        if (ipib_pv->pv_header_len > 2 * TARGET_PAGE_SIZE) {
>> +            return false;
>> +        }
>> +
>> +        if (!address_space_access_valid(&address_space_memory,
>> +                                        ipib_pv->pv_header_addr,
>> +                                        ipib_pv->pv_header_len,
>> +                                        false,
>> +                                        MEMTXATTRS_UNSPECIFIED)) {
>> +            return false;
>> +        }
>> +
>> +        return true;
>> +}
>> +
>>  static inline bool iplb_valid(IplParameterBlock *iplb)
>>  {
>>      switch (iplb->pbt) {
>> @@ -180,6 +251,14 @@ static inline bool iplb_valid(IplParameterBlock *iplb)
>>          return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN;
>>      case S390_IPL_TYPE_CCW:
>>          return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN;
>> +    case S390_IPL_TYPE_PV:
>> +        if (be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN) {
>> +            return false;
>> +        }
>> +        if (!ipl_valid_pv_header(iplb)) {
>> +            return false;
>> +        }
>> +        return ipl_valid_pv_components(iplb);
>>      default:
>>          return false;
>>      }
>> 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",
>> +};
> 
> This is really error-prone (even though this will not change)
> What about something like this? (and no is an acceptable answer)

I'd be ok with that as long as I can add a comment to the macro
explaining what we're doing. And if I can add a newline between rc and
the if in the macro.

> 
> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
> index 1ba8bc7242..fa592513e4 100644
> --- a/hw/s390x/pv.c
> +++ b/hw/s390x/pv.c
> @@ -17,17 +17,7 @@
>  #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)
> +static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data)
>  {
>      int rc;
>      struct kvm_pv_cmd pv_cmd = {
> @@ -38,20 +28,21 @@ static int s390_pv_cmd(uint32_t cmd, void *data)
>      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,
> +                     "IOCTL rc: %d", cmd, cmdname, pv_cmd.rc, pv_cmd.rrc,
>                       rc);
>      }
>      return rc;
>  }
>  
> -static void s390_pv_cmd_exit(uint32_t cmd, void *data)
> -{
> -    int rc;
> +#define s390_pv_cmd(cmd, data) __s390_pv_cmd(cmd, #cmd, data);
>  
> -    rc = s390_pv_cmd(cmd, data);
> -    if (rc) {
> -        exit(1);
> -    }
> +#define s390_pv_cmd_exit(cmd, data)    \
> +{                                      \
> +    int rc;                            \
> +    rc = __s390_pv_cmd(cmd, #cmd, data);\
> +    if (rc) {                          \
> +        exit(1);                       \
> +    }                                  \
>  }
>  
>  int s390_pv_vm_enable(void)
> 
> 
> 
> 
> 
> 
> 
>

signature.asc
Description: OpenPGP digital signature