Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 28 Jul 2023 17:36:19 +0200
From:   Heiko Carstens <hca@linux.ibm.com>
To:     Steffen Eiden <seiden@linux.ibm.com>
Cc:     linux-kernel@vger.kernel.org, linux-s390@vger.kernel.org,
        kvm@vger.kernel.org, Janosch Frank <frankja@linux.ibm.com>,
        Claudio Imbrenda <imbrenda@linux.ibm.com>,
        David Hildenbrand <david@redhat.com>,
        Michael Mueller <mimu@linux.vnet.ibm.com>,
        Marc Hartmayer <mhartmay@linux.ibm.com>
Subject: Re: [PATCH v2 2/3] KVM: s390: Add UV feature negotiation
Message-ID: <20230728153619.13110-B-hca@linux.ibm.com>
References: <20230728092341.1131787-1-seiden@linux.ibm.com>
 <20230728092341.1131787-3-seiden@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230728092341.1131787-3-seiden@linux.ibm.com>
Precedence: bulk

On Fri, Jul 28, 2023 at 11:23:40AM +0200, Steffen Eiden wrote:
> Add a uv_feature list for pv-guests to the kvm cpu-model.
> The feature bits 'AP-interpretation for secure guests' and
> 'AP-interrupt for secure guests' are available.
> 
> Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h |  2 ++
>  arch/s390/include/uapi/asm/kvm.h | 25 +++++++++++++
>  arch/s390/kvm/kvm-s390.c         | 60 ++++++++++++++++++++++++++++++++
>  3 files changed, 87 insertions(+)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 2bbc3d54959d..7ae57ac352b2 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -817,6 +817,8 @@ struct kvm_s390_cpu_model {
>  	__u64 *fac_list;
>  	u64 cpuid;
>  	unsigned short ibc;
> +	/* subset of available uv features for pv-guests enabled by user space */
> +	struct kvm_s390_vm_cpu_uv_feat uv_feat_guest;
>  };
>  
>  typedef int (*crypto_hook)(struct kvm_vcpu *vcpu);
> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
> index a73cf01a1606..b7f4bf5687c7 100644
> --- a/arch/s390/include/uapi/asm/kvm.h
> +++ b/arch/s390/include/uapi/asm/kvm.h
> @@ -159,6 +159,31 @@ struct kvm_s390_vm_cpu_subfunc {
>  	__u8 reserved[1728];
>  };
>  
> +#define KVM_S390_VM_CPU_PROCESSOR_UV_FEAT_GUEST	6
> +#define KVM_S390_VM_CPU_MACHINE_UV_FEAT_GUEST	7
> +
> +#define KVM_S390_VM_CPU_UV_FEAT_NR_BITS	64
> +struct kvm_s390_vm_cpu_uv_feat {
> +	union {
> +		struct {
> +			__u64 res0 : 4;
> +			__u64 ap : 1;		/* bit 4 */
> +			__u64 ap_intr : 1;	/* bit 5 */
> +			__u64 res6 : 58;

Using unnamed bitfields makes life easier.

> +#define KVM_S390_VM_CPU_UV_FEAT_GUEST_MASK  \
> +	(((struct kvm_s390_vm_cpu_uv_feat){ \
> +		  .ap = 1,                  \
> +		  .ap_intr = 1,             \
> +	  })                                \
> +		 .feat)
> +
> +#define KVM_S390_VM_CPU_UV_FEAT_GUEST_DEFAULT ((struct kvm_s390_vm_cpu_uv_feat){})

Why are these two define uapi? Looks to me like both should be kernel
internal. Or why should user space know about them?

Also I think KVM_S390_VM_CPU_UV_FEAT_GUEST_DEFAULT is really odd
compared to all other code we have. Defining it to just 0 and assiging
to .feat instead of assigning a structure is the "usual way".

> +static int kvm_s390_set_uv_feat(struct kvm *kvm, struct kvm_device_attr *attr)
> +{
> +	struct kvm_s390_vm_cpu_uv_feat data = {};
> +	unsigned long filter = uv_info.uv_feature_indications & KVM_S390_VM_CPU_UV_FEAT_GUEST_MASK;
> +
> +	if (copy_from_user(&data, (void __user *)attr->addr, sizeof(data)))
> +		return -EFAULT;
> +	if (!bitmap_subset((unsigned long *)&data, &filter, KVM_S390_VM_CPU_UV_FEAT_NR_BITS))
> +		return -EINVAL;

Casting a structure to an unsinged long pointer is also quite odd. I'd
would use get_user() & friends, and avoid all the casts. Patch below is
something I would do, also in order to reduce line lengths and breaks at
some places.. Feel free to ignore the whole patch, or take parts or all of
it (patch is on top of your complete series, and of course untested).

diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
index b7f4bf5687c7..c1b84c1a297d 100644
--- a/arch/s390/include/uapi/asm/kvm.h
+++ b/arch/s390/include/uapi/asm/kvm.h
@@ -166,23 +166,25 @@ struct kvm_s390_vm_cpu_subfunc {
 struct kvm_s390_vm_cpu_uv_feat {
 	union {
 		struct {
-			__u64 res0 : 4;
+			__u64 : 4;
 			__u64 ap : 1;		/* bit 4 */
 			__u64 ap_intr : 1;	/* bit 5 */
-			__u64 res6 : 58;
+			__u64 : 58;
 		};
 		__u64 feat;
 	};
 };
 
-#define KVM_S390_VM_CPU_UV_FEAT_GUEST_MASK  \
-	(((struct kvm_s390_vm_cpu_uv_feat){ \
-		  .ap = 1,                  \
-		  .ap_intr = 1,             \
-	  })                                \
-		 .feat)
+#define KVM_S390_VM_CPU_UV_FEAT_GUEST_MASK	\
+(						\
+	((struct kvm_s390_vm_cpu_uv_feat){	\
+		.ap = 1,			\
+		.ap_intr = 1,			\
+	})					\
+	.feat					\
+)
 
-#define KVM_S390_VM_CPU_UV_FEAT_GUEST_DEFAULT ((struct kvm_s390_vm_cpu_uv_feat){})
+#define KVM_S390_VM_CPU_UV_FEAT_GUEST_DEFAULT	0
 
 /* kvm attributes for crypto */
 #define KVM_S390_VM_CRYPTO_ENABLE_AES_KW	0
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index f383c48f965a..bbf4de3e27b3 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1533,12 +1533,13 @@ static int kvm_s390_set_processor_subfunc(struct kvm *kvm,
 
 static int kvm_s390_set_uv_feat(struct kvm *kvm, struct kvm_device_attr *attr)
 {
-	struct kvm_s390_vm_cpu_uv_feat data = {};
-	unsigned long filter = uv_info.uv_feature_indications & KVM_S390_VM_CPU_UV_FEAT_GUEST_MASK;
+	struct kvm_s390_vm_cpu_uv_feat __user *ptr = (void __user *)attr->addr;
+	unsigned long data, filter;
 
-	if (copy_from_user(&data, (void __user *)attr->addr, sizeof(data)))
+	filter = uv_info.uv_feature_indications & KVM_S390_VM_CPU_UV_FEAT_GUEST_MASK;
+	if (get_user(data, &ptr->feat))
 		return -EFAULT;
-	if (!bitmap_subset((unsigned long *)&data, &filter, KVM_S390_VM_CPU_UV_FEAT_NR_BITS))
+	if (!bitmap_subset(&data, &filter, KVM_S390_VM_CPU_UV_FEAT_NR_BITS))
 		return -EINVAL;
 
 	mutex_lock(&kvm->lock);
@@ -1546,10 +1547,10 @@ static int kvm_s390_set_uv_feat(struct kvm *kvm, struct kvm_device_attr *attr)
 		mutex_unlock(&kvm->lock);
 		return -EBUSY;
 	}
-	kvm->arch.model.uv_feat_guest = data;
+	kvm->arch.model.uv_feat_guest.feat = data;
 	mutex_unlock(&kvm->lock);
 
-	VM_EVENT(kvm, 3, "SET: guest uv feat: 0x%16.16llx", data.feat);
+	VM_EVENT(kvm, 3, "SET: guest uv feat: 0x%16.16lx", data);
 
 	return 0;
 }
@@ -1805,24 +1806,27 @@ static int kvm_s390_get_machine_subfunc(struct kvm *kvm,
 
 static int kvm_s390_get_processor_uv_feat(struct kvm *kvm, struct kvm_device_attr *attr)
 {
-	struct kvm_s390_vm_cpu_uv_feat *src = &kvm->arch.model.uv_feat_guest;
+	struct kvm_s390_vm_cpu_uv_feat __user *dst = (void __user *)attr->addr;
+	unsigned long feat = kvm->arch.model.uv_feat_guest.feat;
 
-	if (copy_to_user((void __user *)attr->addr, src, sizeof(*src)))
+	if (put_user(feat, &dst->feat))
 		return -EFAULT;
-	VM_EVENT(kvm, 3, "GET: guest  uv feat: 0x%16.16llx", kvm->arch.model.uv_feat_guest.feat);
+	VM_EVENT(kvm, 3, "GET: guest  uv feat: 0x%16.16lx", feat);
 
 	return 0;
 }
 
 static int kvm_s390_get_machine_uv_feat(struct kvm *kvm, struct kvm_device_attr *attr)
 {
-	struct kvm_s390_vm_cpu_uv_feat data = { .feat = uv_info.uv_feature_indications &
-							KVM_S390_VM_CPU_UV_FEAT_GUEST_MASK };
+	struct kvm_s390_vm_cpu_uv_feat __user *dst = (void __user *)attr->addr;
+	unsigned long feat;
 
-	BUILD_BUG_ON(sizeof(data) != sizeof(uv_info.uv_feature_indications));
-	if (copy_to_user((void __user *)attr->addr, &data, sizeof(struct kvm_s390_vm_cpu_uv_feat)))
+	BUILD_BUG_ON(sizeof(*dst) != sizeof(uv_info.uv_feature_indications));
+
+	feat = uv_info.uv_feature_indications & KVM_S390_VM_CPU_UV_FEAT_GUEST_MASK;
+	if (put_user(feat, &dst->feat))
 		return -EFAULT;
-	VM_EVENT(kvm, 3, "GET: guest  uv feat: 0x%16.16llx", data.feat);
+	VM_EVENT(kvm, 3, "GET: guest  uv feat: 0x%16.16lx", feat);
 
 	return 0;
 }
@@ -3354,7 +3358,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	kvm->arch.model.cpuid = kvm_s390_get_initial_cpuid();
 	kvm->arch.model.ibc = sclp.ibc & 0x0fff;
 
-	kvm->arch.model.uv_feat_guest = KVM_S390_VM_CPU_UV_FEAT_GUEST_DEFAULT;
+	kvm->arch.model.uv_feat_guest.feat = KVM_S390_VM_CPU_UV_FEAT_GUEST_DEFAULT;
 
 	kvm_s390_crypto_init(kvm);