* [PATCH v7 01/15] Sync pv
2020-03-09 11:21 [PATCH v7 00/15] s390x: Protected Virtualization support Janosch Frank
@ 2020-03-09 11:21 ` Janosch Frank
2020-03-09 11:21 ` [PATCH v7 02/15] s390x: protvirt: Support unpack facility Janosch Frank
` (13 subsequent siblings)
14 siblings, 0 replies; 45+ messages in thread
From: Janosch Frank @ 2020-03-09 11:21 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
| 45 +++++++++++++++++++++++++++++++++++++--
1 file changed, 43 insertions(+), 2 deletions(-)
--git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 265099100e..c30344ab00 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -474,12 +474,17 @@ struct kvm_s390_mem_op {
__u32 size; /* amount of bytes */
__u32 op; /* type of operation */
__u64 buf; /* buffer in userspace */
- __u8 ar; /* the access register number */
- __u8 reserved[31]; /* should be set to 0 */
+ union {
+ __u8 ar; /* the access register number */
+ __u32 sida_offset; /* offset into the sida */
+ __u8 reserved[32]; /* should be set to 0 */
+ };
};
/* types for kvm_s390_mem_op->op */
#define KVM_S390_MEMOP_LOGICAL_READ 0
#define KVM_S390_MEMOP_LOGICAL_WRITE 1
+#define KVM_S390_MEMOP_SIDA_READ 2
+#define KVM_S390_MEMOP_SIDA_WRITE 3
/* flags for kvm_s390_mem_op->flags */
#define KVM_S390_MEMOP_F_CHECK_ONLY (1ULL << 0)
#define KVM_S390_MEMOP_F_INJECT_EXCEPTION (1ULL << 1)
@@ -1010,6 +1015,7 @@ struct kvm_ppc_resize_hpt {
#define KVM_CAP_ARM_NISV_TO_USER 177
#define KVM_CAP_ARM_INJECT_EXT_DABT 178
#define KVM_CAP_S390_VCPU_RESETS 179
+#define KVM_CAP_S390_PROTECTED 180
#ifdef KVM_CAP_IRQ_ROUTING
@@ -1478,6 +1484,41 @@ struct kvm_enc_region {
#define KVM_S390_NORMAL_RESET _IO(KVMIO, 0xc3)
#define KVM_S390_CLEAR_RESET _IO(KVMIO, 0xc4)
+struct kvm_s390_pv_sec_parm {
+ __u64 origin;
+ __u64 length;
+};
+
+struct kvm_s390_pv_unp {
+ __u64 addr;
+ __u64 size;
+ __u64 tweak;
+};
+
+enum pv_cmd_id {
+ KVM_PV_ENABLE,
+ KVM_PV_DISABLE,
+ KVM_PV_VM_SET_SEC_PARMS,
+ KVM_PV_VM_UNPACK,
+ KVM_PV_VM_VERIFY,
+ KVM_PV_VM_PREP_RESET,
+ KVM_PV_VM_UNSHARE_ALL,
+ KVM_PV_VCPU_CREATE,
+ KVM_PV_VCPU_DESTROY,
+};
+
+struct kvm_pv_cmd {
+ __u32 cmd; /* Command to be executed */
+ __u16 rc; /* Ultravisor return code */
+ __u16 rrc; /* Ultravisor return reason code */
+ __u64 data; /* Data or address */
+ __u32 flags; /* flags for future extensions. Must be 0 for now */
+ __u32 reserved[3];
+};
+
+/* Available with KVM_CAP_S390_PROTECTED */
+#define KVM_S390_PV_COMMAND _IOWR(KVMIO, 0xc5, struct kvm_pv_cmd)
+
/* Secure Encrypted Virtualization command */
enum sev_cmd_id {
/* Guest initialization commands */
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v7 02/15] s390x: protvirt: Support unpack facility
2020-03-09 11:21 [PATCH v7 00/15] s390x: Protected Virtualization support Janosch Frank
2020-03-09 11:21 ` [PATCH v7 01/15] Sync pv Janosch Frank
@ 2020-03-09 11:21 ` Janosch Frank
2020-03-09 13:37 ` David Hildenbrand
2020-03-09 14:28 ` [PATCH v7 02/15] s390x: protvirt: Support unpack facility Viktor Mihajlovski
2020-03-09 11:21 ` [PATCH v7 03/15] s390x: protvirt: Add migration blocker Janosch Frank
` (12 subsequent siblings)
14 siblings, 2 replies; 45+ messages in thread
From: Janosch Frank @ 2020-03-09 11:21 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
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 <frankja@linux.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> [Changes
to machine]
---
hw/s390x/Makefile.objs | 1 +
hw/s390x/ipl.c | 59 ++++++++++++-
hw/s390x/ipl.h | 72 ++++++++++++++--
hw/s390x/pv.c | 104 +++++++++++++++++++++++
hw/s390x/pv.h | 34 ++++++++
hw/s390x/s390-virtio-ccw.c | 127 +++++++++++++++++++++++++++-
include/hw/s390x/s390-virtio-ccw.h | 1 +
target/s390x/cpu.c | 17 ++++
target/s390x/cpu.h | 1 +
target/s390x/cpu_features_def.inc.h | 1 +
target/s390x/diag.c | 34 +++++++-
11 files changed, 436 insertions(+), 15 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 9c1ecd423c..ba3eac30c6 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
@@ -542,11 +543,30 @@ 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);
}
+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();
@@ -561,7 +581,8 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type)
{
S390IPLState *ipl = get_ipl_device();
- if (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL) {
+ if (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL ||
+ reset_type == S390_RESET_PV) {
/* use CPU 0 for full resets */
ipl->reset_cpu_index = 0;
} else {
@@ -635,6 +656,38 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu)
cpu_physical_memory_unmap(addr, len, 1, len);
}
+int s390_ipl_prepare_pv_header(void)
+{
+ S390IPLState *ipl = get_ipl_device();
+ IPLBlockPV *ipib_pv = &ipl->iplb_pv.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)
+{
+ int i, rc = 0;
+ S390IPLState *ipl = get_ipl_device();
+ IPLBlockPV *ipib_pv = &ipl->iplb_pv.pv;
+
+ 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 d4813105db..b2ccdd9dae 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; /* 0x70 */
+ uint64_t pv_header_addr; /* 0x74 */
+ uint64_t pv_header_len; /* 0x7c */
+ 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,16 +200,49 @@ static inline bool iplb_valid_len(IplParameterBlock *iplb)
return be32_to_cpu(iplb->len) <= sizeof(IplParameterBlock);
}
-static inline bool iplb_valid_ccw(IplParameterBlock *iplb)
+static inline bool s390_ipl_pv_check_components(IplParameterBlock *iplb)
{
- return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN &&
- iplb->pbt == S390_IPL_TYPE_CCW;
+ 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 iplb_valid_fcp(IplParameterBlock *iplb)
+static inline bool iplb_valid(IplParameterBlock *iplb)
{
- return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN &&
- iplb->pbt == S390_IPL_TYPE_FCP;
+ switch (iplb->pbt) {
+ case S390_IPL_TYPE_FCP:
+ return (be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN &&
+ iplb->pbt == S390_IPL_TYPE_FCP);
+ case S390_IPL_TYPE_CCW:
+ return (be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN &&
+ iplb->pbt == S390_IPL_TYPE_CCW);
+ case S390_IPL_TYPE_PV:
+ if(be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN ||
+ iplb->pbt != S390_IPL_TYPE_PV) {
+ return false;
+ }
+ return s390_ipl_pv_check_components(iplb);
+ default:
+ return false;
+ }
}
#endif
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
+ *
+ * Copyright IBM Corp. 2020
+ * Author(s):
+ * Janosch Frank <frankja@linux.ibm.com>
+ *
+ * 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;
+ struct kvm_pv_cmd pv_cmd = {
+ .cmd = cmd,
+ .data = (uint64_t)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,
+ rc);
+ }
+ return rc;
+}
+
+static void s390_pv_cmd_exit(uint32_t cmd, void *data)
+{
+ int rc;
+
+ rc = s390_pv_cmd(cmd, data);
+ if (rc) {
+ exit(1);
+ }
+}
+
+int s390_pv_vm_enable(void)
+{
+ return s390_pv_cmd(KVM_PV_ENABLE, NULL);
+}
+
+void s390_pv_vm_disable(void)
+{
+ s390_pv_cmd_exit(KVM_PV_DISABLE, NULL);
+}
+
+int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
+{
+ struct kvm_s390_pv_sec_parm args = {
+ .origin = origin,
+ .length = length,
+ };
+
+ return s390_pv_cmd(KVM_PV_VM_SET_SEC_PARMS, &args);
+}
+
+/*
+ * Called for each component in the SE type IPL parameter block 0.
+ */
+int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak)
+{
+ struct kvm_s390_pv_unp args = {
+ .addr = addr,
+ .size = size,
+ .tweak = tweak,
+ };
+
+ return s390_pv_cmd(KVM_PV_VM_UNPACK, &args);
+}
+
+void s390_pv_perf_clear_reset(void)
+{
+ s390_pv_cmd_exit(KVM_PV_VM_PREP_RESET, NULL);
+}
+
+int s390_pv_verify(void)
+{
+ return s390_pv_cmd(KVM_PV_VM_VERIFY, NULL);
+}
+
+void s390_pv_unshare(void)
+{
+ s390_pv_cmd_exit(KVM_PV_VM_UNSHARE_ALL, NULL);
+}
diff --git a/hw/s390x/pv.h b/hw/s390x/pv.h
new file mode 100644
index 0000000000..e58fbca96a
--- /dev/null
+++ b/hw/s390x/pv.h
@@ -0,0 +1,34 @@
+/*
+ * Protected Virtualization header
+ *
+ * Copyright IBM Corp. 2020
+ * Author(s):
+ * Janosch Frank <frankja@linux.ibm.com>
+ *
+ * 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.
+ */
+
+#ifndef HW_S390_PV_H
+#define HW_S390_PV_H
+
+#ifdef CONFIG_KVM
+int s390_pv_vm_enable(void);
+void s390_pv_vm_disable(void);
+int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
+int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
+void s390_pv_perf_clear_reset(void);
+int s390_pv_verify(void);
+void s390_pv_unshare(void);
+#else
+static inline int s390_pv_vm_enable(void) { return 0; }
+static inline void s390_pv_vm_disable(void) {}
+static inline int s390_pv_set_sec_parms(uint64_t origin, uint64_t length) { return 0; }
+static inline int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak) { return 0; }
+static inline void s390_pv_perf_clear_reset(void) {}
+static inline int s390_pv_verify(void) { return 0; }
+static inline void s390_pv_unshare(void) {}
+#endif
+
+#endif /* HW_S390_PV_H */
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index a89cf4c129..f718cfc591 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)
{
@@ -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;
s390_sclp_init();
/* init memory + setup max page size. Required for the CPU model */
s390_memory_init(machine->ram);
@@ -316,10 +320,90 @@ 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;
+
+ if (ms->pv) {
+ s390_pv_vm_disable();
+ CPU_FOREACH(t) {
+ S390_CPU(t)->env.pv = false;
+ }
+ ms->pv = false;
+ }
+}
+
+static int s390_machine_protect(S390CcwMachineState *ms)
+{
+ CPUState *t;
+ int rc;
+
+ /* Create SE VM */
+ rc = s390_pv_vm_enable();
+ if (rc) {
+ return rc;
+ }
+
+ CPU_FOREACH(t) {
+ S390_CPU(t)->env.pv = true;
+ }
+ ms->pv = true;
+
+ /* Set SE header and unpack */
+ rc = s390_ipl_prepare_pv_header();
+ if (rc) {
+ goto out_err;
+ }
+
+ /* Decrypt image */
+ rc = s390_ipl_pv_unpack();
+ if (rc) {
+ goto out_err;
+ }
+
+ /* Verify integrity */
+ rc = s390_pv_verify();
+ if (rc) {
+ goto out_err;
+ }
+ return rc;
+
+out_err:
+ s390_machine_unprotect(ms);
+ return rc;
+}
+
+#define DIAG_308_RC_INVAL_FOR_PV 0x0a02
+static void s390_machine_inject_pv_error(CPUState *cs)
+{
+ int r1 = (cs->kvm_run->s390_sieic.ipa & 0x00f0) >> 4;
+ CPUS390XState *env = &S390_CPU(cs)->env;
+
+ /* Report that we are unable to enter protected mode */
+ env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV;
+}
+
+static void s390_pv_prepare_reset(CPUS390XState *env)
+{
+ CPUState *cs;
+
+ if (!env->pv) {
+ return;
+ }
+ CPU_FOREACH(cs) {
+ s390_cpu_set_state(S390_CPU_STATE_STOPPED, S390_CPU(cs));
+ }
+ s390_pv_unshare();
+ s390_pv_perf_clear_reset();
+}
+
static void s390_machine_reset(MachineState *machine)
{
enum s390_reset reset_type;
CPUState *cs, *t;
+ S390CPU *cpu;
+ S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
+ 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;
+
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);
break;
default:
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 8aa27199c9..cd1dccc6e3 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -28,6 +28,7 @@ typedef struct S390CcwMachineState {
/*< public >*/
bool aes_key_wrap;
bool dea_key_wrap;
+ bool pv;
uint8_t loadparm[8];
} S390CcwMachineState;
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 3dd396e870..5109b8bbf5 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"
#include "hw/boards.h"
#include "sysemu/arch_init.h"
#include "sysemu/sysemu.h"
@@ -174,6 +176,20 @@ static void s390_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
info->print_insn = print_insn_s390;
}
+#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;
+}
+#endif
+
static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
{
CPUState *cs = CPU(dev);
@@ -205,6 +221,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")
/* Features exposed via SCLP SCCB Byte 80 - 98 (bit numbers relative to byte-80) */
DEF_FEAT(SIE_GSLS, "gsls", SCLP_CONF_CHAR, 40, "SIE: Guest-storage-limit-suppression facility")
diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index b5aec06d6b..670db0eb53 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -52,6 +52,7 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
#define DIAG_308_RC_OK 0x0001
#define DIAG_308_RC_NO_CONF 0x0102
#define DIAG_308_RC_INVALID 0x0402
+#define DIAG_308_RC_NO_PV_CONF 0x0902
#define DIAG308_RESET_MOD_CLR 0
#define DIAG308_RESET_LOAD_NORM 1
@@ -59,10 +60,17 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
#define DIAG308_LOAD_NORMAL_DUMP 4
#define DIAG308_SET 5
#define DIAG308_STORE 6
+#define DIAG308_PV_SET 8
+#define DIAG308_PV_STORE 9
+#define DIAG308_PV_START 10
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)) {
+ s390_program_interrupt(env, PGM_SPECIFICATION, ra);
+ return;
+ }
+
switch (subcode) {
case DIAG308_RESET_MOD_CLR:
s390_ipl_reset_request(cs, S390_RESET_MODIFIED_CLEAR);
@@ -105,6 +118,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
s390_ipl_reset_request(cs, S390_RESET_REIPL);
break;
case DIAG308_SET:
+ case DIAG308_PV_SET:
if (diag308_parm_check(env, r1, addr, ra, false)) {
return;
}
@@ -117,7 +131,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
- if (!iplb_valid_ccw(iplb) && !iplb_valid_fcp(iplb)) {
+ if (!iplb_valid(iplb)) {
env->regs[r1 + 1] = DIAG_308_RC_INVALID;
goto out;
}
@@ -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;
+ 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;
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v7 02/15] s390x: protvirt: Support unpack facility
2020-03-09 11:21 ` [PATCH v7 02/15] s390x: protvirt: Support unpack facility Janosch Frank
@ 2020-03-09 13:37 ` David Hildenbrand
2020-03-09 14:40 ` Christian Borntraeger
` (2 more replies)
2020-03-09 14:28 ` [PATCH v7 02/15] s390x: protvirt: Support unpack facility Viktor Mihajlovski
1 sibling, 3 replies; 45+ messages in thread
From: David Hildenbrand @ 2020-03-09 13:37 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
On 09.03.20 12:21, 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 <frankja@linux.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> [Changes
> to machine]
As you signed this patch off, Maybe this should rather be a Co-developed-by:
[...]
> {
> S390IPLState *ipl = get_ipl_device();
> @@ -561,7 +581,8 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type)
> {
> S390IPLState *ipl = get_ipl_device();
>
> - if (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL) {
> + if (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL ||
> + reset_type == S390_RESET_PV) {
> /* use CPU 0 for full resets */
> ipl->reset_cpu_index = 0;
This looks wrong. In case of an error, you modify the registers of a
theoretically unrelated CPU.
> } else {
> @@ -635,6 +656,38 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu)
> cpu_physical_memory_unmap(addr, len, 1, len);
> }
>
> +int s390_ipl_prepare_pv_header(void)
> +{
> + S390IPLState *ipl = get_ipl_device();
> + IPLBlockPV *ipib_pv = &ipl->iplb_pv.pv;
> + void *hdr = g_malloc(ipib_pv->pv_header_len);
Should there be an upper limit? The guest can allocate quite some memory
this way and theoretically crash the VM.
> + int rc;
> +
> + cpu_physical_memory_read(ipib_pv->pv_header_addr, hdr,
> + ipib_pv->pv_header_len);
Shouldn't we validate if this memory is accessible at all?
> + 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)
> +{
> + int i, rc = 0;
NIT: These declarations last.
> + S390IPLState *ipl = get_ipl_device();
> + IPLBlockPV *ipib_pv = &ipl->iplb_pv.pv;
use s390_ipl_get_iplb_pv() and assert that we don't get NULL?
> +
> + 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 d4813105db..b2ccdd9dae 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; /* 0x70 */
> + uint64_t pv_header_addr; /* 0x74 */
> + uint64_t pv_header_len; /* 0x7c */
> + 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,16 +200,49 @@ static inline bool iplb_valid_len(IplParameterBlock *iplb)
> return be32_to_cpu(iplb->len) <= sizeof(IplParameterBlock);
> }
>
> -static inline bool iplb_valid_ccw(IplParameterBlock *iplb)
> +static inline bool s390_ipl_pv_check_components(IplParameterBlock *iplb)
Still don't like the function name :)
ipl_valid_pv_components() ?
> {
> - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN &&
> - iplb->pbt == S390_IPL_TYPE_CCW;
> + 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 iplb_valid_fcp(IplParameterBlock *iplb)
> +static inline bool iplb_valid(IplParameterBlock *iplb)
> {
> - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN &&
> - iplb->pbt == S390_IPL_TYPE_FCP;
> + switch (iplb->pbt) {
> + case S390_IPL_TYPE_FCP:
> + return (be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN &&
> + iplb->pbt == S390_IPL_TYPE_FCP);
> + case S390_IPL_TYPE_CCW:
> + return (be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN &&
> + iplb->pbt == S390_IPL_TYPE_CCW);
That's a refactoring you could have split out.
> + case S390_IPL_TYPE_PV:
> + if(be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN ||
> + iplb->pbt != S390_IPL_TYPE_PV) {
> + return false;
> + }
> + return s390_ipl_pv_check_components(iplb);
> + default:
> + return false;
> + }
> }
>
> #endif
> 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
> + *
> + * Copyright IBM Corp. 2020
> + * Author(s):
> + * Janosch Frank <frankja@linux.ibm.com>
> + *
> + * 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;
> + struct kvm_pv_cmd pv_cmd = {
> + .cmd = cmd,
> + .data = (uint64_t)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,
> + rc);
> + }
> + return rc;
> +}
> +
> +static void s390_pv_cmd_exit(uint32_t cmd, void *data)
> +{
> + int rc;
> +
> + rc = s390_pv_cmd(cmd, data);
> + if (rc) {
> + exit(1);
> + }
> +}
> +
> +int s390_pv_vm_enable(void)
> +{
> + return s390_pv_cmd(KVM_PV_ENABLE, NULL);
> +}
> +
> +void s390_pv_vm_disable(void)
> +{
> + s390_pv_cmd_exit(KVM_PV_DISABLE, NULL);
> +}
> +
> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
> +{
> + struct kvm_s390_pv_sec_parm args = {
> + .origin = origin,
> + .length = length,
> + };
> +
> + return s390_pv_cmd(KVM_PV_VM_SET_SEC_PARMS, &args);
> +}
> +
> +/*
> + * Called for each component in the SE type IPL parameter block 0.
> + */
> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak)
> +{
> + struct kvm_s390_pv_unp args = {
> + .addr = addr,
> + .size = size,
> + .tweak = tweak,
> + };
> +
> + return s390_pv_cmd(KVM_PV_VM_UNPACK, &args);
> +}
> +
> +void s390_pv_perf_clear_reset(void)
> +{
> + s390_pv_cmd_exit(KVM_PV_VM_PREP_RESET, NULL);
> +}
> +
> +int s390_pv_verify(void)
> +{
> + return s390_pv_cmd(KVM_PV_VM_VERIFY, NULL);
> +}
> +
> +void s390_pv_unshare(void)
> +{
> + s390_pv_cmd_exit(KVM_PV_VM_UNSHARE_ALL, NULL);
> +}
> diff --git a/hw/s390x/pv.h b/hw/s390x/pv.h
> new file mode 100644
> index 0000000000..e58fbca96a
> --- /dev/null
> +++ b/hw/s390x/pv.h
> @@ -0,0 +1,34 @@
> +/*
> + * Protected Virtualization header
> + *
> + * Copyright IBM Corp. 2020
> + * Author(s):
> + * Janosch Frank <frankja@linux.ibm.com>
> + *
> + * 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.
> + */
> +
> +#ifndef HW_S390_PV_H
> +#define HW_S390_PV_H
> +
> +#ifdef CONFIG_KVM
> +int s390_pv_vm_enable(void);
> +void s390_pv_vm_disable(void);
> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
> +void s390_pv_perf_clear_reset(void);
> +int s390_pv_verify(void);
> +void s390_pv_unshare(void);
> +#else
> +static inline int s390_pv_vm_enable(void) { return 0; }
> +static inline void s390_pv_vm_disable(void) {}
> +static inline int s390_pv_set_sec_parms(uint64_t origin, uint64_t length) { return 0; }
> +static inline int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak) { return 0; }
> +static inline void s390_pv_perf_clear_reset(void) {}
> +static inline int s390_pv_verify(void) { return 0; }
> +static inline void s390_pv_unshare(void) {}
> +#endif
> +
> +#endif /* HW_S390_PV_H */
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index a89cf4c129..f718cfc591 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)
> {
> @@ -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;
> s390_sclp_init();
> /* init memory + setup max page size. Required for the CPU model */
> s390_memory_init(machine->ram);
> @@ -316,10 +320,90 @@ 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;
> +
> + if (ms->pv) {
This is always the case. The check for ms->pv dropped (and if other
patches require this, it is to be handled in the caller).
> + s390_pv_vm_disable();
> + CPU_FOREACH(t) {
> + S390_CPU(t)->env.pv = false;
> + }
> + ms->pv = false;
> + }
> +}
> +
> +static int s390_machine_protect(S390CcwMachineState *ms)
> +{
> + CPUState *t;
> + int rc;
> +
> + /* Create SE VM */
> + rc = s390_pv_vm_enable();
> + if (rc) {
> + return rc;
> + }
> +
> + CPU_FOREACH(t) {
> + S390_CPU(t)->env.pv = true;
> + }
> + ms->pv = true;
> +
> + /* Set SE header and unpack */
> + rc = s390_ipl_prepare_pv_header();
> + if (rc) {
> + goto out_err;
> + }
> +
> + /* Decrypt image */
> + rc = s390_ipl_pv_unpack();
> + if (rc) {
> + goto out_err;
> + }
> +
> + /* Verify integrity */
> + rc = s390_pv_verify();
> + if (rc) {
> + goto out_err;
> + }
> + return rc;
> +
> +out_err:
> + s390_machine_unprotect(ms);
> + return rc;
> +}
> +
> +#define DIAG_308_RC_INVAL_FOR_PV 0x0a02
> +static void s390_machine_inject_pv_error(CPUState *cs)
> +{
> + int r1 = (cs->kvm_run->s390_sieic.ipa & 0x00f0) >> 4;
> + CPUS390XState *env = &S390_CPU(cs)->env;
> +
> + /* Report that we are unable to enter protected mode */
> + env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV;
> +}
> +
> +static void s390_pv_prepare_reset(CPUS390XState *env)
> +{
> + CPUState *cs;
> +
> + if (!env->pv) {
> + return;
> + }
> + CPU_FOREACH(cs) {
> + s390_cpu_set_state(S390_CPU_STATE_STOPPED, S390_CPU(cs));
> + }
This is done by the CPU reset in all cases? Why is that needed? (->comment)
> + s390_pv_unshare();
> + s390_pv_perf_clear_reset();
> +}
> +
> static void s390_machine_reset(MachineState *machine)
> {
> enum s390_reset reset_type;
> CPUState *cs, *t;
> + S390CPU *cpu;
> + S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
Nit: Move this to the very top.
> + 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?
> +
> 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.
> +}
> +#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
> + s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> + return;
> + }
> +
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 02/15] s390x: protvirt: Support unpack facility
2020-03-09 13:37 ` David Hildenbrand
@ 2020-03-09 14:40 ` Christian Borntraeger
2020-03-09 14:42 ` David Hildenbrand
2020-03-10 9:27 ` Christian Borntraeger
2020-03-09 15:42 ` Janosch Frank
2020-03-10 8:32 ` [PATCH v8 1/2] s390x: ipl: Consolidate iplb validity check into one function Janosch Frank
2 siblings, 2 replies; 45+ messages in thread
From: Christian Borntraeger @ 2020-03-09 14:40 UTC (permalink / raw)
To: David Hildenbrand, Janosch Frank, qemu-devel; +Cc: qemu-s390x, cohuck
On 09.03.20 14:37, David Hildenbrand wrote:
>>
>> +#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.
>
something like the following?
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index c513f8efe0..cd12c29b9a 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -185,13 +185,18 @@ static void s390_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
static bool machine_is_pv(MachineState *ms)
{
Object *obj;
+ static S390CcwMachineState *ccw;
+
+ if (ccw)
+ return ccw->pv;
/* 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;
+ ccw = S390_CCW_MACHINE(obj);
+ return ccw->pv;
}
#endif
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v7 02/15] s390x: protvirt: Support unpack facility
2020-03-09 14:40 ` Christian Borntraeger
@ 2020-03-09 14:42 ` David Hildenbrand
2020-03-10 9:27 ` Christian Borntraeger
1 sibling, 0 replies; 45+ messages in thread
From: David Hildenbrand @ 2020-03-09 14:42 UTC (permalink / raw)
To: Christian Borntraeger, Janosch Frank, qemu-devel; +Cc: qemu-s390x, cohuck
On 09.03.20 15:40, Christian Borntraeger wrote:
>
>
> On 09.03.20 14:37, David Hildenbrand wrote:
>
>>>
>>> +#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.
>>
>
> something like the following?
>
>
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index c513f8efe0..cd12c29b9a 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -185,13 +185,18 @@ static void s390_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
> static bool machine_is_pv(MachineState *ms)
> {
> Object *obj;
> + static S390CcwMachineState *ccw;
> +
Move this up please (reverse christmas tree ;) )
> + if (ccw)
> + return ccw->pv;
>
> /* 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;
> + ccw = S390_CCW_MACHINE(obj);
> + return ccw->pv;
> }
> #endif
Yeah, guess this makes sense.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 02/15] s390x: protvirt: Support unpack facility
2020-03-09 14:40 ` Christian Borntraeger
2020-03-09 14:42 ` David Hildenbrand
@ 2020-03-10 9:27 ` Christian Borntraeger
1 sibling, 0 replies; 45+ messages in thread
From: Christian Borntraeger @ 2020-03-10 9:27 UTC (permalink / raw)
To: David Hildenbrand, Janosch Frank, qemu-devel; +Cc: qemu-s390x, cohuck
On 09.03.20 15:40, Christian Borntraeger wrote:
> something like the following?
>
>
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index c513f8efe0..cd12c29b9a 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -185,13 +185,18 @@ static void s390_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
> static bool machine_is_pv(MachineState *ms)
> {
> Object *obj;
> + static S390CcwMachineState *ccw;
> +
> + if (ccw)
> + return ccw->pv;
And I think we need curly braces: {}
Sorry. Was doing too many kernel things :-)
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 02/15] s390x: protvirt: Support unpack facility
2020-03-09 13:37 ` David Hildenbrand
2020-03-09 14:40 ` Christian Borntraeger
@ 2020-03-09 15:42 ` Janosch Frank
2020-03-09 15:48 ` David Hildenbrand
2020-03-10 8:32 ` [PATCH v8 1/2] s390x: ipl: Consolidate iplb validity check into one function Janosch Frank
2 siblings, 1 reply; 45+ messages in thread
From: Janosch Frank @ 2020-03-09 15:42 UTC (permalink / raw)
To: David Hildenbrand, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
[-- Attachment #1.1: Type: text/plain, Size: 20667 bytes --]
On 3/9/20 2:37 PM, David Hildenbrand wrote:
> On 09.03.20 12:21, 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 <frankja@linux.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> [Changes
>> to machine]
>
> As you signed this patch off, Maybe this should rather be a Co-developed-by:
>
Ack
> [...]
>
>> {
>> S390IPLState *ipl = get_ipl_device();
>> @@ -561,7 +581,8 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type)
>> {
>> S390IPLState *ipl = get_ipl_device();
>>
>> - if (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL) {
>> + if (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL ||
>> + reset_type == S390_RESET_PV) {
>> /* use CPU 0 for full resets */
>> ipl->reset_cpu_index = 0;
>
> This looks wrong. In case of an error, you modify the registers of a
> theoretically unrelated CPU.
It does indeed, will fix.
>
>
>> } else {
>> @@ -635,6 +656,38 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu)
>> cpu_physical_memory_unmap(addr, len, 1, len);
>> }
>>
>> +int s390_ipl_prepare_pv_header(void)
>> +{
>> + S390IPLState *ipl = get_ipl_device();
>> + IPLBlockPV *ipib_pv = &ipl->iplb_pv.pv;
>> + void *hdr = g_malloc(ipib_pv->pv_header_len);
>
> Should there be an upper limit? The guest can allocate quite some memory
> this way and theoretically crash the VM.
Yes, it's currently two pages max.
>
>> + int rc;
>> +
>> + cpu_physical_memory_read(ipib_pv->pv_header_addr, hdr,
>> + ipib_pv->pv_header_len);
>
> Shouldn't we validate if this memory is accessible at all?
Yes, I moved the check into iplb_valid so we don't need to worry about
that here.
>
>> + 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)
>> +{
>> + int i, rc = 0;
>
> NIT: These declarations last.
Ack
>
>> + S390IPLState *ipl = get_ipl_device();
>> + IPLBlockPV *ipib_pv = &ipl->iplb_pv.pv;
>
> use s390_ipl_get_iplb_pv() and assert that we don't get NULL?
We already check for NULL in the diag code before the DIAG308_PV_START
reset is set. I've used your other suggestion though.
>
>> +
>> + 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 d4813105db..b2ccdd9dae 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; /* 0x70 */
>> + uint64_t pv_header_addr; /* 0x74 */
>> + uint64_t pv_header_len; /* 0x7c */
>> + 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,16 +200,49 @@ static inline bool iplb_valid_len(IplParameterBlock *iplb)
>> return be32_to_cpu(iplb->len) <= sizeof(IplParameterBlock);
>> }
>>
>> -static inline bool iplb_valid_ccw(IplParameterBlock *iplb)
>> +static inline bool s390_ipl_pv_check_components(IplParameterBlock *iplb)
>
> Still don't like the function name :)
>
> ipl_valid_pv_components() ?
If it makes you happier
>
>> {
>> - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN &&
>> - iplb->pbt == S390_IPL_TYPE_CCW;
>> + 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 iplb_valid_fcp(IplParameterBlock *iplb)
>> +static inline bool iplb_valid(IplParameterBlock *iplb)
>> {
>> - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN &&
>> - iplb->pbt == S390_IPL_TYPE_FCP;
>> + switch (iplb->pbt) {
>> + case S390_IPL_TYPE_FCP:
>> + return (be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN &&
>> + iplb->pbt == S390_IPL_TYPE_FCP);
>> + case S390_IPL_TYPE_CCW:
>> + return (be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN &&
>> + iplb->pbt == S390_IPL_TYPE_CCW);
>
> That's a refactoring you could have split out.
Sure
>
>> + case S390_IPL_TYPE_PV:
>> + if(be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN ||
>> + iplb->pbt != S390_IPL_TYPE_PV) {
>> + return false;
>> + }
>> + return s390_ipl_pv_check_components(iplb);
>> + default:
>> + return false;
>> + }
>> }
>>
>> #endif
>> 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
>> + *
>> + * Copyright IBM Corp. 2020
>> + * Author(s):
>> + * Janosch Frank <frankja@linux.ibm.com>
>> + *
>> + * 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;
>> + struct kvm_pv_cmd pv_cmd = {
>> + .cmd = cmd,
>> + .data = (uint64_t)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,
>> + rc);
>> + }
>> + return rc;
>> +}
>> +
>> +static void s390_pv_cmd_exit(uint32_t cmd, void *data)
>> +{
>> + int rc;
>> +
>> + rc = s390_pv_cmd(cmd, data);
>> + if (rc) {
>> + exit(1);
>> + }
>> +}
>> +
>> +int s390_pv_vm_enable(void)
>> +{
>> + return s390_pv_cmd(KVM_PV_ENABLE, NULL);
>> +}
>> +
>> +void s390_pv_vm_disable(void)
>> +{
>> + s390_pv_cmd_exit(KVM_PV_DISABLE, NULL);
>> +}
>> +
>> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
>> +{
>> + struct kvm_s390_pv_sec_parm args = {
>> + .origin = origin,
>> + .length = length,
>> + };
>> +
>> + return s390_pv_cmd(KVM_PV_VM_SET_SEC_PARMS, &args);
>> +}
>> +
>> +/*
>> + * Called for each component in the SE type IPL parameter block 0.
>> + */
>> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak)
>> +{
>> + struct kvm_s390_pv_unp args = {
>> + .addr = addr,
>> + .size = size,
>> + .tweak = tweak,
>> + };
>> +
>> + return s390_pv_cmd(KVM_PV_VM_UNPACK, &args);
>> +}
>> +
>> +void s390_pv_perf_clear_reset(void)
>> +{
>> + s390_pv_cmd_exit(KVM_PV_VM_PREP_RESET, NULL);
>> +}
>> +
>> +int s390_pv_verify(void)
>> +{
>> + return s390_pv_cmd(KVM_PV_VM_VERIFY, NULL);
>> +}
>> +
>> +void s390_pv_unshare(void)
>> +{
>> + s390_pv_cmd_exit(KVM_PV_VM_UNSHARE_ALL, NULL);
>> +}
>> diff --git a/hw/s390x/pv.h b/hw/s390x/pv.h
>> new file mode 100644
>> index 0000000000..e58fbca96a
>> --- /dev/null
>> +++ b/hw/s390x/pv.h
>> @@ -0,0 +1,34 @@
>> +/*
>> + * Protected Virtualization header
>> + *
>> + * Copyright IBM Corp. 2020
>> + * Author(s):
>> + * Janosch Frank <frankja@linux.ibm.com>
>> + *
>> + * 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.
>> + */
>> +
>> +#ifndef HW_S390_PV_H
>> +#define HW_S390_PV_H
>> +
>> +#ifdef CONFIG_KVM
>> +int s390_pv_vm_enable(void);
>> +void s390_pv_vm_disable(void);
>> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
>> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
>> +void s390_pv_perf_clear_reset(void);
>> +int s390_pv_verify(void);
>> +void s390_pv_unshare(void);
>> +#else
>> +static inline int s390_pv_vm_enable(void) { return 0; }
>> +static inline void s390_pv_vm_disable(void) {}
>> +static inline int s390_pv_set_sec_parms(uint64_t origin, uint64_t length) { return 0; }
>> +static inline int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak) { return 0; }
>> +static inline void s390_pv_perf_clear_reset(void) {}
>> +static inline int s390_pv_verify(void) { return 0; }
>> +static inline void s390_pv_unshare(void) {}
>> +#endif
>> +
>> +#endif /* HW_S390_PV_H */
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index a89cf4c129..f718cfc591 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)
>> {
>> @@ -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;
>> s390_sclp_init();
>> /* init memory + setup max page size. Required for the CPU model */
>> s390_memory_init(machine->ram);
>> @@ -316,10 +320,90 @@ 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;
>> +
>> + if (ms->pv) {
>
> This is always the case. The check for ms->pv dropped (and if other
> patches require this, it is to be handled in the caller).
Ack
>
>> + s390_pv_vm_disable();
>> + CPU_FOREACH(t) {
>> + S390_CPU(t)->env.pv = false;
>> + }
>> + ms->pv = false;
>> + }
>> +}
>> +
>> +static int s390_machine_protect(S390CcwMachineState *ms)
>> +{
>> + CPUState *t;
>> + int rc;
>> +
>> + /* Create SE VM */
>> + rc = s390_pv_vm_enable();
>> + if (rc) {
>> + return rc;
>> + }
>> +
>> + CPU_FOREACH(t) {
>> + S390_CPU(t)->env.pv = true;
>> + }
>> + ms->pv = true;
>> +
>> + /* Set SE header and unpack */
>> + rc = s390_ipl_prepare_pv_header();
>> + if (rc) {
>> + goto out_err;
>> + }
>> +
>> + /* Decrypt image */
>> + rc = s390_ipl_pv_unpack();
>> + if (rc) {
>> + goto out_err;
>> + }
>> +
>> + /* Verify integrity */
>> + rc = s390_pv_verify();
>> + if (rc) {
>> + goto out_err;
>> + }
>> + return rc;
>> +
>> +out_err:
>> + s390_machine_unprotect(ms);
>> + return rc;
>> +}
>> +
>> +#define DIAG_308_RC_INVAL_FOR_PV 0x0a02
>> +static void s390_machine_inject_pv_error(CPUState *cs)
>> +{
>> + int r1 = (cs->kvm_run->s390_sieic.ipa & 0x00f0) >> 4;
>> + CPUS390XState *env = &S390_CPU(cs)->env;
>> +
>> + /* Report that we are unable to enter protected mode */
>> + env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV;
>> +}
>> +
>> +static void s390_pv_prepare_reset(CPUS390XState *env)
>> +{
>> + CPUState *cs;
>> +
>> + if (!env->pv) {
>> + return;
>> + }
>> + CPU_FOREACH(cs) {
>> + s390_cpu_set_state(S390_CPU_STATE_STOPPED, S390_CPU(cs));
>> + }
>
> This is done by the CPU reset in all cases? Why is that needed? (->comment)
>
>> + s390_pv_unshare();
>> + s390_pv_perf_clear_reset();
>> +}
>> +
>> static void s390_machine_reset(MachineState *machine)
>> {
>> enum s390_reset reset_type;
>> CPUState *cs, *t;
>> + S390CPU *cpu;
>> + S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
>
> Nit: Move this to the very top.
Ack
>
>> + 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?
>
>> +
>> 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
?
>
>> + s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>> + return;
>> + }
>> +
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 02/15] s390x: protvirt: Support unpack facility
2020-03-09 15:42 ` Janosch Frank
@ 2020-03-09 15:48 ` David Hildenbrand
0 siblings, 0 replies; 45+ messages in thread
From: David Hildenbrand @ 2020-03-09 15:48 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
>>
>>> + 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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v8 1/2] s390x: ipl: Consolidate iplb validity check into one function
2020-03-09 13:37 ` David Hildenbrand
2020-03-09 14:40 ` Christian Borntraeger
2020-03-09 15:42 ` Janosch Frank
@ 2020-03-10 8:32 ` Janosch Frank
2020-03-10 8:32 ` [PATCH v8 2/2] s390x: protvirt: Support unpack facility Janosch Frank
` (2 more replies)
2 siblings, 3 replies; 45+ messages in thread
From: Janosch Frank @ 2020-03-10 8:32 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
It's nicer to just call one function than calling a function for each
possible iplb type.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
hw/s390x/ipl.h | 20 +++++++++++---------
target/s390x/diag.c | 2 +-
2 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index d4813105db..211ce2dbeb 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -173,16 +173,18 @@ static inline bool iplb_valid_len(IplParameterBlock *iplb)
return be32_to_cpu(iplb->len) <= sizeof(IplParameterBlock);
}
-static inline bool iplb_valid_ccw(IplParameterBlock *iplb)
+static inline bool iplb_valid(IplParameterBlock *iplb)
{
- return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN &&
- iplb->pbt == S390_IPL_TYPE_CCW;
-}
-
-static inline bool iplb_valid_fcp(IplParameterBlock *iplb)
-{
- return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN &&
- iplb->pbt == S390_IPL_TYPE_FCP;
+ switch (iplb->pbt) {
+ case S390_IPL_TYPE_FCP:
+ return (be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN &&
+ iplb->pbt == S390_IPL_TYPE_FCP);
+ case S390_IPL_TYPE_CCW:
+ return (be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN &&
+ iplb->pbt == S390_IPL_TYPE_CCW);
+ default:
+ return false;
+ }
}
#endif
diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index b5aec06d6b..54e5670b3f 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -117,7 +117,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
- if (!iplb_valid_ccw(iplb) && !iplb_valid_fcp(iplb)) {
+ if (!iplb_valid(iplb)) {
env->regs[r1 + 1] = DIAG_308_RC_INVALID;
goto out;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v8 2/2] s390x: protvirt: Support unpack facility
2020-03-10 8:32 ` [PATCH v8 1/2] s390x: ipl: Consolidate iplb validity check into one function Janosch Frank
@ 2020-03-10 8:32 ` Janosch Frank
2020-03-10 9:00 ` David Hildenbrand
2020-03-10 8:40 ` [PATCH v8 1/2] s390x: ipl: Consolidate iplb validity check into one function David Hildenbrand
2020-03-10 9:05 ` Christian Borntraeger
2 siblings, 1 reply; 45+ messages in thread
From: Janosch Frank @ 2020-03-10 8:32 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
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 <frankja@linux.ibm.com>
Co-developed-by: Christian Borntraeger <borntraeger@de.ibm.com> [Changes
to machine]
---
hw/s390x/Makefile.objs | 1 +
hw/s390x/ipl.c | 56 +++++++++++-
hw/s390x/ipl.h | 80 +++++++++++++++++
hw/s390x/pv.c | 104 ++++++++++++++++++++++
hw/s390x/pv.h | 34 ++++++++
hw/s390x/s390-virtio-ccw.c | 128 +++++++++++++++++++++++++++-
include/hw/s390x/s390-virtio-ccw.h | 1 +
target/s390x/cpu.c | 22 +++++
target/s390x/cpu.h | 1 +
target/s390x/cpu_features_def.inc.h | 1 +
target/s390x/diag.c | 32 ++++++-
11 files changed, 453 insertions(+), 7 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 9c1ecd423c..74d42a8d11 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
@@ -542,11 +543,30 @@ 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);
}
+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();
@@ -635,6 +655,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 211ce2dbeb..66c8380397 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) {
@@ -182,6 +253,15 @@ static inline bool iplb_valid(IplParameterBlock *iplb)
case S390_IPL_TYPE_CCW:
return (be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN &&
iplb->pbt == S390_IPL_TYPE_CCW);
+ case S390_IPL_TYPE_PV:
+ if(be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN ||
+ iplb->pbt != S390_IPL_TYPE_PV) {
+ 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..ba6409246e
--- /dev/null
+++ b/hw/s390x/pv.c
@@ -0,0 +1,104 @@
+/*
+ * Secure execution functions
+ *
+ * Copyright IBM Corp. 2020
+ * Author(s):
+ * Janosch Frank <frankja@linux.ibm.com>
+ *
+ * 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;
+ struct kvm_pv_cmd pv_cmd = {
+ .cmd = cmd,
+ .data = (uint64_t)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,
+ rc);
+ }
+ return rc;
+}
+
+static void s390_pv_cmd_exit(uint32_t cmd, void *data)
+{
+ int rc;
+
+ rc = s390_pv_cmd(cmd, data);
+ if (rc) {
+ exit(1);
+ }
+}
+
+int s390_pv_vm_enable(void)
+{
+ return s390_pv_cmd(KVM_PV_ENABLE, NULL);
+}
+
+void s390_pv_vm_disable(void)
+{
+ s390_pv_cmd_exit(KVM_PV_DISABLE, NULL);
+}
+
+int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
+{
+ struct kvm_s390_pv_sec_parm args = {
+ .origin = origin,
+ .length = length,
+ };
+
+ return s390_pv_cmd(KVM_PV_VM_SET_SEC_PARMS, &args);
+}
+
+/*
+ * Called for each component in the SE type IPL parameter block 0.
+ */
+int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak)
+{
+ struct kvm_s390_pv_unp args = {
+ .addr = addr,
+ .size = size,
+ .tweak = tweak,
+ };
+
+ return s390_pv_cmd(KVM_PV_VM_UNPACK, &args);
+}
+
+void s390_pv_perf_clear_reset(void)
+{
+ s390_pv_cmd_exit(KVM_PV_VM_PREP_RESET, NULL);
+}
+
+int s390_pv_verify(void)
+{
+ return s390_pv_cmd(KVM_PV_VM_VERIFY, NULL);
+}
+
+void s390_pv_unshare(void)
+{
+ s390_pv_cmd_exit(KVM_PV_VM_UNSHARE_ALL, NULL);
+}
diff --git a/hw/s390x/pv.h b/hw/s390x/pv.h
new file mode 100644
index 0000000000..e58fbca96a
--- /dev/null
+++ b/hw/s390x/pv.h
@@ -0,0 +1,34 @@
+/*
+ * Protected Virtualization header
+ *
+ * Copyright IBM Corp. 2020
+ * Author(s):
+ * Janosch Frank <frankja@linux.ibm.com>
+ *
+ * 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.
+ */
+
+#ifndef HW_S390_PV_H
+#define HW_S390_PV_H
+
+#ifdef CONFIG_KVM
+int s390_pv_vm_enable(void);
+void s390_pv_vm_disable(void);
+int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
+int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
+void s390_pv_perf_clear_reset(void);
+int s390_pv_verify(void);
+void s390_pv_unshare(void);
+#else
+static inline int s390_pv_vm_enable(void) { return 0; }
+static inline void s390_pv_vm_disable(void) {}
+static inline int s390_pv_set_sec_parms(uint64_t origin, uint64_t length) { return 0; }
+static inline int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak) { return 0; }
+static inline void s390_pv_perf_clear_reset(void) {}
+static inline int s390_pv_verify(void) { return 0; }
+static inline void s390_pv_unshare(void) {}
+#endif
+
+#endif /* HW_S390_PV_H */
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index a89cf4c129..fc84ddb3d1 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)
{
@@ -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;
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;
+ }
+ ms->pv = false;
+}
+
+static int s390_machine_protect(S390CcwMachineState *ms)
+{
+ CPUState *t;
+ int rc;
+
+ /* Create SE VM */
+ rc = s390_pv_vm_enable();
+ if (rc) {
+ return rc;
+ }
+
+ CPU_FOREACH(t) {
+ S390_CPU(t)->env.pv = true;
+ }
+ ms->pv = true;
+
+ /* Set SE header and unpack */
+ rc = s390_ipl_prepare_pv_header();
+ if (rc) {
+ goto out_err;
+ }
+
+ /* Decrypt image */
+ rc = s390_ipl_pv_unpack();
+ if (rc) {
+ goto out_err;
+ }
+
+ /* Verify integrity */
+ rc = s390_pv_verify();
+ if (rc) {
+ goto out_err;
+ }
+ return rc;
+
+out_err:
+ s390_machine_unprotect(ms);
+ return rc;
+}
+
+#define DIAG_308_RC_INVAL_FOR_PV 0x0a02
+static void s390_machine_inject_pv_error(CPUState *cs)
+{
+ int r1 = (cs->kvm_run->s390_sieic.ipa & 0x00f0) >> 4;
+ CPUS390XState *env = &S390_CPU(cs)->env;
+
+ /* Report that we are unable to enter protected mode */
+ env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV;
+}
+
+static void s390_pv_prepare_reset(S390CcwMachineState *ms)
+{
+ CPUState *cs;
+
+ if (!ms->pv) {
+ return;
+ }
+ /* Unsharing requires all cpus to be stopped */
+ CPU_FOREACH(cs) {
+ s390_cpu_set_state(S390_CPU_STATE_STOPPED, S390_CPU(cs));
+ }
+ s390_pv_unshare();
+ s390_pv_perf_clear_reset();
+}
+
static void s390_machine_reset(MachineState *machine)
{
+ S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
enum s390_reset reset_type;
CPUState *cs, *t;
+ S390CPU *cpu;
/* get the reset parameters, reset them once done */
s390_ipl_get_reset_request(&cs, &reset_type);
@@ -327,9 +409,15 @@ static void s390_machine_reset(MachineState *machine)
/* all CPUs are paused and synchronized at this point */
s390_cmma_reset();
+ cpu = S390_CPU(cs);
+
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 +425,56 @@ 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(ms);
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(ms);
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);
+ /*
+ * Continue after the diag308 so the guest knows something
+ * went wrong.
+ */
+ s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
+ return;
+ }
+
run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
break;
default:
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 8aa27199c9..cd1dccc6e3 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -28,6 +28,7 @@ typedef struct S390CcwMachineState {
/*< public >*/
bool aes_key_wrap;
bool dea_key_wrap;
+ bool pv;
uint8_t loadparm[8];
} S390CcwMachineState;
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 3dd396e870..aec5fb3b76 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"
#include "hw/boards.h"
#include "sysemu/arch_init.h"
#include "sysemu/sysemu.h"
@@ -174,6 +176,25 @@ static void s390_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
info->print_insn = print_insn_s390;
}
+#if !defined(CONFIG_USER_ONLY)
+static bool machine_is_pv(MachineState *ms)
+{
+ static S390CcwMachineState *ccw;
+ Object *obj;
+
+ if (ccw)
+ return ccw->pv;
+
+ /* 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
+
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")
/* Features exposed via SCLP SCCB Byte 80 - 98 (bit numbers relative to byte-80) */
DEF_FEAT(SIE_GSLS, "gsls", SCLP_CONF_CHAR, 40, "SIE: Guest-storage-limit-suppression facility")
diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index 54e5670b3f..51f51eb5c6 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -52,6 +52,7 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
#define DIAG_308_RC_OK 0x0001
#define DIAG_308_RC_NO_CONF 0x0102
#define DIAG_308_RC_INVALID 0x0402
+#define DIAG_308_RC_NO_PV_CONF 0x0902
#define DIAG308_RESET_MOD_CLR 0
#define DIAG308_RESET_LOAD_NORM 1
@@ -59,10 +60,17 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
#define DIAG308_LOAD_NORMAL_DUMP 4
#define DIAG308_SET 5
#define DIAG308_STORE 6
+#define DIAG308_PV_SET 8
+#define DIAG308_PV_STORE 9
+#define DIAG308_PV_START 10
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 >= DIAG308_PV_SET && !s390_has_feat(S390_FEAT_UNPACK)) {
+ s390_program_interrupt(env, PGM_SPECIFICATION, ra);
+ return;
+ }
+
switch (subcode) {
case DIAG308_RESET_MOD_CLR:
s390_ipl_reset_request(cs, S390_RESET_MODIFIED_CLEAR);
@@ -105,6 +118,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
s390_ipl_reset_request(cs, S390_RESET_REIPL);
break;
case DIAG308_SET:
+ case DIAG308_PV_SET:
if (diag308_parm_check(env, r1, addr, ra, false)) {
return;
}
@@ -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;
+ 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;
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v8 2/2] s390x: protvirt: Support unpack facility
2020-03-10 8:32 ` [PATCH v8 2/2] s390x: protvirt: Support unpack facility Janosch Frank
@ 2020-03-10 9:00 ` David Hildenbrand
2020-03-10 9:23 ` Janosch Frank
2020-03-10 9:24 ` Christian Borntraeger
0 siblings, 2 replies; 45+ messages in thread
From: David Hildenbrand @ 2020-03-10 9:00 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
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 <frankja@linux.ibm.com>
> Co-developed-by: Christian Borntraeger <borntraeger@de.ibm.com> [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 ;)
> + *
> + * Copyright IBM Corp. 2020
> + * Author(s):
> + * Janosch Frank <frankja@linux.ibm.com>
> + *
> + * 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.
> 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.
[...]
> + 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)
> + }
> +
> 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)
> +
> 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.
[...]
> @@ -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.
> + 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;
>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v8 2/2] s390x: protvirt: Support unpack facility
2020-03-10 9:00 ` David Hildenbrand
@ 2020-03-10 9:23 ` Janosch Frank
2020-03-10 9:28 ` David Hildenbrand
2020-03-10 9:24 ` Christian Borntraeger
1 sibling, 1 reply; 45+ messages in thread
From: Janosch Frank @ 2020-03-10 9:23 UTC (permalink / raw)
To: David Hildenbrand, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
[-- Attachment #1.1: Type: text/plain, Size: 8036 bytes --]
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 <frankja@linux.ibm.com>
>> Co-developed-by: Christian Borntraeger <borntraeger@de.ibm.com> [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 <frankja@linux.ibm.com>
>> + *
>> + * 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;
>>
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v8 2/2] s390x: protvirt: Support unpack facility
2020-03-10 9:23 ` Janosch Frank
@ 2020-03-10 9:28 ` David Hildenbrand
0 siblings, 0 replies; 45+ messages in thread
From: David Hildenbrand @ 2020-03-10 9:28 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
>>> + 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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v8 2/2] s390x: protvirt: Support unpack facility
2020-03-10 9:00 ` David Hildenbrand
2020-03-10 9:23 ` Janosch Frank
@ 2020-03-10 9:24 ` Christian Borntraeger
1 sibling, 0 replies; 45+ messages in thread
From: Christian Borntraeger @ 2020-03-10 9:24 UTC (permalink / raw)
To: David Hildenbrand, Janosch Frank, qemu-devel; +Cc: qemu-s390x, cohuck
On 10.03.20 10:00, David Hildenbrand wrote:
>
> 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)
Yes that makes sense. Now that we have enable/disable we can use the machine pv property.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v8 1/2] s390x: ipl: Consolidate iplb validity check into one function
2020-03-10 8:32 ` [PATCH v8 1/2] s390x: ipl: Consolidate iplb validity check into one function Janosch Frank
2020-03-10 8:32 ` [PATCH v8 2/2] s390x: protvirt: Support unpack facility Janosch Frank
@ 2020-03-10 8:40 ` David Hildenbrand
2020-03-10 9:05 ` Christian Borntraeger
2 siblings, 0 replies; 45+ messages in thread
From: David Hildenbrand @ 2020-03-10 8:40 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
On 10.03.20 09:32, Janosch Frank wrote:
> It's nicer to just call one function than calling a function for each
> possible iplb type.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> hw/s390x/ipl.h | 20 +++++++++++---------
> target/s390x/diag.c | 2 +-
> 2 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index d4813105db..211ce2dbeb 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -173,16 +173,18 @@ static inline bool iplb_valid_len(IplParameterBlock *iplb)
> return be32_to_cpu(iplb->len) <= sizeof(IplParameterBlock);
> }
>
> -static inline bool iplb_valid_ccw(IplParameterBlock *iplb)
> +static inline bool iplb_valid(IplParameterBlock *iplb)
> {
> - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN &&
> - iplb->pbt == S390_IPL_TYPE_CCW;
> -}
> -
> -static inline bool iplb_valid_fcp(IplParameterBlock *iplb)
> -{
> - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN &&
> - iplb->pbt == S390_IPL_TYPE_FCP;
> + switch (iplb->pbt) {
> + case S390_IPL_TYPE_FCP:
> + return (be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN &&
> + iplb->pbt == S390_IPL_TYPE_FCP);
> + case S390_IPL_TYPE_CCW:
> + return (be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN &&
> + iplb->pbt == S390_IPL_TYPE_CCW);
> + default:
> + return false;
> + }
> }
>
> #endif
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index b5aec06d6b..54e5670b3f 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -117,7 +117,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>
> cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
>
> - if (!iplb_valid_ccw(iplb) && !iplb_valid_fcp(iplb)) {
> + if (!iplb_valid(iplb)) {
> env->regs[r1 + 1] = DIAG_308_RC_INVALID;
> goto out;
> }
>
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v8 1/2] s390x: ipl: Consolidate iplb validity check into one function
2020-03-10 8:32 ` [PATCH v8 1/2] s390x: ipl: Consolidate iplb validity check into one function Janosch Frank
2020-03-10 8:32 ` [PATCH v8 2/2] s390x: protvirt: Support unpack facility Janosch Frank
2020-03-10 8:40 ` [PATCH v8 1/2] s390x: ipl: Consolidate iplb validity check into one function David Hildenbrand
@ 2020-03-10 9:05 ` Christian Borntraeger
2020-03-10 9:09 ` [PATCH v8] " Janosch Frank
2 siblings, 1 reply; 45+ messages in thread
From: Christian Borntraeger @ 2020-03-10 9:05 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: qemu-s390x, cohuck, david
On 10.03.20 09:32, Janosch Frank wrote:
> It's nicer to just call one function than calling a function for each
> possible iplb type.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> hw/s390x/ipl.h | 20 +++++++++++---------
> target/s390x/diag.c | 2 +-
> 2 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index d4813105db..211ce2dbeb 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -173,16 +173,18 @@ static inline bool iplb_valid_len(IplParameterBlock *iplb)
> return be32_to_cpu(iplb->len) <= sizeof(IplParameterBlock);
> }
>
> -static inline bool iplb_valid_ccw(IplParameterBlock *iplb)
> +static inline bool iplb_valid(IplParameterBlock *iplb)
> {
> - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN &&
> - iplb->pbt == S390_IPL_TYPE_CCW;
> -}
> -
> -static inline bool iplb_valid_fcp(IplParameterBlock *iplb)
> -{
> - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN &&
> - iplb->pbt == S390_IPL_TYPE_FCP;
> + switch (iplb->pbt) {
> + case S390_IPL_TYPE_FCP:
> + return (be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN &&
> + iplb->pbt == S390_IPL_TYPE_FCP);
Isnt the iplb->pbt check redundant due to the switch statement?
> + case S390_IPL_TYPE_CCW:
> + return (be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN &&
> + iplb->pbt == S390_IPL_TYPE_CCW);
> + default:
same here.
^ permalink raw reply [flat|nested] 45+ messages in thread* [PATCH v8] s390x: ipl: Consolidate iplb validity check into one function
2020-03-10 9:05 ` Christian Borntraeger
@ 2020-03-10 9:09 ` Janosch Frank
2020-03-10 9:11 ` Christian Borntraeger
2020-03-10 9:21 ` Christian Borntraeger
0 siblings, 2 replies; 45+ messages in thread
From: Janosch Frank @ 2020-03-10 9:09 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
It's nicer to just call one function than calling a function for each
possible iplb type.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
hw/s390x/ipl.h | 18 +++++++++---------
target/s390x/diag.c | 2 +-
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index d4813105db..3e44abe1c6 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -173,16 +173,16 @@ static inline bool iplb_valid_len(IplParameterBlock *iplb)
return be32_to_cpu(iplb->len) <= sizeof(IplParameterBlock);
}
-static inline bool iplb_valid_ccw(IplParameterBlock *iplb)
+static inline bool iplb_valid(IplParameterBlock *iplb)
{
- return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN &&
- iplb->pbt == S390_IPL_TYPE_CCW;
-}
-
-static inline bool iplb_valid_fcp(IplParameterBlock *iplb)
-{
- return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN &&
- iplb->pbt == S390_IPL_TYPE_FCP;
+ switch (iplb->pbt) {
+ case S390_IPL_TYPE_FCP:
+ 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;
+ default:
+ return false;
+ }
}
#endif
diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index b5aec06d6b..54e5670b3f 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -117,7 +117,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
- if (!iplb_valid_ccw(iplb) && !iplb_valid_fcp(iplb)) {
+ if (!iplb_valid(iplb)) {
env->regs[r1 + 1] = DIAG_308_RC_INVALID;
goto out;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v8] s390x: ipl: Consolidate iplb validity check into one function
2020-03-10 9:09 ` [PATCH v8] " Janosch Frank
@ 2020-03-10 9:11 ` Christian Borntraeger
2020-03-10 9:20 ` David Hildenbrand
2020-03-10 9:21 ` Christian Borntraeger
1 sibling, 1 reply; 45+ messages in thread
From: Christian Borntraeger @ 2020-03-10 9:11 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: qemu-s390x, cohuck, david
On 10.03.20 10:09, Janosch Frank wrote:
> It's nicer to just call one function than calling a function for each
> possible iplb type.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
David, still ok? If yes I will pick this one.
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> hw/s390x/ipl.h | 18 +++++++++---------
> target/s390x/diag.c | 2 +-
> 2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index d4813105db..3e44abe1c6 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -173,16 +173,16 @@ static inline bool iplb_valid_len(IplParameterBlock *iplb)
> return be32_to_cpu(iplb->len) <= sizeof(IplParameterBlock);
> }
>
> -static inline bool iplb_valid_ccw(IplParameterBlock *iplb)
> +static inline bool iplb_valid(IplParameterBlock *iplb)
> {
> - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN &&
> - iplb->pbt == S390_IPL_TYPE_CCW;
> -}
> -
> -static inline bool iplb_valid_fcp(IplParameterBlock *iplb)
> -{
> - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN &&
> - iplb->pbt == S390_IPL_TYPE_FCP;
> + switch (iplb->pbt) {
> + case S390_IPL_TYPE_FCP:
> + 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;
> + default:
> + return false;
> + }
> }
>
> #endif
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index b5aec06d6b..54e5670b3f 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -117,7 +117,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>
> cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
>
> - if (!iplb_valid_ccw(iplb) && !iplb_valid_fcp(iplb)) {
> + if (!iplb_valid(iplb)) {
> env->regs[r1 + 1] = DIAG_308_RC_INVALID;
> goto out;
> }
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v8] s390x: ipl: Consolidate iplb validity check into one function
2020-03-10 9:11 ` Christian Borntraeger
@ 2020-03-10 9:20 ` David Hildenbrand
0 siblings, 0 replies; 45+ messages in thread
From: David Hildenbrand @ 2020-03-10 9:20 UTC (permalink / raw)
To: Christian Borntraeger, Janosch Frank, qemu-devel; +Cc: qemu-s390x, cohuck
On 10.03.20 10:11, Christian Borntraeger wrote:
>
>
> On 10.03.20 10:09, Janosch Frank wrote:
>> It's nicer to just call one function than calling a function for each
>> possible iplb type.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>
> David, still ok? If yes I will pick this one.
Sure, only missed the duplicate check. thanks
>
>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v8] s390x: ipl: Consolidate iplb validity check into one function
2020-03-10 9:09 ` [PATCH v8] " Janosch Frank
2020-03-10 9:11 ` Christian Borntraeger
@ 2020-03-10 9:21 ` Christian Borntraeger
1 sibling, 0 replies; 45+ messages in thread
From: Christian Borntraeger @ 2020-03-10 9:21 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: qemu-s390x, cohuck, david
On 10.03.20 10:09, Janosch Frank wrote:
> It's nicer to just call one function than calling a function for each
> possible iplb type.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
thanks applied.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 02/15] s390x: protvirt: Support unpack facility
2020-03-09 11:21 ` [PATCH v7 02/15] s390x: protvirt: Support unpack facility Janosch Frank
2020-03-09 13:37 ` David Hildenbrand
@ 2020-03-09 14:28 ` Viktor Mihajlovski
2020-03-09 14:46 ` Janosch Frank
1 sibling, 1 reply; 45+ messages in thread
From: Viktor Mihajlovski @ 2020-03-09 14:28 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
On 3/9/20 12:21 PM, 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 <frankja@linux.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> [Changes
> to machine]
A nit: [Changes...] should go before the s-o-b.
> ---
> hw/s390x/Makefile.objs | 1 +
> hw/s390x/ipl.c | 59 ++++++++++++-
> hw/s390x/ipl.h | 72 ++++++++++++++--
> hw/s390x/pv.c | 104 +++++++++++++++++++++++
> hw/s390x/pv.h | 34 ++++++++
> hw/s390x/s390-virtio-ccw.c | 127 +++++++++++++++++++++++++++-
> include/hw/s390x/s390-virtio-ccw.h | 1 +
> target/s390x/cpu.c | 17 ++++
> target/s390x/cpu.h | 1 +
> target/s390x/cpu_features_def.inc.h | 1 +
> target/s390x/diag.c | 34 +++++++-
> 11 files changed, 436 insertions(+), 15 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 9c1ecd423c..ba3eac30c6 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
> @@ -542,11 +543,30 @@ 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);
> }
>
> +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();
> @@ -561,7 +581,8 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type)
> {
> S390IPLState *ipl = get_ipl_device();
>
> - if (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL) {
> + if (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL ||
> + reset_type == S390_RESET_PV) {
> /* use CPU 0 for full resets */
> ipl->reset_cpu_index = 0;
> } else {
> @@ -635,6 +656,38 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu)
> cpu_physical_memory_unmap(addr, len, 1, len);
> }
>
> +int s390_ipl_prepare_pv_header(void)
> +{
> + S390IPLState *ipl = get_ipl_device();
> + IPLBlockPV *ipib_pv = &ipl->iplb_pv.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)
> +{
> + int i, rc = 0;
> + S390IPLState *ipl = get_ipl_device();
> + IPLBlockPV *ipib_pv = &ipl->iplb_pv.pv;
> +
> + 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 d4813105db..b2ccdd9dae 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; /* 0x70 */
Another nit: offset should be 0x74
> + uint64_t pv_header_addr; /* 0x74 */
> + uint64_t pv_header_len; /* 0x7c */
> + struct IPLBlockPVComp components[];
> +} QEMU_PACKED;
> +typedef struct IPLBlockPV IPLBlockPV;
> +
> struct IplBlockCcw {
> uint8_t reserved0[85];
> uint8_t ssid;[...]
--
Kind Regards,
Viktor
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 02/15] s390x: protvirt: Support unpack facility
2020-03-09 14:28 ` [PATCH v7 02/15] s390x: protvirt: Support unpack facility Viktor Mihajlovski
@ 2020-03-09 14:46 ` Janosch Frank
0 siblings, 0 replies; 45+ messages in thread
From: Janosch Frank @ 2020-03-09 14:46 UTC (permalink / raw)
To: Viktor Mihajlovski, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
[-- Attachment #1.1: Type: text/plain, Size: 6189 bytes --]
On 3/9/20 3:28 PM, Viktor Mihajlovski wrote:
>
>
> On 3/9/20 12:21 PM, 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 <frankja@linux.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> [Changes
>> to machine]
> A nit: [Changes...] should go before the s-o-b.
Will be changed anyway
>> ---
>> hw/s390x/Makefile.objs | 1 +
>> hw/s390x/ipl.c | 59 ++++++++++++-
>> hw/s390x/ipl.h | 72 ++++++++++++++--
>> hw/s390x/pv.c | 104 +++++++++++++++++++++++
>> hw/s390x/pv.h | 34 ++++++++
>> hw/s390x/s390-virtio-ccw.c | 127 +++++++++++++++++++++++++++-
>> include/hw/s390x/s390-virtio-ccw.h | 1 +
>> target/s390x/cpu.c | 17 ++++
>> target/s390x/cpu.h | 1 +
>> target/s390x/cpu_features_def.inc.h | 1 +
>> target/s390x/diag.c | 34 +++++++-
>> 11 files changed, 436 insertions(+), 15 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 9c1ecd423c..ba3eac30c6 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
>> @@ -542,11 +543,30 @@ 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);
>> }
>>
>> +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();
>> @@ -561,7 +581,8 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type)
>> {
>> S390IPLState *ipl = get_ipl_device();
>>
>> - if (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL) {
>> + if (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL ||
>> + reset_type == S390_RESET_PV) {
>> /* use CPU 0 for full resets */
>> ipl->reset_cpu_index = 0;
>> } else {
>> @@ -635,6 +656,38 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu)
>> cpu_physical_memory_unmap(addr, len, 1, len);
>> }
>>
>> +int s390_ipl_prepare_pv_header(void)
>> +{
>> + S390IPLState *ipl = get_ipl_device();
>> + IPLBlockPV *ipib_pv = &ipl->iplb_pv.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)
>> +{
>> + int i, rc = 0;
>> + S390IPLState *ipl = get_ipl_device();
>> + IPLBlockPV *ipib_pv = &ipl->iplb_pv.pv;
>> +
>> + 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 d4813105db..b2ccdd9dae 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; /* 0x70 */
> Another nit: offset should be 0x74
Ups
>> + uint64_t pv_header_addr; /* 0x74 */
>> + uint64_t pv_header_len; /* 0x7c */
>> + struct IPLBlockPVComp components[];
>> +} QEMU_PACKED;
>> +typedef struct IPLBlockPV IPLBlockPV;
>> +
>> struct IplBlockCcw {
>> uint8_t reserved0[85];
>> uint8_t ssid;[...]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v7 03/15] s390x: protvirt: Add migration blocker
2020-03-09 11:21 [PATCH v7 00/15] s390x: Protected Virtualization support Janosch Frank
2020-03-09 11:21 ` [PATCH v7 01/15] Sync pv Janosch Frank
2020-03-09 11:21 ` [PATCH v7 02/15] s390x: protvirt: Support unpack facility Janosch Frank
@ 2020-03-09 11:21 ` Janosch Frank
2020-03-09 13:41 ` David Hildenbrand
2020-03-09 11:21 ` [PATCH v7 04/15] s390x: protvirt: Inhibit balloon when switching to protected mode Janosch Frank
` (11 subsequent siblings)
14 siblings, 1 reply; 45+ messages in thread
From: Janosch Frank @ 2020-03-09 11:21 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
Migration is not yet supported.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
hw/s390x/s390-virtio-ccw.c | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index f718cfc591..4bb38704ff 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -43,6 +43,9 @@
#include "sysemu/sysemu.h"
#include "hw/s390x/pv.h"
#include <linux/kvm.h>
+#include "migration/blocker.h"
+
+static Error *pv_mig_blocker;
S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
{
@@ -331,16 +334,33 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
}
ms->pv = false;
}
+ migrate_del_blocker(pv_mig_blocker);
+ error_free(pv_mig_blocker);
+ pv_mig_blocker = NULL;
}
static int s390_machine_protect(S390CcwMachineState *ms)
{
+ static Error *local_err;
CPUState *t;
int rc;
+ error_setg(&pv_mig_blocker,
+ "protected VMs are currently not migrateable.");
+ rc = migrate_add_blocker(pv_mig_blocker, &local_err);
+ if (local_err) {
+ error_report_err(local_err);
+ error_free(pv_mig_blocker);
+ pv_mig_blocker = NULL;
+ return rc;
+ }
+
/* Create SE VM */
rc = s390_pv_vm_enable();
if (rc) {
+ error_report_err(local_err);
+ error_free(pv_mig_blocker);
+ pv_mig_blocker = NULL;
return rc;
}
@@ -470,11 +490,13 @@ static void s390_machine_reset(MachineState *machine)
if (s390_machine_protect(ms)) {
s390_machine_inject_pv_error(cs);
- s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
- return;
+ goto pv_err;
}
run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
+pv_err:
+ /* Continue after the diag308 so the guest knows something went wrong. */
+ s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
break;
default:
g_assert_not_reached();
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v7 03/15] s390x: protvirt: Add migration blocker
2020-03-09 11:21 ` [PATCH v7 03/15] s390x: protvirt: Add migration blocker Janosch Frank
@ 2020-03-09 13:41 ` David Hildenbrand
2020-03-09 13:51 ` Janosch Frank
0 siblings, 1 reply; 45+ messages in thread
From: David Hildenbrand @ 2020-03-09 13:41 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
On 09.03.20 12:21, Janosch Frank wrote:
> Migration is not yet supported.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> hw/s390x/s390-virtio-ccw.c | 26 ++++++++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index f718cfc591..4bb38704ff 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -43,6 +43,9 @@
> #include "sysemu/sysemu.h"
> #include "hw/s390x/pv.h"
> #include <linux/kvm.h>
> +#include "migration/blocker.h"
> +
> +static Error *pv_mig_blocker;
>
> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
> {
> @@ -331,16 +334,33 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
> }
> ms->pv = false;
> }
> + migrate_del_blocker(pv_mig_blocker);
> + error_free(pv_mig_blocker);
> + pv_mig_blocker = NULL;
Maybe use error_free_or_abort(&pv_mig_blocker);
> }
>
> static int s390_machine_protect(S390CcwMachineState *ms)
> {
> + static Error *local_err;
> CPUState *t;
> int rc;
>
> + error_setg(&pv_mig_blocker,
> + "protected VMs are currently not migrateable.");
> + rc = migrate_add_blocker(pv_mig_blocker, &local_err);
> + if (local_err) {
> + error_report_err(local_err);
> + error_free(pv_mig_blocker);
> + pv_mig_blocker = NULL;
Dito.
> + return rc;
> + }
> +
> /* Create SE VM */
> rc = s390_pv_vm_enable();
> if (rc) {
> + error_report_err(local_err);
> + error_free(pv_mig_blocker);
> + pv_mig_blocker = NULL;
Dito.
> return rc;
> }
>
> @@ -470,11 +490,13 @@ static void s390_machine_reset(MachineState *machine)
>
> if (s390_machine_protect(ms)) {
> s390_machine_inject_pv_error(cs);
> - s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
> - return;
> + goto pv_err;
> }
>
> run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
> +pv_err:
> + /* Continue after the diag308 so the guest knows something went wrong. */
> + s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
> break;
Why does this change *not* belong into path #2? I think I asked this
already.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 03/15] s390x: protvirt: Add migration blocker
2020-03-09 13:41 ` David Hildenbrand
@ 2020-03-09 13:51 ` Janosch Frank
2020-03-09 14:15 ` David Hildenbrand
0 siblings, 1 reply; 45+ messages in thread
From: Janosch Frank @ 2020-03-09 13:51 UTC (permalink / raw)
To: David Hildenbrand, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
[-- Attachment #1.1: Type: text/plain, Size: 2746 bytes --]
On 3/9/20 2:41 PM, David Hildenbrand wrote:
> On 09.03.20 12:21, Janosch Frank wrote:
>> Migration is not yet supported.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> hw/s390x/s390-virtio-ccw.c | 26 ++++++++++++++++++++++++--
>> 1 file changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index f718cfc591..4bb38704ff 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -43,6 +43,9 @@
>> #include "sysemu/sysemu.h"
>> #include "hw/s390x/pv.h"
>> #include <linux/kvm.h>
>> +#include "migration/blocker.h"
>> +
>> +static Error *pv_mig_blocker;
>>
>> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>> {
>> @@ -331,16 +334,33 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
>> }
>> ms->pv = false;
>> }
>> + migrate_del_blocker(pv_mig_blocker);
>> + error_free(pv_mig_blocker);
>> + pv_mig_blocker = NULL;
>
> Maybe use error_free_or_abort(&pv_mig_blocker);
I dislike hiding setting the ptr to NULL with that function.
The assert for NULL in error_setg has caused me a lot of headache initially.
But if that's how it's done I'll fix it.
>
>> }
>>
>> static int s390_machine_protect(S390CcwMachineState *ms)
>> {
>> + static Error *local_err;
>> CPUState *t;
>> int rc;
>>
>> + error_setg(&pv_mig_blocker,
>> + "protected VMs are currently not migrateable.");
>> + rc = migrate_add_blocker(pv_mig_blocker, &local_err);
>> + if (local_err) {
>> + error_report_err(local_err);
>> + error_free(pv_mig_blocker);
>> + pv_mig_blocker = NULL;
>
> Dito.
>
>> + return rc;
>> + }
>> +
>> /* Create SE VM */
>> rc = s390_pv_vm_enable();
>> if (rc) {
>> + error_report_err(local_err);
>> + error_free(pv_mig_blocker);
>> + pv_mig_blocker = NULL;
>
> Dito.
>
>> return rc;
>> }
>>
>> @@ -470,11 +490,13 @@ static void s390_machine_reset(MachineState *machine)
>>
>> if (s390_machine_protect(ms)) {
>> s390_machine_inject_pv_error(cs);
>> - s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>> - return;
>> + goto pv_err;
>> }
>>
>> run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>> +pv_err:
>> + /* Continue after the diag308 so the guest knows something went wrong. */
>> + s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>> break;
>
> Why does this change *not* belong into path #2? I think I asked this
> already.
Yes, I'll move it
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 03/15] s390x: protvirt: Add migration blocker
2020-03-09 13:51 ` Janosch Frank
@ 2020-03-09 14:15 ` David Hildenbrand
0 siblings, 0 replies; 45+ messages in thread
From: David Hildenbrand @ 2020-03-09 14:15 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
On 09.03.20 14:51, Janosch Frank wrote:
> On 3/9/20 2:41 PM, David Hildenbrand wrote:
>> On 09.03.20 12:21, Janosch Frank wrote:
>>> Migration is not yet supported.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>> hw/s390x/s390-virtio-ccw.c | 26 ++++++++++++++++++++++++--
>>> 1 file changed, 24 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index f718cfc591..4bb38704ff 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -43,6 +43,9 @@
>>> #include "sysemu/sysemu.h"
>>> #include "hw/s390x/pv.h"
>>> #include <linux/kvm.h>
>>> +#include "migration/blocker.h"
>>> +
>>> +static Error *pv_mig_blocker;
>>>
>>> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>>> {
>>> @@ -331,16 +334,33 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
>>> }
>>> ms->pv = false;
>>> }
>>> + migrate_del_blocker(pv_mig_blocker);
>>> + error_free(pv_mig_blocker);
>>> + pv_mig_blocker = NULL;
>>
>> Maybe use error_free_or_abort(&pv_mig_blocker);
>
> I dislike hiding setting the ptr to NULL with that function.
> The assert for NULL in error_setg has caused me a lot of headache initially.
>
> But if that's how it's done I'll fix it.
No strong feelings on my side. Seemed like a nice function to me :)
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v7 04/15] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-03-09 11:21 [PATCH v7 00/15] s390x: Protected Virtualization support Janosch Frank
` (2 preceding siblings ...)
2020-03-09 11:21 ` [PATCH v7 03/15] s390x: protvirt: Add migration blocker Janosch Frank
@ 2020-03-09 11:21 ` Janosch Frank
2020-03-09 13:42 ` David Hildenbrand
2020-03-09 11:21 ` [PATCH v7 05/15] s390x: protvirt: KVM intercept changes Janosch Frank
` (10 subsequent siblings)
14 siblings, 1 reply; 45+ messages in thread
From: Janosch Frank @ 2020-03-09 11:21 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
Ballooning in protected VMs can only be done when the guest shares the
pages it gives to the host. If pages are not shared, the integrity
checks will fail once those pages have been altered and are given back
to the guest.
Hence, until we have a solution for this in the guest kernel, we
inhibit ballooning when switching into protected mode and reverse that
once we move out of it.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
hw/s390x/s390-virtio-ccw.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 4bb38704ff..2557e214a0 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -41,6 +41,7 @@
#include "hw/qdev-properties.h"
#include "hw/s390x/tod.h"
#include "sysemu/sysemu.h"
+#include "sysemu/balloon.h"
#include "hw/s390x/pv.h"
#include <linux/kvm.h>
#include "migration/blocker.h"
@@ -337,6 +338,7 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
migrate_del_blocker(pv_mig_blocker);
error_free(pv_mig_blocker);
pv_mig_blocker = NULL;
+ qemu_balloon_inhibit(false);
}
static int s390_machine_protect(S390CcwMachineState *ms)
@@ -345,10 +347,12 @@ static int s390_machine_protect(S390CcwMachineState *ms)
CPUState *t;
int rc;
+ qemu_balloon_inhibit(true);
error_setg(&pv_mig_blocker,
"protected VMs are currently not migrateable.");
rc = migrate_add_blocker(pv_mig_blocker, &local_err);
if (local_err) {
+ qemu_balloon_inhibit(false);
error_report_err(local_err);
error_free(pv_mig_blocker);
pv_mig_blocker = NULL;
@@ -358,6 +362,7 @@ static int s390_machine_protect(S390CcwMachineState *ms)
/* Create SE VM */
rc = s390_pv_vm_enable();
if (rc) {
+ qemu_balloon_inhibit(false);
error_report_err(local_err);
error_free(pv_mig_blocker);
pv_mig_blocker = NULL;
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v7 04/15] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-03-09 11:21 ` [PATCH v7 04/15] s390x: protvirt: Inhibit balloon when switching to protected mode Janosch Frank
@ 2020-03-09 13:42 ` David Hildenbrand
2020-03-09 14:00 ` Janosch Frank
0 siblings, 1 reply; 45+ messages in thread
From: David Hildenbrand @ 2020-03-09 13:42 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
On 09.03.20 12:21, Janosch Frank wrote:
> Ballooning in protected VMs can only be done when the guest shares the
> pages it gives to the host. If pages are not shared, the integrity
> checks will fail once those pages have been altered and are given back
> to the guest.
>
> Hence, until we have a solution for this in the guest kernel, we
> inhibit ballooning when switching into protected mode and reverse that
> once we move out of it.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
I think Christian requested to add the next steps.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 04/15] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-03-09 13:42 ` David Hildenbrand
@ 2020-03-09 14:00 ` Janosch Frank
0 siblings, 0 replies; 45+ messages in thread
From: Janosch Frank @ 2020-03-09 14:00 UTC (permalink / raw)
To: David Hildenbrand, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
[-- Attachment #1.1: Type: text/plain, Size: 1743 bytes --]
On 3/9/20 2:42 PM, David Hildenbrand wrote:
> On 09.03.20 12:21, Janosch Frank wrote:
>> Ballooning in protected VMs can only be done when the guest shares the
>> pages it gives to the host. If pages are not shared, the integrity
>> checks will fail once those pages have been altered and are given back
>> to the guest.
>>
>> Hence, until we have a solution for this in the guest kernel, we
>> inhibit ballooning when switching into protected mode and reverse that
>> once we move out of it.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>
> I think Christian requested to add the next steps.
Right, I lost that request somewhere.
This is what I have now:
s390x: protvirt: Inhibit balloon when switching to protected mode
Ballooning in protected VMs can only be done when the guest shares the
pages it gives to the host. If pages are not shared, the integrity
checks will fail once those pages have been altered and are given back
to the guest.
As we currently do not yet have a solution for this we will continue
like this:
1. We block ballooning now in QEMU (with this patch)
2. Later we will provide a change to virtio that removes the blocker
and adds VIRTIO_F_IOMMU_PLATFORM automatically by QEMU when doing the
protvirt switch. This is ok as the guest balloon driver will reject to
work with the IOMMU change
3. later we can fix the guest balloon driver to accept the IOMMU feature
bit and correctly exercise sharing and unsharing of balloon pages
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v7 05/15] s390x: protvirt: KVM intercept changes
2020-03-09 11:21 [PATCH v7 00/15] s390x: Protected Virtualization support Janosch Frank
` (3 preceding siblings ...)
2020-03-09 11:21 ` [PATCH v7 04/15] s390x: protvirt: Inhibit balloon when switching to protected mode Janosch Frank
@ 2020-03-09 11:21 ` Janosch Frank
2020-03-09 11:21 ` [PATCH v7 06/15] s390x: Add SIDA memory ops Janosch Frank
` (9 subsequent siblings)
14 siblings, 0 replies; 45+ messages in thread
From: Janosch Frank @ 2020-03-09 11:21 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
Secure guests no longer intercept with code 4 for an instruction
interception. Instead they have codes 104 and 108 for secure
instruction interception and secure instruction notification
respectively.
The 104 mirrors the 4 interception.
The 108 is a notification interception to let KVM and QEMU know that
something changed and we need to update tracking information or
perform specific tasks. It's currently taken for the following
instructions:
* stpx (To inform about the changed prefix location)
* sclp (On incorrect SCCB values, so we can inject a IRQ)
* sigp (All but "stop and store status")
* diag308 (Subcodes 0/1)
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
target/s390x/kvm.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 1d6fd6a27b..eec0b92479 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -115,6 +115,8 @@
#define ICPT_CPU_STOP 0x28
#define ICPT_OPEREXC 0x2c
#define ICPT_IO 0x40
+#define ICPT_PV_INSTR 0x68
+#define ICPT_PV_INSTR_NOTIFICATION 0x6c
#define NR_LOCAL_IRQS 32
/*
@@ -1693,6 +1695,8 @@ static int handle_intercept(S390CPU *cpu)
(long)cs->kvm_run->psw_addr);
switch (icpt_code) {
case ICPT_INSTRUCTION:
+ case ICPT_PV_INSTR:
+ case ICPT_PV_INSTR_NOTIFICATION:
r = handle_instruction(cpu, run);
break;
case ICPT_PROGRAM:
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v7 06/15] s390x: Add SIDA memory ops
2020-03-09 11:21 [PATCH v7 00/15] s390x: Protected Virtualization support Janosch Frank
` (4 preceding siblings ...)
2020-03-09 11:21 ` [PATCH v7 05/15] s390x: protvirt: KVM intercept changes Janosch Frank
@ 2020-03-09 11:21 ` Janosch Frank
2020-03-09 11:21 ` [PATCH v7 07/15] s390x: protvirt: Move STSI data over SIDAD Janosch Frank
` (8 subsequent siblings)
14 siblings, 0 replies; 45+ messages in thread
From: Janosch Frank @ 2020-03-09 11:21 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
Protected guests save the instruction control blocks in the SIDA
instead of QEMU/KVM directly accessing the guest's memory.
Let's introduce new functions to access the SIDA.
Also the new memops are available with KVM_CAP_S390_PROTECTED, so
let's check for that.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
target/s390x/cpu.h | 7 ++++++-
target/s390x/kvm.c | 25 +++++++++++++++++++++++++
target/s390x/kvm_s390x.h | 2 ++
target/s390x/mmu_helper.c | 14 ++++++++++++++
4 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 7e4d9d267c..72b7700dad 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -824,7 +824,12 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
#define s390_cpu_virt_mem_check_write(cpu, laddr, ar, len) \
s390_cpu_virt_mem_rw(cpu, laddr, ar, NULL, len, true)
void s390_cpu_virt_mem_handle_exc(S390CPU *cpu, uintptr_t ra);
-
+int s390_cpu_pv_mem_rw(S390CPU *cpu, unsigned int offset, void *hostbuf,
+ int len, bool is_write);
+#define s390_cpu_pv_mem_read(cpu, offset, dest, len) \
+ s390_cpu_pv_mem_rw(cpu, offset, dest, len, false)
+#define s390_cpu_pv_mem_write(cpu, offset, dest, len) \
+ s390_cpu_pv_mem_rw(cpu, offset, dest, len, true)
/* sigp.c */
int s390_cpu_restart(S390CPU *cpu);
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index eec0b92479..cdcd538b4f 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -154,6 +154,7 @@ static int cap_ri;
static int cap_gs;
static int cap_hpage_1m;
static int cap_vcpu_resets;
+static int cap_protected;
static int active_cmma;
@@ -346,6 +347,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS);
+ cap_protected = kvm_check_extension(s, KVM_CAP_S390_PROTECTED);
if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
|| !kvm_check_extension(s, KVM_CAP_S390_COW)) {
@@ -846,6 +848,29 @@ int kvm_s390_mem_op(S390CPU *cpu, vaddr addr, uint8_t ar, void *hostbuf,
return ret;
}
+int kvm_s390_mem_op_pv(S390CPU *cpu, uint64_t offset, void *hostbuf,
+ int len, bool is_write)
+{
+ struct kvm_s390_mem_op mem_op = {
+ .sida_offset = offset,
+ .size = len,
+ .op = is_write ? KVM_S390_MEMOP_SIDA_WRITE
+ : KVM_S390_MEMOP_SIDA_READ,
+ .buf = (uint64_t)hostbuf,
+ };
+ int ret;
+
+ if (!cap_mem_op || !cap_protected) {
+ return -ENOSYS;
+ }
+
+ ret = kvm_vcpu_ioctl(CPU(cpu), KVM_S390_MEM_OP, &mem_op);
+ if (ret < 0) {
+ error_report("KVM_S390_MEM_OP failed: %s", strerror(-ret));
+ }
+ return ret;
+}
+
/*
* Legacy layout for s390:
* Older S390 KVM requires the topmost vma of the RAM to be
diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
index 0b21789796..9c38f6ccce 100644
--- a/target/s390x/kvm_s390x.h
+++ b/target/s390x/kvm_s390x.h
@@ -19,6 +19,8 @@ void kvm_s390_vcpu_interrupt(S390CPU *cpu, struct kvm_s390_irq *irq);
void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t te_code);
int kvm_s390_mem_op(S390CPU *cpu, vaddr addr, uint8_t ar, void *hostbuf,
int len, bool is_write);
+int kvm_s390_mem_op_pv(S390CPU *cpu, vaddr addr, void *hostbuf, int len,
+ bool is_write);
void kvm_s390_program_interrupt(S390CPU *cpu, uint16_t code);
int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state);
void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu);
diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index 0be2f300bb..7d9f3059cd 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -474,6 +474,20 @@ static int translate_pages(S390CPU *cpu, vaddr addr, int nr_pages,
return 0;
}
+int s390_cpu_pv_mem_rw(S390CPU *cpu, unsigned int offset, void *hostbuf,
+ int len, bool is_write)
+{
+ int ret;
+
+ if (kvm_enabled()) {
+ ret = kvm_s390_mem_op_pv(cpu, offset, hostbuf, len, is_write);
+ } else {
+ /* Protected Virtualization is a KVM/Hardware only feature */
+ g_assert_not_reached();
+ }
+ return ret;
+}
+
/**
* s390_cpu_virt_mem_rw:
* @laddr: the logical start address
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v7 07/15] s390x: protvirt: Move STSI data over SIDAD
2020-03-09 11:21 [PATCH v7 00/15] s390x: Protected Virtualization support Janosch Frank
` (5 preceding siblings ...)
2020-03-09 11:21 ` [PATCH v7 06/15] s390x: Add SIDA memory ops Janosch Frank
@ 2020-03-09 11:21 ` Janosch Frank
2020-03-09 11:21 ` [PATCH v7 08/15] s390x: protvirt: SCLP interpretation Janosch Frank
` (7 subsequent siblings)
14 siblings, 0 replies; 45+ messages in thread
From: Janosch Frank @ 2020-03-09 11:21 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
For protected guests, we need to put the STSI emulation results into
the SIDA, so SIE will write them into the guest at the next entry.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
target/s390x/kvm.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index cdcd538b4f..295ed12a38 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1797,10 +1797,13 @@ static int handle_tsch(S390CPU *cpu)
static void insert_stsi_3_2_2(S390CPU *cpu, __u64 addr, uint8_t ar)
{
+ CPUS390XState *env = &cpu->env;
SysIB_322 sysib;
int del;
- if (s390_cpu_virt_mem_read(cpu, addr, ar, &sysib, sizeof(sysib))) {
+ if (env->pv) {
+ s390_cpu_pv_mem_read(cpu, 0, &sysib, sizeof(sysib));
+ } else if (s390_cpu_virt_mem_read(cpu, addr, ar, &sysib, sizeof(sysib))) {
return;
}
/* Shift the stack of Extended Names to prepare for our own data */
@@ -1840,7 +1843,11 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64 addr, uint8_t ar)
/* Insert UUID */
memcpy(sysib.vm[0].uuid, &qemu_uuid, sizeof(sysib.vm[0].uuid));
- s390_cpu_virt_mem_write(cpu, addr, ar, &sysib, sizeof(sysib));
+ if (env->pv) {
+ s390_cpu_pv_mem_write(cpu, 0, &sysib, sizeof(sysib));
+ } else {
+ s390_cpu_virt_mem_write(cpu, addr, ar, &sysib, sizeof(sysib));
+ }
}
static int handle_stsi(S390CPU *cpu)
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v7 08/15] s390x: protvirt: SCLP interpretation
2020-03-09 11:21 [PATCH v7 00/15] s390x: Protected Virtualization support Janosch Frank
` (6 preceding siblings ...)
2020-03-09 11:21 ` [PATCH v7 07/15] s390x: protvirt: Move STSI data over SIDAD Janosch Frank
@ 2020-03-09 11:21 ` Janosch Frank
2020-03-09 15:27 ` David Hildenbrand
2020-03-09 11:22 ` [PATCH v7 09/15] s390x: protvirt: Set guest IPL PSW Janosch Frank
` (6 subsequent siblings)
14 siblings, 1 reply; 45+ messages in thread
From: Janosch Frank @ 2020-03-09 11:21 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
SCLP for a protected guest is done over the SIDAD, so we need to use
the s390_cpu_pv_mem_* functions to access the SIDAD instead of guest
memory when reading/writing SCBs.
To not confuse the sclp emulation, we set 0x4000 as the SCCB address,
since the function that injects the sclp external interrupt would
reject a zero sccb address.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
hw/s390x/sclp.c | 28 ++++++++++++++++++++++++++++
include/hw/s390x/sclp.h | 2 ++
target/s390x/kvm.c | 21 ++++++++++++++++-----
3 files changed, 46 insertions(+), 5 deletions(-)
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index af0bfbc2ec..b4bc2dd659 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -193,6 +193,34 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code)
}
}
+/*
+* We only need the address to have something valid for the
+* service_interrupt call.
+*/
+#define SCLP_PV_DUMMY_ADDR 0x4000
+int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
+ uint32_t code)
+{
+ SCLPDevice *sclp = get_sclp_device();
+ SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
+ SCCB work_sccb;
+ hwaddr sccb_len = sizeof(SCCB);
+
+ /*
+ * Only a very limited amount of calls is permitted by the
+ * Ultravisor and we support all of them, so we don't check for
+ * them. All other specification exceptions are also interpreted
+ * by the Ultravisor and hence never cause an exit we need to
+ * handle.
+ */
+ s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
+ sclp_c->execute(sclp, &work_sccb, code);
+ s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
+ be16_to_cpu(work_sccb.h.length));
+ sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR);
+ return 0;
+}
+
int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
{
SCLPDevice *sclp = get_sclp_device();
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index c54413b78c..c0a3faa37d 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -217,5 +217,7 @@ void s390_sclp_init(void);
void sclp_service_interrupt(uint32_t sccb);
void raise_irq_cpu_hotplug(void);
int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
+int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
+ uint32_t code);
#endif
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 295ed12a38..5c7757ac28 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1226,12 +1226,23 @@ static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
sccb = env->regs[ipbh0 & 0xf];
code = env->regs[(ipbh0 & 0xf0) >> 4];
- r = sclp_service_call(env, sccb, code);
- if (r < 0) {
- kvm_s390_program_interrupt(cpu, -r);
- return;
+ switch (run->s390_sieic.icptcode) {
+ case ICPT_PV_INSTR_NOTIFICATION:
+ /* The notification intercepts are currently handled by KVM */
+ fprintf(stderr, "unexpected SCLP PV notification\n");
+ exit(1);
+ break;
+ case ICPT_PV_INSTR:
+ sclp_service_call_protected(env, sccb, code);
+ break;
+ case ICPT_INSTRUCTION:
+ r = sclp_service_call(env, sccb, code);
+ if (r < 0) {
+ kvm_s390_program_interrupt(cpu, -r);
+ return;
+ }
+ setcc(cpu, r);
}
- setcc(cpu, r);
}
static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v7 08/15] s390x: protvirt: SCLP interpretation
2020-03-09 11:21 ` [PATCH v7 08/15] s390x: protvirt: SCLP interpretation Janosch Frank
@ 2020-03-09 15:27 ` David Hildenbrand
2020-03-09 16:05 ` Janosch Frank
2020-03-10 8:42 ` [PATCH v8] " Janosch Frank
0 siblings, 2 replies; 45+ messages in thread
From: David Hildenbrand @ 2020-03-09 15:27 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
On 09.03.20 12:21, Janosch Frank wrote:
> SCLP for a protected guest is done over the SIDAD, so we need to use
> the s390_cpu_pv_mem_* functions to access the SIDAD instead of guest
> memory when reading/writing SCBs.
>
> To not confuse the sclp emulation, we set 0x4000 as the SCCB address,
> since the function that injects the sclp external interrupt would
> reject a zero sccb address.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> hw/s390x/sclp.c | 28 ++++++++++++++++++++++++++++
> include/hw/s390x/sclp.h | 2 ++
> target/s390x/kvm.c | 21 ++++++++++++++++-----
> 3 files changed, 46 insertions(+), 5 deletions(-)
>
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index af0bfbc2ec..b4bc2dd659 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -193,6 +193,34 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code)
> }
> }
>
> +/*
> +* We only need the address to have something valid for the
> +* service_interrupt call.
> +*/
> +#define SCLP_PV_DUMMY_ADDR 0x4000
> +int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
> + uint32_t code)
> +{
> + SCLPDevice *sclp = get_sclp_device();
> + SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
> + SCCB work_sccb;
> + hwaddr sccb_len = sizeof(SCCB);
> +
> + /*
> + * Only a very limited amount of calls is permitted by the
> + * Ultravisor and we support all of them, so we don't check for
> + * them. All other specification exceptions are also interpreted
> + * by the Ultravisor and hence never cause an exit we need to
> + * handle.
> + */
> + s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
> + sclp_c->execute(sclp, &work_sccb, code);
> + s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
> + be16_to_cpu(work_sccb.h.length));
> + sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR);
> + return 0;
> +}
> +
> int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
> {
> SCLPDevice *sclp = get_sclp_device();
> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
> index c54413b78c..c0a3faa37d 100644
> --- a/include/hw/s390x/sclp.h
> +++ b/include/hw/s390x/sclp.h
> @@ -217,5 +217,7 @@ void s390_sclp_init(void);
> void sclp_service_interrupt(uint32_t sccb);
> void raise_irq_cpu_hotplug(void);
> int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
> +int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
> + uint32_t code);
>
> #endif
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 295ed12a38..5c7757ac28 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1226,12 +1226,23 @@ static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
> sccb = env->regs[ipbh0 & 0xf];
> code = env->regs[(ipbh0 & 0xf0) >> 4];
>
> - r = sclp_service_call(env, sccb, code);
> - if (r < 0) {
> - kvm_s390_program_interrupt(cpu, -r);
> - return;
> + switch (run->s390_sieic.icptcode) {
> + case ICPT_PV_INSTR_NOTIFICATION:
> + /* The notification intercepts are currently handled by KVM */
> + fprintf(stderr, "unexpected SCLP PV notification\n");
Maybe g_assert(env->pv);
error_report() ?
> + exit(1);
> + break;
> + case ICPT_PV_INSTR:
> + sclp_service_call_protected(env, sccb, code);
> + break;
> + case ICPT_INSTRUCTION:
Maybe g_assert(!env->pv);
> + r = sclp_service_call(env, sccb, code);
> + if (r < 0) {
> + kvm_s390_program_interrupt(cpu, -r);
> + return;
> + }
> + setcc(cpu, r);
> }
> - setcc(cpu, r);
What about the cc in case of ICPT_PV_INSTR?
> }
>
> static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 08/15] s390x: protvirt: SCLP interpretation
2020-03-09 15:27 ` David Hildenbrand
@ 2020-03-09 16:05 ` Janosch Frank
2020-03-10 8:42 ` [PATCH v8] " Janosch Frank
1 sibling, 0 replies; 45+ messages in thread
From: Janosch Frank @ 2020-03-09 16:05 UTC (permalink / raw)
To: David Hildenbrand, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
[-- Attachment #1.1: Type: text/plain, Size: 4295 bytes --]
On 3/9/20 4:27 PM, David Hildenbrand wrote:
> On 09.03.20 12:21, Janosch Frank wrote:
>> SCLP for a protected guest is done over the SIDAD, so we need to use
>> the s390_cpu_pv_mem_* functions to access the SIDAD instead of guest
>> memory when reading/writing SCBs.
>>
>> To not confuse the sclp emulation, we set 0x4000 as the SCCB address,
>> since the function that injects the sclp external interrupt would
>> reject a zero sccb address.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> hw/s390x/sclp.c | 28 ++++++++++++++++++++++++++++
>> include/hw/s390x/sclp.h | 2 ++
>> target/s390x/kvm.c | 21 ++++++++++++++++-----
>> 3 files changed, 46 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index af0bfbc2ec..b4bc2dd659 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -193,6 +193,34 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code)
>> }
>> }
>>
>> +/*
>> +* We only need the address to have something valid for the
>> +* service_interrupt call.
>> +*/
>> +#define SCLP_PV_DUMMY_ADDR 0x4000
>> +int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>> + uint32_t code)
>> +{
>> + SCLPDevice *sclp = get_sclp_device();
>> + SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
>> + SCCB work_sccb;
>> + hwaddr sccb_len = sizeof(SCCB);
>> +
>> + /*
>> + * Only a very limited amount of calls is permitted by the
>> + * Ultravisor and we support all of them, so we don't check for
>> + * them. All other specification exceptions are also interpreted
>> + * by the Ultravisor and hence never cause an exit we need to
>> + * handle.
>> + */
>> + s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
>> + sclp_c->execute(sclp, &work_sccb, code);
>> + s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
>> + be16_to_cpu(work_sccb.h.length));
>> + sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR);
>> + return 0;
>> +}
>> +
>> int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>> {
>> SCLPDevice *sclp = get_sclp_device();
>> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
>> index c54413b78c..c0a3faa37d 100644
>> --- a/include/hw/s390x/sclp.h
>> +++ b/include/hw/s390x/sclp.h
>> @@ -217,5 +217,7 @@ void s390_sclp_init(void);
>> void sclp_service_interrupt(uint32_t sccb);
>> void raise_irq_cpu_hotplug(void);
>> int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
>> +int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>> + uint32_t code);
>>
>> #endif
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index 295ed12a38..5c7757ac28 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -1226,12 +1226,23 @@ static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
>> sccb = env->regs[ipbh0 & 0xf];
>> code = env->regs[(ipbh0 & 0xf0) >> 4];
>>
>> - r = sclp_service_call(env, sccb, code);
>> - if (r < 0) {
>> - kvm_s390_program_interrupt(cpu, -r);
>> - return;
>> + switch (run->s390_sieic.icptcode) {
>> + case ICPT_PV_INSTR_NOTIFICATION:
>> + /* The notification intercepts are currently handled by KVM */
>> + fprintf(stderr, "unexpected SCLP PV notification\n");
>
> Maybe g_assert(env->pv);
>
> error_report() ?
Sure, I just followed suit with the other aborts in this file.
>
>> + exit(1);
>> + break;
>> + case ICPT_PV_INSTR:
>> + sclp_service_call_protected(env, sccb, code);
>> + break;
>> + case ICPT_INSTRUCTION:
>
> Maybe g_assert(!env->pv);
>
>> + r = sclp_service_call(env, sccb, code);
>> + if (r < 0) {
>> + kvm_s390_program_interrupt(cpu, -r);
>> + return;
>> + }
>> + setcc(cpu, r);
>> }
>> - setcc(cpu, r);
>
> What about the cc in case of ICPT_PV_INSTR?
Set to 0 by the Ultravisor/SIE
>
>> }
>>
>> static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
>>
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread* [PATCH v8] s390x: protvirt: SCLP interpretation
2020-03-09 15:27 ` David Hildenbrand
2020-03-09 16:05 ` Janosch Frank
@ 2020-03-10 8:42 ` Janosch Frank
2020-03-10 9:01 ` David Hildenbrand
1 sibling, 1 reply; 45+ messages in thread
From: Janosch Frank @ 2020-03-10 8:42 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
SCLP for a protected guest is done over the SIDAD, so we need to use
the s390_cpu_pv_mem_* functions to access the SIDAD instead of guest
memory when reading/writing SCBs.
To not confuse the sclp emulation, we set 0x4000 as the SCCB address,
since the function that injects the sclp external interrupt would
reject a zero sccb address.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
hw/s390x/sclp.c | 30 ++++++++++++++++++++++++++++++
include/hw/s390x/sclp.h | 2 ++
target/s390x/kvm.c | 24 +++++++++++++++++++-----
3 files changed, 51 insertions(+), 5 deletions(-)
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index af0bfbc2ec..15dd15099c 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -193,6 +193,36 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code)
}
}
+/*
+* We only need the address to have something valid for the
+* service_interrupt call.
+*/
+#define SCLP_PV_DUMMY_ADDR 0x4000
+int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
+ uint32_t code)
+{
+ SCLPDevice *sclp = get_sclp_device();
+ SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
+ SCCB work_sccb;
+ hwaddr sccb_len = sizeof(SCCB);
+
+ /*
+ * Only a very limited amount of calls is permitted by the
+ * Ultravisor and we support all of them, so we don't check for
+ * them. All other specification exceptions are also interpreted
+ * by the Ultravisor and hence never cause an exit we need to
+ * handle.
+ *
+ * Setting the CC is also done by the Ultravisor.
+ */
+ s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
+ sclp_c->execute(sclp, &work_sccb, code);
+ s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
+ be16_to_cpu(work_sccb.h.length));
+ sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR);
+ return 0;
+}
+
int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
{
SCLPDevice *sclp = get_sclp_device();
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index c54413b78c..c0a3faa37d 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -217,5 +217,7 @@ void s390_sclp_init(void);
void sclp_service_interrupt(uint32_t sccb);
void raise_irq_cpu_hotplug(void);
int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
+int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
+ uint32_t code);
#endif
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 295ed12a38..78e1b34ad5 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1226,12 +1226,26 @@ static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
sccb = env->regs[ipbh0 & 0xf];
code = env->regs[(ipbh0 & 0xf0) >> 4];
- r = sclp_service_call(env, sccb, code);
- if (r < 0) {
- kvm_s390_program_interrupt(cpu, -r);
- return;
+ switch (run->s390_sieic.icptcode) {
+ case ICPT_PV_INSTR_NOTIFICATION:
+ g_assert(env->pv);
+ /* The notification intercepts are currently handled by KVM */
+ error_report("unexpected SCLP PV notification\n");
+ exit(1);
+ break;
+ case ICPT_PV_INSTR:
+ g_assert(env->pv);
+ sclp_service_call_protected(env, sccb, code);
+ break;
+ case ICPT_INSTRUCTION:
+ g_assert(!env->pv);
+ r = sclp_service_call(env, sccb, code);
+ if (r < 0) {
+ kvm_s390_program_interrupt(cpu, -r);
+ return;
+ }
+ setcc(cpu, r);
}
- setcc(cpu, r);
}
static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v8] s390x: protvirt: SCLP interpretation
2020-03-10 8:42 ` [PATCH v8] " Janosch Frank
@ 2020-03-10 9:01 ` David Hildenbrand
0 siblings, 0 replies; 45+ messages in thread
From: David Hildenbrand @ 2020-03-10 9:01 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
On 10.03.20 09:42, Janosch Frank wrote:
> SCLP for a protected guest is done over the SIDAD, so we need to use
> the s390_cpu_pv_mem_* functions to access the SIDAD instead of guest
> memory when reading/writing SCBs.
>
> To not confuse the sclp emulation, we set 0x4000 as the SCCB address,
> since the function that injects the sclp external interrupt would
> reject a zero sccb address.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> hw/s390x/sclp.c | 30 ++++++++++++++++++++++++++++++
> include/hw/s390x/sclp.h | 2 ++
> target/s390x/kvm.c | 24 +++++++++++++++++++-----
> 3 files changed, 51 insertions(+), 5 deletions(-)
>
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index af0bfbc2ec..15dd15099c 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -193,6 +193,36 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code)
> }
> }
>
> +/*
> +* We only need the address to have something valid for the
> +* service_interrupt call.
> +*/
> +#define SCLP_PV_DUMMY_ADDR 0x4000
> +int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
> + uint32_t code)
> +{
> + SCLPDevice *sclp = get_sclp_device();
> + SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
> + SCCB work_sccb;
> + hwaddr sccb_len = sizeof(SCCB);
> +
> + /*
> + * Only a very limited amount of calls is permitted by the
> + * Ultravisor and we support all of them, so we don't check for
> + * them. All other specification exceptions are also interpreted
> + * by the Ultravisor and hence never cause an exit we need to
> + * handle.
> + *
> + * Setting the CC is also done by the Ultravisor.
> + */
> + s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
> + sclp_c->execute(sclp, &work_sccb, code);
> + s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
> + be16_to_cpu(work_sccb.h.length));
> + sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR);
> + return 0;
> +}
> +
> int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
> {
> SCLPDevice *sclp = get_sclp_device();
> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
> index c54413b78c..c0a3faa37d 100644
> --- a/include/hw/s390x/sclp.h
> +++ b/include/hw/s390x/sclp.h
> @@ -217,5 +217,7 @@ void s390_sclp_init(void);
> void sclp_service_interrupt(uint32_t sccb);
> void raise_irq_cpu_hotplug(void);
> int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
> +int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
> + uint32_t code);
>
> #endif
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 295ed12a38..78e1b34ad5 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1226,12 +1226,26 @@ static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
> sccb = env->regs[ipbh0 & 0xf];
> code = env->regs[(ipbh0 & 0xf0) >> 4];
>
> - r = sclp_service_call(env, sccb, code);
> - if (r < 0) {
> - kvm_s390_program_interrupt(cpu, -r);
> - return;
> + switch (run->s390_sieic.icptcode) {
> + case ICPT_PV_INSTR_NOTIFICATION:
> + g_assert(env->pv);
> + /* The notification intercepts are currently handled by KVM */
> + error_report("unexpected SCLP PV notification\n");
> + exit(1);
> + break;
> + case ICPT_PV_INSTR:
> + g_assert(env->pv);
> + sclp_service_call_protected(env, sccb, code);
> + break;
> + case ICPT_INSTRUCTION:
> + g_assert(!env->pv);
> + r = sclp_service_call(env, sccb, code);
> + if (r < 0) {
> + kvm_s390_program_interrupt(cpu, -r);
> + return;
> + }
> + setcc(cpu, r);
> }
> - setcc(cpu, r);
> }
>
> static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
>
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v7 09/15] s390x: protvirt: Set guest IPL PSW
2020-03-09 11:21 [PATCH v7 00/15] s390x: Protected Virtualization support Janosch Frank
` (7 preceding siblings ...)
2020-03-09 11:21 ` [PATCH v7 08/15] s390x: protvirt: SCLP interpretation Janosch Frank
@ 2020-03-09 11:22 ` Janosch Frank
2020-03-09 11:22 ` [PATCH v7 10/15] s390x: protvirt: Move diag 308 data over SIDA Janosch Frank
` (5 subsequent siblings)
14 siblings, 0 replies; 45+ messages in thread
From: Janosch Frank @ 2020-03-09 11:22 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
Handling of CPU reset and setting of the IPL psw from guest storage at
offset 0 is done by a Ultravisor call. Let's only fetch it if
necessary.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
target/s390x/cpu.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 5109b8bbf5..c513f8efe0 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -78,16 +78,21 @@ static bool s390_cpu_has_work(CPUState *cs)
static void s390_cpu_load_normal(CPUState *s)
{
S390CPU *cpu = S390_CPU(s);
- uint64_t spsw = ldq_phys(s->as, 0);
-
- cpu->env.psw.mask = spsw & PSW_MASK_SHORT_CTRL;
- /*
- * Invert short psw indication, so SIE will report a specification
- * exception if it was not set.
- */
- cpu->env.psw.mask ^= PSW_MASK_SHORTPSW;
- cpu->env.psw.addr = spsw & PSW_MASK_SHORT_ADDR;
+ CPUS390XState *env = &cpu->env;
+ uint64_t spsw;
+ if (!env->pv) {
+ spsw = ldq_phys(s->as, 0);
+ cpu->env.psw.mask = spsw & PSW_MASK_SHORT_CTRL;
+ /*
+ * Invert short psw indication, so SIE will report a specification
+ * exception if it was not set.
+ */
+ cpu->env.psw.mask ^= PSW_MASK_SHORTPSW;
+ cpu->env.psw.addr = spsw & PSW_MASK_SHORT_ADDR;
+ } else {
+ s390_cpu_set_state(S390_CPU_STATE_LOAD, cpu);
+ }
s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
}
#endif
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v7 10/15] s390x: protvirt: Move diag 308 data over SIDA
2020-03-09 11:21 [PATCH v7 00/15] s390x: Protected Virtualization support Janosch Frank
` (8 preceding siblings ...)
2020-03-09 11:22 ` [PATCH v7 09/15] s390x: protvirt: Set guest IPL PSW Janosch Frank
@ 2020-03-09 11:22 ` Janosch Frank
2020-03-09 11:22 ` [PATCH v7 11/15] s390x: protvirt: Disable address checks for PV guest IO emulation Janosch Frank
` (4 subsequent siblings)
14 siblings, 0 replies; 45+ messages in thread
From: Janosch Frank @ 2020-03-09 11:22 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
For protected guests the IPIB is written/read to/from the SIDA, so we
need those accesses to go through s390_cpu_pv_mem_read/write().
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
target/s390x/diag.c | 30 +++++++++++++++++++++++-------
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index 670db0eb53..4d1aa09b49 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -87,6 +87,7 @@ static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
{
CPUState *cs = env_cpu(env);
+ S390CPU *cpu = S390_CPU(cs);
uint64_t addr = env->regs[r1];
uint64_t subcode = env->regs[r3];
IplParameterBlock *iplb;
@@ -123,13 +124,22 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
return;
}
iplb = g_new0(IplParameterBlock, 1);
- cpu_physical_memory_read(addr, iplb, sizeof(iplb->len));
+ if (!env->pv) {
+ cpu_physical_memory_read(addr, iplb, sizeof(iplb->len));
+ } else {
+ s390_cpu_pv_mem_read(cpu, 0, iplb, sizeof(iplb->len));
+ }
+
if (!iplb_valid_len(iplb)) {
env->regs[r1 + 1] = DIAG_308_RC_INVALID;
goto out;
}
- cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
+ if (!env->pv) {
+ cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
+ } else {
+ s390_cpu_pv_mem_read(cpu, 0, iplb, be32_to_cpu(iplb->len));
+ }
if (!iplb_valid(iplb)) {
env->regs[r1 + 1] = DIAG_308_RC_INVALID;
@@ -140,7 +150,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
env->regs[r1 + 1] = DIAG_308_RC_OK;
out:
g_free(iplb);
- return;
+ break;
case DIAG308_STORE:
case DIAG308_PV_STORE:
if (diag308_parm_check(env, r1, addr, ra, true)) {
@@ -151,12 +161,18 @@ out:
} 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 {
+ if (!iplb) {
env->regs[r1 + 1] = DIAG_308_RC_NO_CONF;
+ return;
}
+
+ if (!env->pv) {
+ cpu_physical_memory_write(addr, iplb, be32_to_cpu(iplb->len));
+ } else {
+ s390_cpu_pv_mem_write(cpu, 0, iplb, be32_to_cpu(iplb->len));
+ }
+
+ env->regs[r1 + 1] = DIAG_308_RC_OK;
break;
case DIAG308_PV_START:
iplb = s390_ipl_get_iplb_pv();
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v7 11/15] s390x: protvirt: Disable address checks for PV guest IO emulation
2020-03-09 11:21 [PATCH v7 00/15] s390x: Protected Virtualization support Janosch Frank
` (9 preceding siblings ...)
2020-03-09 11:22 ` [PATCH v7 10/15] s390x: protvirt: Move diag 308 data over SIDA Janosch Frank
@ 2020-03-09 11:22 ` Janosch Frank
2020-03-09 11:22 ` [PATCH v7 12/15] s390x: protvirt: Move IO control structures over SIDA Janosch Frank
` (3 subsequent siblings)
14 siblings, 0 replies; 45+ messages in thread
From: Janosch Frank @ 2020-03-09 11:22 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
IO instruction data is routed through SIDAD for protected guests, so
adresses do not need to be checked, as this is kernel memory.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
target/s390x/ioinst.c | 32 +++++++++++++++++++++++++-------
1 file changed, 25 insertions(+), 7 deletions(-)
diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
index c437a1d8c6..a63d29534f 100644
--- a/target/s390x/ioinst.c
+++ b/target/s390x/ioinst.c
@@ -17,6 +17,22 @@
#include "trace.h"
#include "hw/s390x/s390-pci-bus.h"
+static uint64_t get_address_from_regs(CPUS390XState *env, uint32_t ipb,
+ uint8_t *ar)
+{
+ /*
+ * Addresses for protected guests are all offsets into the
+ * satellite block which holds the IO control structures. Those
+ * control structures are always aligned and accessible, so we can
+ * return 0 here which will pass the following address checks.
+ */
+ if (env->pv) {
+ *ar = 0;
+ return 0;
+ }
+ return decode_basedisp_s(env, ipb, ar);
+}
+
int ioinst_disassemble_sch_ident(uint32_t value, int *m, int *cssid, int *ssid,
int *schid)
{
@@ -114,7 +130,7 @@ void ioinst_handle_msch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra)
CPUS390XState *env = &cpu->env;
uint8_t ar;
- addr = decode_basedisp_s(env, ipb, &ar);
+ addr = get_address_from_regs(env, ipb, &ar);
if (addr & 3) {
s390_program_interrupt(env, PGM_SPECIFICATION, ra);
return;
@@ -171,7 +187,7 @@ void ioinst_handle_ssch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra)
CPUS390XState *env = &cpu->env;
uint8_t ar;
- addr = decode_basedisp_s(env, ipb, &ar);
+ addr = get_address_from_regs(env, ipb, &ar);
if (addr & 3) {
s390_program_interrupt(env, PGM_SPECIFICATION, ra);
return;
@@ -203,7 +219,7 @@ void ioinst_handle_stcrw(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
CPUS390XState *env = &cpu->env;
uint8_t ar;
- addr = decode_basedisp_s(env, ipb, &ar);
+ addr = get_address_from_regs(env, ipb, &ar);
if (addr & 3) {
s390_program_interrupt(env, PGM_SPECIFICATION, ra);
return;
@@ -234,7 +250,7 @@ void ioinst_handle_stsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb,
CPUS390XState *env = &cpu->env;
uint8_t ar;
- addr = decode_basedisp_s(env, ipb, &ar);
+ addr = get_address_from_regs(env, ipb, &ar);
if (addr & 3) {
s390_program_interrupt(env, PGM_SPECIFICATION, ra);
return;
@@ -303,7 +319,7 @@ int ioinst_handle_tsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra)
return -EIO;
}
trace_ioinst_sch_id("tsch", cssid, ssid, schid);
- addr = decode_basedisp_s(env, ipb, &ar);
+ addr = get_address_from_regs(env, ipb, &ar);
if (addr & 3) {
s390_program_interrupt(env, PGM_SPECIFICATION, ra);
return -EIO;
@@ -601,7 +617,7 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
{
ChscReq *req;
ChscResp *res;
- uint64_t addr;
+ uint64_t addr = 0;
int reg;
uint16_t len;
uint16_t command;
@@ -610,7 +626,9 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
trace_ioinst("chsc");
reg = (ipb >> 20) & 0x00f;
- addr = env->regs[reg];
+ if (!env->pv) {
+ addr = env->regs[reg];
+ }
/* Page boundary? */
if (addr & 0xfff) {
s390_program_interrupt(env, PGM_SPECIFICATION, ra);
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v7 12/15] s390x: protvirt: Move IO control structures over SIDA
2020-03-09 11:21 [PATCH v7 00/15] s390x: Protected Virtualization support Janosch Frank
` (10 preceding siblings ...)
2020-03-09 11:22 ` [PATCH v7 11/15] s390x: protvirt: Disable address checks for PV guest IO emulation Janosch Frank
@ 2020-03-09 11:22 ` Janosch Frank
2020-03-09 11:22 ` [PATCH v7 13/15] s390x: protvirt: Handle SIGP store status correctly Janosch Frank
` (2 subsequent siblings)
14 siblings, 0 replies; 45+ messages in thread
From: Janosch Frank @ 2020-03-09 11:22 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
For protected guests, we need to put the IO emulation results into the
SIDA, so SIE will write them into the guest at the next entry.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
target/s390x/ioinst.c | 87 ++++++++++++++++++++++++++++++-------------
1 file changed, 61 insertions(+), 26 deletions(-)
diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
index a63d29534f..ba812b75c2 100644
--- a/target/s390x/ioinst.c
+++ b/target/s390x/ioinst.c
@@ -135,9 +135,13 @@ void ioinst_handle_msch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra)
s390_program_interrupt(env, PGM_SPECIFICATION, ra);
return;
}
- if (s390_cpu_virt_mem_read(cpu, addr, ar, &schib, sizeof(schib))) {
- s390_cpu_virt_mem_handle_exc(cpu, ra);
- return;
+ if (env->pv) {
+ s390_cpu_pv_mem_read(cpu, addr, &schib, sizeof(schib));
+ } else {
+ if (s390_cpu_virt_mem_read(cpu, addr, ar, &schib, sizeof(schib))) {
+ s390_cpu_virt_mem_handle_exc(cpu, ra);
+ return;
+ }
}
if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid) ||
!ioinst_schib_valid(&schib)) {
@@ -192,9 +196,13 @@ void ioinst_handle_ssch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra)
s390_program_interrupt(env, PGM_SPECIFICATION, ra);
return;
}
- if (s390_cpu_virt_mem_read(cpu, addr, ar, &orig_orb, sizeof(orb))) {
- s390_cpu_virt_mem_handle_exc(cpu, ra);
- return;
+ if (env->pv) {
+ s390_cpu_pv_mem_read(cpu, addr, &orig_orb, sizeof(orb));
+ } else {
+ if (s390_cpu_virt_mem_read(cpu, addr, ar, &orig_orb, sizeof(orb))) {
+ s390_cpu_virt_mem_handle_exc(cpu, ra);
+ return;
+ }
}
copy_orb_from_guest(&orb, &orig_orb);
if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid) ||
@@ -228,14 +236,19 @@ void ioinst_handle_stcrw(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
cc = css_do_stcrw(&crw);
/* 0 - crw stored, 1 - zeroes stored */
- if (s390_cpu_virt_mem_write(cpu, addr, ar, &crw, sizeof(crw)) == 0) {
+ if (env->pv) {
+ s390_cpu_pv_mem_write(cpu, addr, &crw, sizeof(crw));
setcc(cpu, cc);
} else {
- if (cc == 0) {
- /* Write failed: requeue CRW since STCRW is suppressing */
- css_undo_stcrw(&crw);
+ if (s390_cpu_virt_mem_write(cpu, addr, ar, &crw, sizeof(crw)) == 0) {
+ setcc(cpu, cc);
+ } else {
+ if (cc == 0) {
+ /* Write failed: requeue CRW since STCRW is suppressing */
+ css_undo_stcrw(&crw);
+ }
+ s390_cpu_virt_mem_handle_exc(cpu, ra);
}
- s390_cpu_virt_mem_handle_exc(cpu, ra);
}
}
@@ -257,6 +270,9 @@ void ioinst_handle_stsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb,
}
if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
+ if (env->pv) {
+ return;
+ }
/*
* As operand exceptions have a lower priority than access exceptions,
* we check whether the memory area is writeable (injecting the
@@ -289,14 +305,19 @@ void ioinst_handle_stsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb,
}
}
if (cc != 3) {
- if (s390_cpu_virt_mem_write(cpu, addr, ar, &schib,
- sizeof(schib)) != 0) {
- s390_cpu_virt_mem_handle_exc(cpu, ra);
- return;
+ if (env->pv) {
+ s390_cpu_pv_mem_write(cpu, addr, &schib, sizeof(schib));
+ } else {
+ if (s390_cpu_virt_mem_write(cpu, addr, ar, &schib,
+ sizeof(schib)) != 0) {
+ s390_cpu_virt_mem_handle_exc(cpu, ra);
+ return;
+ }
}
} else {
/* Access exceptions have a higher priority than cc3 */
- if (s390_cpu_virt_mem_check_write(cpu, addr, ar, sizeof(schib)) != 0) {
+ if (!env->pv &&
+ s390_cpu_virt_mem_check_write(cpu, addr, ar, sizeof(schib)) != 0) {
s390_cpu_virt_mem_handle_exc(cpu, ra);
return;
}
@@ -333,15 +354,20 @@ int ioinst_handle_tsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra)
}
/* 0 - status pending, 1 - not status pending, 3 - not operational */
if (cc != 3) {
- if (s390_cpu_virt_mem_write(cpu, addr, ar, &irb, irb_len) != 0) {
- s390_cpu_virt_mem_handle_exc(cpu, ra);
- return -EFAULT;
+ if (env->pv) {
+ s390_cpu_pv_mem_write(cpu, addr, &irb, irb_len);
+ } else {
+ if (s390_cpu_virt_mem_write(cpu, addr, ar, &irb, irb_len) != 0) {
+ s390_cpu_virt_mem_handle_exc(cpu, ra);
+ return -EFAULT;
+ }
}
css_do_tsch_update_subch(sch);
} else {
irb_len = sizeof(irb) - sizeof(irb.emw);
/* Access exceptions have a higher priority than cc3 */
- if (s390_cpu_virt_mem_check_write(cpu, addr, ar, irb_len) != 0) {
+ if (!env->pv &&
+ s390_cpu_virt_mem_check_write(cpu, addr, ar, irb_len) != 0) {
s390_cpu_virt_mem_handle_exc(cpu, ra);
return -EFAULT;
}
@@ -639,9 +665,13 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
* present CHSC sub-handlers ... if we ever need more, we should take
* care of req->len here first.
*/
- if (s390_cpu_virt_mem_read(cpu, addr, reg, buf, sizeof(ChscReq))) {
- s390_cpu_virt_mem_handle_exc(cpu, ra);
- return;
+ if (env->pv) {
+ s390_cpu_pv_mem_read(cpu, addr, buf, sizeof(ChscReq));
+ } else {
+ if (s390_cpu_virt_mem_read(cpu, addr, reg, buf, sizeof(ChscReq))) {
+ s390_cpu_virt_mem_handle_exc(cpu, ra);
+ return;
+ }
}
req = (ChscReq *)buf;
len = be16_to_cpu(req->len);
@@ -672,11 +702,16 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
break;
}
- if (!s390_cpu_virt_mem_write(cpu, addr + len, reg, res,
- be16_to_cpu(res->len))) {
+ if (env->pv) {
+ s390_cpu_pv_mem_write(cpu, addr + len, res, be16_to_cpu(res->len));
setcc(cpu, 0); /* Command execution complete */
} else {
- s390_cpu_virt_mem_handle_exc(cpu, ra);
+ if (!s390_cpu_virt_mem_write(cpu, addr + len, reg, res,
+ be16_to_cpu(res->len))) {
+ setcc(cpu, 0); /* Command execution complete */
+ } else {
+ s390_cpu_virt_mem_handle_exc(cpu, ra);
+ }
}
}
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v7 13/15] s390x: protvirt: Handle SIGP store status correctly
2020-03-09 11:21 [PATCH v7 00/15] s390x: Protected Virtualization support Janosch Frank
` (11 preceding siblings ...)
2020-03-09 11:22 ` [PATCH v7 12/15] s390x: protvirt: Move IO control structures over SIDA Janosch Frank
@ 2020-03-09 11:22 ` Janosch Frank
2020-03-09 11:22 ` [PATCH v7 14/15] docs: Add protvirt docs Janosch Frank
2020-03-09 11:22 ` [PATCH v7 15/15] s390x: Add unpack facility feature to GA1 Janosch Frank
14 siblings, 0 replies; 45+ messages in thread
From: Janosch Frank @ 2020-03-09 11:22 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
For protected VMs status storing is not done by QEMU anymore.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
target/s390x/helper.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index ed72684911..4851452439 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -246,6 +246,11 @@ int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch)
hwaddr len = sizeof(*sa);
int i;
+ /* Storing will occur on next SIE entry for protected VMs */
+ if (cpu->env.pv) {
+ return 0;
+ }
+
sa = cpu_physical_memory_map(addr, &len, true);
if (!sa) {
return -EFAULT;
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v7 14/15] docs: Add protvirt docs
2020-03-09 11:21 [PATCH v7 00/15] s390x: Protected Virtualization support Janosch Frank
` (12 preceding siblings ...)
2020-03-09 11:22 ` [PATCH v7 13/15] s390x: protvirt: Handle SIGP store status correctly Janosch Frank
@ 2020-03-09 11:22 ` Janosch Frank
2020-03-09 11:22 ` [PATCH v7 15/15] s390x: Add unpack facility feature to GA1 Janosch Frank
14 siblings, 0 replies; 45+ messages in thread
From: Janosch Frank @ 2020-03-09 11:22 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
Lets add some documentation for the Protected VM functionality.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
docs/system/index.rst | 1 +
docs/system/protvirt.rst | 56 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 57 insertions(+)
create mode 100644 docs/system/protvirt.rst
diff --git a/docs/system/index.rst b/docs/system/index.rst
index 1a4b2c82ac..d2dc63b973 100644
--- a/docs/system/index.rst
+++ b/docs/system/index.rst
@@ -16,3 +16,4 @@ Contents:
qemu-block-drivers
vfio-ap
+ protvirt
diff --git a/docs/system/protvirt.rst b/docs/system/protvirt.rst
new file mode 100644
index 0000000000..6c8cf0f791
--- /dev/null
+++ b/docs/system/protvirt.rst
@@ -0,0 +1,56 @@
+Protected Virtualization on s390x
+=================================
+
+The memory and most of the registers of Protected Virtual Machines
+(PVMs) are encrypted or inaccessible to the hypervisor, effectively
+prohibiting VM introspection when the VM is running. At rest, PVMs are
+encrypted and can only be decrypted by the firmware, represented by an
+entity called Ultravisor, of specific IBM Z machines.
+
+
+Prerequisites
+-------------
+
+To run PVMs a machine with the Protected Virtualization feature
+which is indicated by the Ultravisor Call facility (stfle bit
+158) is required. The Ultravisor needs to be initialized at boot by
+setting `prot_virt=1` on the kernel command line.
+
+If those requirements are met, the capability `KVM_CAP_S390_PROTECTED`
+will indicate that KVM can support PVMs on that LPAR.
+
+
+QEMU Settings
+-------------
+
+To indicate to the VM that it can transition into protected mode, the
+`Unpack facility` (stfle bit 161 represented by the feature
+`S390_FEAT_UNPACK`) needs to be part of the cpu model of the VM.
+
+All I/O devices need to use the IOMMU.
+Passthrough (vfio) devices are currently not supported.
+
+Host huge page backings are not supported. However guests can use huge
+pages as indicated by its facilities.
+
+
+Boot Process
+------------
+
+A secure guest image can either be loaded from disk or supplied on the
+QEMU command line. Booting from disk is done by the unmodified
+s390-ccw BIOS. I.e., the bootmap is interpreted, multiple components
+are read into memory and control is transferred to one of the
+components (zipl stage3). Stag3 does some fixups and then transfers
+control to some program residing in guest memory, which is normally
+the OS kernel. The secure image has another component prepended
+(stage3a) that uses the new diag308 subcodes 8 and 10 to trigger the
+transition into secure mode.
+
+Booting from the image supplied via the QEMU command line requires
+that the file passed via -kernel has the same memory layout as would
+result from the disk boot. This memory layout includes the encrypted
+components (kernel, initrd, cmdline), the stage3a loader and
+metadata. In case this boot method is used, the command line
+options -initrd and -cmdline are ineffective. The preparation of a PVM
+image is done by genprotimg of the s390-tools package.
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v7 15/15] s390x: Add unpack facility feature to GA1
2020-03-09 11:21 [PATCH v7 00/15] s390x: Protected Virtualization support Janosch Frank
` (13 preceding siblings ...)
2020-03-09 11:22 ` [PATCH v7 14/15] docs: Add protvirt docs Janosch Frank
@ 2020-03-09 11:22 ` Janosch Frank
14 siblings, 0 replies; 45+ messages in thread
From: Janosch Frank @ 2020-03-09 11:22 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
From: Christian Borntraeger <borntraeger@de.ibm.com>
The unpack facility is an indication that diagnose 308 subcodes 8-10
are available to the guest. That means, that the guest can put itself
into protected mode.
Once it is in protected mode, the hardware stops any attempt of VM
introspection by the hypervisor.
Some features are currently not supported in protected mode:
* Passthrough devices
* Migration
* Huge page backings
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
target/s390x/gen-features.c | 1 +
target/s390x/kvm.c | 5 +++++
2 files changed, 6 insertions(+)
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 6278845b12..8ddeebc544 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -562,6 +562,7 @@ static uint16_t full_GEN15_GA1[] = {
S390_FEAT_GROUP_MSA_EXT_9,
S390_FEAT_GROUP_MSA_EXT_9_PCKMO,
S390_FEAT_ETOKEN,
+ S390_FEAT_UNPACK,
};
/* Default features (in order of release)
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 5c7757ac28..ade0752f0c 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2400,6 +2400,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
clear_bit(S390_FEAT_BPB, model->features);
}
+ /* we do have the IPL enhancements */
+ if (cap_protected) {
+ set_bit(S390_FEAT_UNPACK, model->features);
+ }
+
/* We emulate a zPCI bus and AEN, therefore we don't need HW support */
set_bit(S390_FEAT_ZPCI, model->features);
set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread