[1/2] KVM: s390: implement subfunction processor calls
diff mbox series

Message ID 20190221184304.42805-2-borntraeger@de.ibm.com
State New
Headers show
Series
  • KVM: s390: small prep patches for cpu model
Related show

Commit Message

Christian Borntraeger Feb. 21, 2019, 6:43 p.m. UTC
While we will not implement interception for query functions yet, we can
and should disable functions that have a control bit based on the given
CPU model.

Let us start with enabling the subfunction interface.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/include/asm/kvm_host.h |  1 +
 arch/s390/kvm/kvm-s390.c         | 34 +++++++++++++++++++++-----------
 2 files changed, 23 insertions(+), 12 deletions(-)

Comments

David Hildenbrand Feb. 21, 2019, 7:09 p.m. UTC | #1
On 21.02.19 19:43, Christian Borntraeger wrote:
> While we will not implement interception for query functions yet, we can
> and should disable functions that have a control bit based on the given
> CPU model.
> 
> Let us start with enabling the subfunction interface.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h |  1 +
>  arch/s390/kvm/kvm-s390.c         | 34 +++++++++++++++++++++-----------
>  2 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index d5d24889c3bcf..a042861554507 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -712,6 +712,7 @@ struct s390_io_adapter {
>  struct kvm_s390_cpu_model {
>  	/* facility mask supported by kvm & hosting machine */
>  	__u64 fac_mask[S390_ARCH_FAC_LIST_SIZE_U64];
> +	struct kvm_s390_vm_cpu_subfunc subfuncs;
>  	/* facility list requested by guest (in dma page) */
>  	__u64 *fac_list;
>  	u64 cpuid;
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 7f4bc58a53b97..5058fe58ece77 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -1258,11 +1258,20 @@ static int kvm_s390_set_processor_feat(struct kvm *kvm,
>  static int kvm_s390_set_processor_subfunc(struct kvm *kvm,
>  					  struct kvm_device_attr *attr)
>  {
> -	/*
> -	 * Once supported by kernel + hw, we have to store the subfunctions
> -	 * in kvm->arch and remember that user space configured them.
> -	 */
> -	return -ENXIO;
> +	mutex_lock(&kvm->lock);
> +	if (kvm->created_vcpus) {
> +		mutex_unlock(&kvm->lock);
> +		return -EBUSY;
> +	}
> +
> +	if (copy_from_user(&kvm->arch.model.subfuncs, (void __user *)attr->addr,
> +			   sizeof(struct kvm_s390_vm_cpu_subfunc))) {
> +		mutex_unlock(&kvm->lock);
> +		return -EFAULT;
> +	}

Wonder if we want to allow setting features the host does not have. I
guess we want to (similar to stfle).

> +	mutex_unlock(&kvm->lock);
> +
> +	return 0;
>  }
>  
>  static int kvm_s390_set_cpu_model(struct kvm *kvm, struct kvm_device_attr *attr)
> @@ -1381,12 +1390,11 @@ static int kvm_s390_get_machine_feat(struct kvm *kvm,
>  static int kvm_s390_get_processor_subfunc(struct kvm *kvm,
>  					  struct kvm_device_attr *attr)
>  {
> -	/*
> -	 * Once we can actually configure subfunctions (kernel + hw support),
> -	 * we have to check if they were already set by user space, if so copy
> -	 * them from kvm->arch.
> -	 */
> -	return -ENXIO;
> +	if (copy_to_user((void __user *)attr->addr, &kvm->arch.model.subfuncs,
> +	    sizeof(struct kvm_s390_vm_cpu_subfunc)))
> +		return -EFAULT;
> +
> +	return 0;
>  }
>  
>  static int kvm_s390_get_machine_subfunc(struct kvm *kvm,
> @@ -1397,6 +1405,7 @@ static int kvm_s390_get_machine_subfunc(struct kvm *kvm,
>  		return -EFAULT;
>  	return 0;
>  }
> +
>  static int kvm_s390_get_cpu_model(struct kvm *kvm, struct kvm_device_attr *attr)
>  {
>  	int ret = -ENXIO;
> @@ -1514,10 +1523,10 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>  		case KVM_S390_VM_CPU_PROCESSOR_FEAT:
>  		case KVM_S390_VM_CPU_MACHINE_FEAT:
>  		case KVM_S390_VM_CPU_MACHINE_SUBFUNC:
> +		case KVM_S390_VM_CPU_PROCESSOR_SUBFUNC:
>  			ret = 0;
>  			break;
>  		/* configuring subfunctions is not supported yet */

You want to drop that comment as well I think.


> -		case KVM_S390_VM_CPU_PROCESSOR_SUBFUNC:
>  		default:
>  			ret = -ENXIO;
>  			break;
> @@ -2218,6 +2227,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>  		kvm->arch.model.fac_list[i] = S390_lowcore.stfle_fac_list[i] &
>  					      kvm_s390_fac_base[i];
>  	}
> +	kvm->arch.model.subfuncs = kvm_s390_available_subfunc;
>  
>  	/* we are always in czam mode - even on pre z14 machines */
>  	set_kvm_facility(kvm->arch.model.fac_mask, 138);
> 

Reviewed-by: David Hildenbrand <david@redhat.com>
Christian Borntraeger Feb. 22, 2019, 7:54 a.m. UTC | #2
On 21.02.2019 20:09, David Hildenbrand wrote:
> On 21.02.19 19:43, Christian Borntraeger wrote:
>> While we will not implement interception for query functions yet, we can
>> and should disable functions that have a control bit based on the given
>> CPU model.
>>
>> Let us start with enabling the subfunction interface.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  arch/s390/include/asm/kvm_host.h |  1 +
>>  arch/s390/kvm/kvm-s390.c         | 34 +++++++++++++++++++++-----------
>>  2 files changed, 23 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index d5d24889c3bcf..a042861554507 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -712,6 +712,7 @@ struct s390_io_adapter {
>>  struct kvm_s390_cpu_model {
>>  	/* facility mask supported by kvm & hosting machine */
>>  	__u64 fac_mask[S390_ARCH_FAC_LIST_SIZE_U64];
>> +	struct kvm_s390_vm_cpu_subfunc subfuncs;
>>  	/* facility list requested by guest (in dma page) */
>>  	__u64 *fac_list;
>>  	u64 cpuid;
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 7f4bc58a53b97..5058fe58ece77 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -1258,11 +1258,20 @@ static int kvm_s390_set_processor_feat(struct kvm *kvm,
>>  static int kvm_s390_set_processor_subfunc(struct kvm *kvm,
>>  					  struct kvm_device_attr *attr)
>>  {
>> -	/*
>> -	 * Once supported by kernel + hw, we have to store the subfunctions
>> -	 * in kvm->arch and remember that user space configured them.
>> -	 */
>> -	return -ENXIO;
>> +	mutex_lock(&kvm->lock);
>> +	if (kvm->created_vcpus) {
>> +		mutex_unlock(&kvm->lock);
>> +		return -EBUSY;
>> +	}
>> +
>> +	if (copy_from_user(&kvm->arch.model.subfuncs, (void __user *)attr->addr,
>> +			   sizeof(struct kvm_s390_vm_cpu_subfunc))) {
>> +		mutex_unlock(&kvm->lock);
>> +		return -EFAULT;
>> +	}
> 
> Wonder if we want to allow setting features the host does not have. I
> guess we want to (similar to stfle).

Right now we allow this and I think this is ok.

> 
>> +	mutex_unlock(&kvm->lock);
>> +
>> +	return 0;
>>  }
>>  
>>  static int kvm_s390_set_cpu_model(struct kvm *kvm, struct kvm_device_attr *attr)
>> @@ -1381,12 +1390,11 @@ static int kvm_s390_get_machine_feat(struct kvm *kvm,
>>  static int kvm_s390_get_processor_subfunc(struct kvm *kvm,
>>  					  struct kvm_device_attr *attr)
>>  {
>> -	/*
>> -	 * Once we can actually configure subfunctions (kernel + hw support),
>> -	 * we have to check if they were already set by user space, if so copy
>> -	 * them from kvm->arch.
>> -	 */
>> -	return -ENXIO;
>> +	if (copy_to_user((void __user *)attr->addr, &kvm->arch.model.subfuncs,
>> +	    sizeof(struct kvm_s390_vm_cpu_subfunc)))
>> +		return -EFAULT;
>> +
>> +	return 0;
>>  }
>>  
>>  static int kvm_s390_get_machine_subfunc(struct kvm *kvm,
>> @@ -1397,6 +1405,7 @@ static int kvm_s390_get_machine_subfunc(struct kvm *kvm,
>>  		return -EFAULT;
>>  	return 0;
>>  }
>> +
>>  static int kvm_s390_get_cpu_model(struct kvm *kvm, struct kvm_device_attr *attr)
>>  {
>>  	int ret = -ENXIO;
>> @@ -1514,10 +1523,10 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>>  		case KVM_S390_VM_CPU_PROCESSOR_FEAT:
>>  		case KVM_S390_VM_CPU_MACHINE_FEAT:
>>  		case KVM_S390_VM_CPU_MACHINE_SUBFUNC:
>> +		case KVM_S390_VM_CPU_PROCESSOR_SUBFUNC:
>>  			ret = 0;
>>  			break;
>>  		/* configuring subfunctions is not supported yet */
> 
> You want to drop that comment as well I think.

yes, will drop.
> 
> 
>> -		case KVM_S390_VM_CPU_PROCESSOR_SUBFUNC:
>>  		default:
>>  			ret = -ENXIO;
>>  			break;
>> @@ -2218,6 +2227,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>>  		kvm->arch.model.fac_list[i] = S390_lowcore.stfle_fac_list[i] &
>>  					      kvm_s390_fac_base[i];
>>  	}
>> +	kvm->arch.model.subfuncs = kvm_s390_available_subfunc;
>>  
>>  	/* we are always in czam mode - even on pre z14 machines */
>>  	set_kvm_facility(kvm->arch.model.fac_mask, 138);
>>
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
>
Cornelia Huck Feb. 22, 2019, 8:48 a.m. UTC | #3
On Thu, 21 Feb 2019 19:43:03 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> While we will not implement interception for query functions yet, we can
> and should disable functions that have a control bit based on the given
> CPU model.
> 
> Let us start with enabling the subfunction interface.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h |  1 +
>  arch/s390/kvm/kvm-s390.c         | 34 +++++++++++++++++++++-----------
>  2 files changed, 23 insertions(+), 12 deletions(-)
> 

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 7f4bc58a53b97..5058fe58ece77 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -1258,11 +1258,20 @@ static int kvm_s390_set_processor_feat(struct kvm *kvm,
>  static int kvm_s390_set_processor_subfunc(struct kvm *kvm,
>  					  struct kvm_device_attr *attr)
>  {
> -	/*
> -	 * Once supported by kernel + hw, we have to store the subfunctions
> -	 * in kvm->arch and remember that user space configured them.
> -	 */
> -	return -ENXIO;
> +	mutex_lock(&kvm->lock);
> +	if (kvm->created_vcpus) {
> +		mutex_unlock(&kvm->lock);
> +		return -EBUSY;
> +	}
> +
> +	if (copy_from_user(&kvm->arch.model.subfuncs, (void __user *)attr->addr,
> +			   sizeof(struct kvm_s390_vm_cpu_subfunc))) {

Is there any need for sanitizing/checking the data provided by
userspace before sticking it into our internal structures? Not that we
do anything useful with it yet :)

> +		mutex_unlock(&kvm->lock);
> +		return -EFAULT;
> +	}
> +	mutex_unlock(&kvm->lock);
> +
> +	return 0;
>  }
Christian Borntraeger Feb. 22, 2019, 9:07 a.m. UTC | #4
On 22.02.2019 09:48, Cornelia Huck wrote:
> On Thu, 21 Feb 2019 19:43:03 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> While we will not implement interception for query functions yet, we can
>> and should disable functions that have a control bit based on the given
>> CPU model.
>>
>> Let us start with enabling the subfunction interface.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  arch/s390/include/asm/kvm_host.h |  1 +
>>  arch/s390/kvm/kvm-s390.c         | 34 +++++++++++++++++++++-----------
>>  2 files changed, 23 insertions(+), 12 deletions(-)
>>
> 
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 7f4bc58a53b97..5058fe58ece77 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -1258,11 +1258,20 @@ static int kvm_s390_set_processor_feat(struct kvm *kvm,
>>  static int kvm_s390_set_processor_subfunc(struct kvm *kvm,
>>  					  struct kvm_device_attr *attr)
>>  {
>> -	/*
>> -	 * Once supported by kernel + hw, we have to store the subfunctions
>> -	 * in kvm->arch and remember that user space configured them.
>> -	 */
>> -	return -ENXIO;
>> +	mutex_lock(&kvm->lock);
>> +	if (kvm->created_vcpus) {
>> +		mutex_unlock(&kvm->lock);
>> +		return -EBUSY;
>> +	}
>> +
>> +	if (copy_from_user(&kvm->arch.model.subfuncs, (void __user *)attr->addr,
>> +			   sizeof(struct kvm_s390_vm_cpu_subfunc))) {
> 
> Is there any need for sanitizing/checking the data provided by
> userspace before sticking it into our internal structures? Not that we
> do anything useful with it yet :)

No, its more like facility bits. Its the value that is returned by the query 
function of several instructions. If QEMU wants to lie then the guest might
misbehave but the host kernel is fine.

> 
>> +		mutex_unlock(&kvm->lock);
>> +		return -EFAULT;
>> +	}
>> +	mutex_unlock(&kvm->lock);
>> +
>> +	return 0;
>>  }
>
Cornelia Huck Feb. 22, 2019, 9:26 a.m. UTC | #5
On Fri, 22 Feb 2019 10:07:53 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 22.02.2019 09:48, Cornelia Huck wrote:
> > On Thu, 21 Feb 2019 19:43:03 +0100
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> While we will not implement interception for query functions yet, we can
> >> and should disable functions that have a control bit based on the given
> >> CPU model.
> >>
> >> Let us start with enabling the subfunction interface.
> >>
> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >> ---
> >>  arch/s390/include/asm/kvm_host.h |  1 +
> >>  arch/s390/kvm/kvm-s390.c         | 34 +++++++++++++++++++++-----------
> >>  2 files changed, 23 insertions(+), 12 deletions(-)
> >>  
> >   
> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> >> index 7f4bc58a53b97..5058fe58ece77 100644
> >> --- a/arch/s390/kvm/kvm-s390.c
> >> +++ b/arch/s390/kvm/kvm-s390.c
> >> @@ -1258,11 +1258,20 @@ static int kvm_s390_set_processor_feat(struct kvm *kvm,
> >>  static int kvm_s390_set_processor_subfunc(struct kvm *kvm,
> >>  					  struct kvm_device_attr *attr)
> >>  {
> >> -	/*
> >> -	 * Once supported by kernel + hw, we have to store the subfunctions
> >> -	 * in kvm->arch and remember that user space configured them.
> >> -	 */
> >> -	return -ENXIO;
> >> +	mutex_lock(&kvm->lock);
> >> +	if (kvm->created_vcpus) {
> >> +		mutex_unlock(&kvm->lock);
> >> +		return -EBUSY;
> >> +	}
> >> +
> >> +	if (copy_from_user(&kvm->arch.model.subfuncs, (void __user *)attr->addr,
> >> +			   sizeof(struct kvm_s390_vm_cpu_subfunc))) {  
> > 
> > Is there any need for sanitizing/checking the data provided by
> > userspace before sticking it into our internal structures? Not that we
> > do anything useful with it yet :)  
> 
> No, its more like facility bits. Its the value that is returned by the query 
> function of several instructions. If QEMU wants to lie then the guest might
> misbehave but the host kernel is fine.

Fair enough, if wonky data cannot affect the host negatively.

> 
> >   
> >> +		mutex_unlock(&kvm->lock);
> >> +		return -EFAULT;
> >> +	}
> >> +	mutex_unlock(&kvm->lock);
> >> +
> >> +	return 0;
> >>  }  
> >   
>
Cornelia Huck Feb. 22, 2019, 9:27 a.m. UTC | #6
On Thu, 21 Feb 2019 19:43:03 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> While we will not implement interception for query functions yet, we can
> and should disable functions that have a control bit based on the given
> CPU model.
> 
> Let us start with enabling the subfunction interface.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h |  1 +
>  arch/s390/kvm/kvm-s390.c         | 34 +++++++++++++++++++++-----------
>  2 files changed, 23 insertions(+), 12 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Janosch Frank Feb. 22, 2019, 9:36 a.m. UTC | #7
On 21.02.19 19:43, Christian Borntraeger wrote:
> While we will not implement interception for query functions yet, we can
> and should disable functions that have a control bit based on the given
> CPU model.
> 
> Let us start with enabling the subfunction interface.

Always wondered when we'll use these interfaces :-)
Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>

Patch
diff mbox series

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index d5d24889c3bcf..a042861554507 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -712,6 +712,7 @@  struct s390_io_adapter {
 struct kvm_s390_cpu_model {
 	/* facility mask supported by kvm & hosting machine */
 	__u64 fac_mask[S390_ARCH_FAC_LIST_SIZE_U64];
+	struct kvm_s390_vm_cpu_subfunc subfuncs;
 	/* facility list requested by guest (in dma page) */
 	__u64 *fac_list;
 	u64 cpuid;
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 7f4bc58a53b97..5058fe58ece77 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1258,11 +1258,20 @@  static int kvm_s390_set_processor_feat(struct kvm *kvm,
 static int kvm_s390_set_processor_subfunc(struct kvm *kvm,
 					  struct kvm_device_attr *attr)
 {
-	/*
-	 * Once supported by kernel + hw, we have to store the subfunctions
-	 * in kvm->arch and remember that user space configured them.
-	 */
-	return -ENXIO;
+	mutex_lock(&kvm->lock);
+	if (kvm->created_vcpus) {
+		mutex_unlock(&kvm->lock);
+		return -EBUSY;
+	}
+
+	if (copy_from_user(&kvm->arch.model.subfuncs, (void __user *)attr->addr,
+			   sizeof(struct kvm_s390_vm_cpu_subfunc))) {
+		mutex_unlock(&kvm->lock);
+		return -EFAULT;
+	}
+	mutex_unlock(&kvm->lock);
+
+	return 0;
 }
 
 static int kvm_s390_set_cpu_model(struct kvm *kvm, struct kvm_device_attr *attr)
@@ -1381,12 +1390,11 @@  static int kvm_s390_get_machine_feat(struct kvm *kvm,
 static int kvm_s390_get_processor_subfunc(struct kvm *kvm,
 					  struct kvm_device_attr *attr)
 {
-	/*
-	 * Once we can actually configure subfunctions (kernel + hw support),
-	 * we have to check if they were already set by user space, if so copy
-	 * them from kvm->arch.
-	 */
-	return -ENXIO;
+	if (copy_to_user((void __user *)attr->addr, &kvm->arch.model.subfuncs,
+	    sizeof(struct kvm_s390_vm_cpu_subfunc)))
+		return -EFAULT;
+
+	return 0;
 }
 
 static int kvm_s390_get_machine_subfunc(struct kvm *kvm,
@@ -1397,6 +1405,7 @@  static int kvm_s390_get_machine_subfunc(struct kvm *kvm,
 		return -EFAULT;
 	return 0;
 }
+
 static int kvm_s390_get_cpu_model(struct kvm *kvm, struct kvm_device_attr *attr)
 {
 	int ret = -ENXIO;
@@ -1514,10 +1523,10 @@  static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
 		case KVM_S390_VM_CPU_PROCESSOR_FEAT:
 		case KVM_S390_VM_CPU_MACHINE_FEAT:
 		case KVM_S390_VM_CPU_MACHINE_SUBFUNC:
+		case KVM_S390_VM_CPU_PROCESSOR_SUBFUNC:
 			ret = 0;
 			break;
 		/* configuring subfunctions is not supported yet */
-		case KVM_S390_VM_CPU_PROCESSOR_SUBFUNC:
 		default:
 			ret = -ENXIO;
 			break;
@@ -2218,6 +2227,7 @@  int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 		kvm->arch.model.fac_list[i] = S390_lowcore.stfle_fac_list[i] &
 					      kvm_s390_fac_base[i];
 	}
+	kvm->arch.model.subfuncs = kvm_s390_available_subfunc;
 
 	/* we are always in czam mode - even on pre z14 machines */
 	set_kvm_facility(kvm->arch.model.fac_mask, 138);