Re: [PATCH v2 3/9] KVM: s390: pv: Add query interface

Janosch Frank <frankja@xxxxxxxxxxxxx> · Mon, 14 Mar 2022 11:34:38 +0100

On 3/14/22 11:17, Claudio Imbrenda wrote:
On Mon, 14 Mar 2022 11:02:40 +0100
Janosch Frank <frankja@xxxxxxxxxxxxx> wrote:

On 3/11/22 18:40, Claudio Imbrenda wrote:
On Thu, 10 Mar 2022 10:31:06 +0000
Janosch Frank <frankja@xxxxxxxxxxxxx> wrote:
   
Some of the query information is already available via sysfs but
having a IOCTL makes the information easier to retrieve.

if I understand correctly, this will be forward-compatible but not
backwards compatible.

you return the amount of bytes written into the buffer, but only if the
buffer was already big enough.

a newer userspace will work with an older kernel, but an older
userspace will not work with a newer kernel.

I expect the first version of userspace to request a minimum length
hence I return -EINVAL if less space is given.

this makes sense


In the future the minimum will be a constant and we'll write between the
min and the new data length.

fair enough, but this means that future patches must be careful and not
forget this.

Sure, I'll find a way to make this clearer.


it's probably easier to read, and more future proof, if you put a check
for the minimum size (and perhaps also a comment to explain why), and
not use sizeof(struct ...)


IMHO there's no sense in allowing to request less data than the v1 of
the interface will provide.

of course




a solution would be to return the size of the struct, so userspace can
know how much of the buffer was written (if it was bigger than the
struct), or that there are unwritten bits (if the buffer was smaller).

and even if the buffer was too small, write back as much of it as
possible to userspace.

this way, an older userspace will get the information it expects.


I am also not a big fan of writing the size in the input struct (I think
returning it would be cleaner), but I do not have a strong opinion
   

Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
---
   arch/s390/kvm/kvm-s390.c | 76 ++++++++++++++++++++++++++++++++++++++++
   include/uapi/linux/kvm.h | 25 +++++++++++++
   2 files changed, 101 insertions(+)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 020356653d1a..67e1e445681f 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2224,6 +2224,42 @@ static int kvm_s390_cpus_to_pv(struct kvm *kvm, u16 *rc, u16 *rrc)
   	return r;
   }
   
+/*
+ * Here we provide user space with a direct interface to query UV
+ * related data like UV maxima and available features as well as
+ * feature specific data.
+ *
+ * To facilitate future extension of the data structures we'll try to
+ * write data up to the maximum requested length.
+ */
+static ssize_t kvm_s390_handle_pv_info(struct kvm_s390_pv_info *info)
+{
+	ssize_t len;
+
+	switch (info->header.id) {
+	case KVM_PV_INFO_VM: {
+		len =  sizeof(info->header) + sizeof(info->vm);
+
+		if (info->header.len_max < len)
+			return -EINVAL;
+
+		memcpy(info->vm.inst_calls_list,
+		       uv_info.inst_calls_list,
+		       sizeof(uv_info.inst_calls_list));
+
+		/* It's max cpuidm not max cpus so it's off by one */
+		info->vm.max_cpus = uv_info.max_guest_cpu_id + 1;
+		info->vm.max_guests = uv_info.max_num_sec_conf;
+		info->vm.max_guest_addr = uv_info.max_sec_stor_addr;
+		info->vm.feature_indication = uv_info.uv_feature_indications;
+
+		return len;
+	}
+	default:
+		return -EINVAL;
+	}
+}
+
   static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
   {
   	int r = 0;
@@ -2360,6 +2396,46 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
   			     cmd->rc, cmd->rrc);
   		break;
   	}
+	case KVM_PV_INFO: {
+		struct kvm_s390_pv_info info = {};
+		ssize_t data_len;
+
+		/*
+		 * No need to check the VM protection here.
+		 *
+		 * Maybe user space wants to query some of the data
+		 * when the VM is still unprotected. If we see the
+		 * need to fence a new data command we can still
+		 * return an error in the info handler.
+		 */
+
+		r = -EFAULT;
+		if (copy_from_user(&info, argp, sizeof(info.header)))
+			break;
+
+		r = -EINVAL;
+		if (info.header.len_max < sizeof(info.header))
+			break;
+
+		data_len = kvm_s390_handle_pv_info(&info);
+		if (data_len < 0) {
+			r = data_len;
+			break;
+		}
+		/*
+		 * If a data command struct is extended (multiple
+		 * times) this can be used to determine how much of it
+		 * is valid.
+		 */
+		info.header.len_written = data_len;
+
+		r = -EFAULT;
+		if (copy_to_user(argp, &info, data_len))
+			break;
+
+		r = 0;
+		break;
+	}
   	default:
   		r = -ENOTTY;
   	}
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index a02bbf8fd0f6..21f19863c417 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1643,6 +1643,30 @@ struct kvm_s390_pv_unp {
   	__u64 tweak;
   };
   
+enum pv_cmd_info_id {
+	KVM_PV_INFO_VM,
+};
+
+struct kvm_s390_pv_info_vm {
+	__u64 inst_calls_list[4];
+	__u64 max_cpus;
+	__u64 max_guests;
+	__u64 max_guest_addr;
+	__u64 feature_indication;
+};
+
+struct kvm_s390_pv_info_header {
+	__u32 id;
+	__u32 len_max;
+	__u32 len_written;
+	__u32 reserved;
+};
+
+struct kvm_s390_pv_info {
+	struct kvm_s390_pv_info_header header;
+	struct kvm_s390_pv_info_vm vm;
+};
+
   enum pv_cmd_id {
   	KVM_PV_ENABLE,
   	KVM_PV_DISABLE,
@@ -1651,6 +1675,7 @@ enum pv_cmd_id {
   	KVM_PV_VERIFY,
   	KVM_PV_PREP_RESET,
   	KVM_PV_UNSHARE_ALL,
+	KVM_PV_INFO,
   };
   
   struct kvm_pv_cmd {
   









