On 24/10/2019 13.40, Janosch Frank wrote:
Let's add a KVM interface to create and destroy protected VMs.
I agree with David, some more description here would be helpful.
Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
---
arch/s390/include/asm/kvm_host.h | 24 +++-
arch/s390/include/asm/uv.h | 110 ++++++++++++++
arch/s390/kvm/Makefile | 2 +-
arch/s390/kvm/kvm-s390.c | 173 +++++++++++++++++++++-
arch/s390/kvm/kvm-s390.h | 47 ++++++
arch/s390/kvm/pv.c | 237 +++++++++++++++++++++++++++++++
include/uapi/linux/kvm.h | 33 +++++
7 files changed, 622 insertions(+), 4 deletions(-)
create mode 100644 arch/s390/kvm/pv.c
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 02f4c21c57f6..d4fd0f3af676 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -155,7 +155,13 @@ struct kvm_s390_sie_block {
__u8 reserved08[4]; /* 0x0008 */
#define PROG_IN_SIE (1<<0)
__u32 prog0c; /* 0x000c */
- __u8 reserved10[16]; /* 0x0010 */
+ union {
+ __u8 reserved10[16]; /* 0x0010 */
+ struct {
+ __u64 pv_handle_cpu;
+ __u64 pv_handle_config;
+ };
+ };
Why do you need to keep reserved10[] here? Simply replace it with the
two new fields, and get rid of the union?
+/*
+ * Generic cmd executor for calls that only transport the cpu or guest
+ * handle and the command.
+ */
+static inline int uv_cmd_nodata(u64 handle, u16 cmd, u32 *ret)
+{
+ int rc;
+ struct uv_cb_nodata uvcb = {
+ .header.cmd = cmd,
+ .header.len = sizeof(uvcb),
+ .handle = handle,
+ };
+
+ WARN(!handle, "No handle provided to Ultravisor call cmd %x\n", cmd);
If this is not supposed to happen, I thing you should return here
instead of doing the uv_call() below?
Or maybe even turn this into a BUG() statement?
+ rc = uv_call(0, (u64)&uvcb);
+ if (ret)
+ *ret = *(u32 *)&uvcb.header.rc;
+ return rc ? -EINVAL : 0;
+}
[...]
@@ -2157,6 +2164,96 @@ static int kvm_s390_set_cmma_bits(struct kvm *kvm,
return r;
}
+#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
+static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
+{
+ int r = 0;
+ void __user *argp = (void __user *)cmd->data;
+
+ switch (cmd->cmd) {
Why are you using curly braces for the case statements below? They do
not seem to be necessary in most cases?
+ case KVM_PV_VM_CREATE: {
+ r = kvm_s390_pv_alloc_vm(kvm);
+ if (r)
+ break;
+
+ mutex_lock(&kvm->lock);
+ kvm_s390_vcpu_block_all(kvm);
+ /* FMT 4 SIE needs esca */
+ r = sca_switch_to_extended(kvm);
+ if (!r)
+ r = kvm_s390_pv_create_vm(kvm);
+ kvm_s390_vcpu_unblock_all(kvm);
+ mutex_unlock(&kvm->lock);
+ break;
+ }
+ case KVM_PV_VM_DESTROY: {
+ /* All VCPUs have to be destroyed before this call. */
+ mutex_lock(&kvm->lock);
+ kvm_s390_vcpu_block_all(kvm);
+ r = kvm_s390_pv_destroy_vm(kvm);
+ if (!r)
+ kvm_s390_pv_dealloc_vm(kvm);
+ kvm_s390_vcpu_unblock_all(kvm);
+ mutex_unlock(&kvm->lock);
+ break;
+ }
+ case KVM_PV_VM_SET_SEC_PARMS: {
+ struct kvm_s390_pv_sec_parm parms = {};
+ void *hdr;
+
+ r = -EFAULT;
+ if (copy_from_user(&parms, argp, sizeof(parms)))
+ break;
+
+ /* Currently restricted to 8KB */
+ r = -EINVAL;
+ if (parms.length > PAGE_SIZE * 2)
+ break;
I think you should also check fr parms.length == 0 ... otherwise you'll
get an unfriendly complaint from vmalloc().
+ r = -ENOMEM;
+ hdr = vmalloc(parms.length);
+ if (!hdr)
+ break;
+
+ r = -EFAULT;
+ if (!copy_from_user(hdr, (void __user *)parms.origin,
+ parms.length))
+ r = kvm_s390_pv_set_sec_parms(kvm, hdr, parms.length);
+
+ vfree(hdr);
+ break;
+ }
+ case KVM_PV_VM_UNPACK: {
+ struct kvm_s390_pv_unp unp = {};
+
+ r = -EFAULT;
+ if (copy_from_user(&unp, argp, sizeof(unp)))
+ break;
+
+ r = kvm_s390_pv_unpack(kvm, unp.addr, unp.size, unp.tweak);
+ break;
+ }
+ case KVM_PV_VM_VERIFY: {
+ u32 ret;
+
+ r = -EINVAL;
+ if (!kvm_s390_pv_is_protected(kvm))
+ break;
+
+ r = uv_cmd_nodata(kvm_s390_pv_handle(kvm),
+ UVC_CMD_VERIFY_IMG,
+ &ret);
+ VM_EVENT(kvm, 3, "PROTVIRT VERIFY: rc %x rrc %x",
+ ret >> 16, ret & 0x0000ffff);
+ break;
+ }
+ default:
+ return -ENOTTY;
Is ENOTTY the right thing to return for an invalid cmd here? It might
get confused with the ioctl not being available at all? Maybe EINVAL
would be better?
+ }
+ return r;
+}
+#endif
+
[...]
@@ -4338,6 +4471,28 @@ long kvm_arch_vcpu_async_ioctl(struct file *filp,
return -ENOIOCTLCMD;
}
+#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
+static int kvm_s390_handle_pv_vcpu(struct kvm_vcpu *vcpu,
+ struct kvm_pv_cmd *cmd)
+{
+ int r = 0;
+
+ switch (cmd->cmd) {
Also no need for the curly braces of the case statements here?
+ case KVM_PV_VCPU_CREATE: {
+ r = kvm_s390_pv_create_cpu(vcpu);
+ break;
+ }
+ case KVM_PV_VCPU_DESTROY: {
+ r = kvm_s390_pv_destroy_cpu(vcpu);
+ break;
+ }
+ default:
+ r = -ENOTTY;
Or EINVAL?
+ }
+ return r;
+}
+#endif
Thomas
