LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] KVM: s390: reset crypto attributes for all vcpus
@ 2018-04-19 21:13 Tony Krowiak
  2018-04-20  8:57 ` Cornelia Huck
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Tony Krowiak @ 2018-04-19 21:13 UTC (permalink / raw)
  To: linux-s390, linux-kernel, kvm
  Cc: borntraeger, cohuck, pmorel, pasic, akrowiak, pbonzini, rkrcmar

Introduces a new function to reset the crypto attributes for all
vcpus whether they are running or not. Each vcpu in KVM will
be removed from SIE prior to resetting the crypto attributes in its
SIE state description. After all vcpus have had their crypto attributes
reset the vcpus will be restored to SIE.

This function is incorporated into the kvm_s390_vm_set_crypto(kvm)
function to fix a reported issue whereby the crypto key wrapping
attributes could potentially get out of synch for running vcpus.

Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
---
 arch/s390/kvm/kvm-s390.c |   18 ++++++++++++++----
 arch/s390/kvm/kvm-s390.h |   13 +++++++++++++
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index fa355a6..4fa3037 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -789,6 +789,19 @@ static int kvm_s390_set_mem_control(struct kvm *kvm, struct kvm_device_attr *att
 	return ret;
 }
 
+void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm)
+ {
+	int i;
+	struct kvm_vcpu *vcpu;
+
+	kvm_s390_vcpu_block_all(kvm);
+
+	kvm_for_each_vcpu(i, vcpu, kvm)
+	        kvm_s390_vcpu_crypto_setup(vcpu);
+
+	kvm_s390_vcpu_unblock_all(kvm);
+}
+
 static void kvm_s390_vcpu_crypto_setup(struct kvm_vcpu *vcpu);
 
 static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
@@ -832,10 +845,7 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
 		return -ENXIO;
 	}
 
-	kvm_for_each_vcpu(i, vcpu, kvm) {
-		kvm_s390_vcpu_crypto_setup(vcpu);
-		exit_sie(vcpu);
-	}
+	kvm_s390_vcpu_crypto_reset_all(kvm);
 	mutex_unlock(&kvm->lock);
 	return 0;
 }
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index 1b5621f..981e3ba 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -410,4 +410,17 @@ static inline int kvm_s390_use_sca_entries(void)
 }
 void kvm_s390_reinject_machine_check(struct kvm_vcpu *vcpu,
 				     struct mcck_volatile_info *mcck_info);
+
+/**
+ * kvm_s390_vcpu_crypto_reset_all
+ *
+ * Reset the crypto attributes for each vcpu. This can be done while the vcpus
+ * are running as each vcpu will be removed from SIE before resetting the crypt
+ * attributes and restored to SIE afterward.
+ *
+ * Note: The kvm->lock must be held while calling this function
+ *
+ * @kvm: the KVM guest
+ */
+void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm);
 #endif
-- 
1.7.1

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus
  2018-04-19 21:13 [PATCH] KVM: s390: reset crypto attributes for all vcpus Tony Krowiak
@ 2018-04-20  8:57 ` Cornelia Huck
  2018-04-20 11:59 ` Janosch Frank
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: Cornelia Huck @ 2018-04-20  8:57 UTC (permalink / raw)
  To: Tony Krowiak
  Cc: linux-s390, linux-kernel, kvm, borntraeger, pmorel, pasic,
	pbonzini, rkrcmar

On Thu, 19 Apr 2018 17:13:52 -0400
Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote:

> Introduces a new function to reset the crypto attributes for all
> vcpus whether they are running or not. Each vcpu in KVM will
> be removed from SIE prior to resetting the crypto attributes in its
> SIE state description. After all vcpus have had their crypto attributes
> reset the vcpus will be restored to SIE.
> 
> This function is incorporated into the kvm_s390_vm_set_crypto(kvm)
> function to fix a reported issue whereby the crypto key wrapping
> attributes could potentially get out of synch for running vcpus.
> 
> Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
> ---
>  arch/s390/kvm/kvm-s390.c |   18 ++++++++++++++----
>  arch/s390/kvm/kvm-s390.h |   13 +++++++++++++
>  2 files changed, 27 insertions(+), 4 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus
  2018-04-19 21:13 [PATCH] KVM: s390: reset crypto attributes for all vcpus Tony Krowiak
  2018-04-20  8:57 ` Cornelia Huck
@ 2018-04-20 11:59 ` Janosch Frank
  2018-04-20 12:15   ` Janosch Frank
  2018-04-20 12:26 ` David Hildenbrand
  2018-04-21  0:11 ` kbuild test robot
  3 siblings, 1 reply; 11+ messages in thread
From: Janosch Frank @ 2018-04-20 11:59 UTC (permalink / raw)
  To: Tony Krowiak, linux-s390, linux-kernel, kvm
  Cc: borntraeger, cohuck, pmorel, pasic, pbonzini, rkrcmar


[-- Attachment #1.1: Type: text/plain, Size: 2800 bytes --]

Thanks, applied.


On 19.04.2018 23:13, Tony Krowiak wrote:
> Introduces a new function to reset the crypto attributes for all
> vcpus whether they are running or not. Each vcpu in KVM will
> be removed from SIE prior to resetting the crypto attributes in its
> SIE state description. After all vcpus have had their crypto attributes
> reset the vcpus will be restored to SIE.
> 
> This function is incorporated into the kvm_s390_vm_set_crypto(kvm)
> function to fix a reported issue whereby the crypto key wrapping
> attributes could potentially get out of synch for running vcpus.
> 
> Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
> ---
>  arch/s390/kvm/kvm-s390.c |   18 ++++++++++++++----
>  arch/s390/kvm/kvm-s390.h |   13 +++++++++++++
>  2 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index fa355a6..4fa3037 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -789,6 +789,19 @@ static int kvm_s390_set_mem_control(struct kvm *kvm, struct kvm_device_attr *att
>  	return ret;
>  }
> 
> +void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm)
> + {
> +	int i;
> +	struct kvm_vcpu *vcpu;
> +
> +	kvm_s390_vcpu_block_all(kvm);
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm)
> +	        kvm_s390_vcpu_crypto_setup(vcpu);
> +
> +	kvm_s390_vcpu_unblock_all(kvm);
> +}
> +
>  static void kvm_s390_vcpu_crypto_setup(struct kvm_vcpu *vcpu);
> 
>  static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
> @@ -832,10 +845,7 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>  		return -ENXIO;
>  	}
> 
> -	kvm_for_each_vcpu(i, vcpu, kvm) {
> -		kvm_s390_vcpu_crypto_setup(vcpu);
> -		exit_sie(vcpu);
> -	}
> +	kvm_s390_vcpu_crypto_reset_all(kvm);
>  	mutex_unlock(&kvm->lock);
>  	return 0;
>  }
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index 1b5621f..981e3ba 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -410,4 +410,17 @@ static inline int kvm_s390_use_sca_entries(void)
>  }
>  void kvm_s390_reinject_machine_check(struct kvm_vcpu *vcpu,
>  				     struct mcck_volatile_info *mcck_info);
> +
> +/**
> + * kvm_s390_vcpu_crypto_reset_all
> + *
> + * Reset the crypto attributes for each vcpu. This can be done while the vcpus
> + * are running as each vcpu will be removed from SIE before resetting the crypt
> + * attributes and restored to SIE afterward.
> + *
> + * Note: The kvm->lock must be held while calling this function
> + *
> + * @kvm: the KVM guest
> + */
> +void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm);
>  #endif
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus
  2018-04-20 11:59 ` Janosch Frank
@ 2018-04-20 12:15   ` Janosch Frank
  2018-04-22 15:10     ` Tony Krowiak
  0 siblings, 1 reply; 11+ messages in thread
From: Janosch Frank @ 2018-04-20 12:15 UTC (permalink / raw)
  To: Tony Krowiak, linux-s390, linux-kernel, kvm
  Cc: borntraeger, cohuck, pmorel, pasic, pbonzini, rkrcmar


[-- Attachment #1.1: Type: text/plain, Size: 3060 bytes --]

On 20.04.2018 13:59, Janosch Frank wrote:
> Thanks, applied.
> 

Well this does not compile, as you use kvm_s390_vcpu_crypto_setup before
declaration. Please fix, then I'll take the patch.


> 
> On 19.04.2018 23:13, Tony Krowiak wrote:
>> Introduces a new function to reset the crypto attributes for all
>> vcpus whether they are running or not. Each vcpu in KVM will
>> be removed from SIE prior to resetting the crypto attributes in its
>> SIE state description. After all vcpus have had their crypto attributes
>> reset the vcpus will be restored to SIE.
>>
>> This function is incorporated into the kvm_s390_vm_set_crypto(kvm)
>> function to fix a reported issue whereby the crypto key wrapping
>> attributes could potentially get out of synch for running vcpus.
>>
>> Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
>> ---
>>  arch/s390/kvm/kvm-s390.c |   18 ++++++++++++++----
>>  arch/s390/kvm/kvm-s390.h |   13 +++++++++++++
>>  2 files changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index fa355a6..4fa3037 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -789,6 +789,19 @@ static int kvm_s390_set_mem_control(struct kvm *kvm, struct kvm_device_attr *att
>>  	return ret;
>>  }
>>
>> +void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm)
>> + {
>> +	int i;
>> +	struct kvm_vcpu *vcpu;
>> +
>> +	kvm_s390_vcpu_block_all(kvm);
>> +
>> +	kvm_for_each_vcpu(i, vcpu, kvm)
>> +	        kvm_s390_vcpu_crypto_setup(vcpu);
>> +
>> +	kvm_s390_vcpu_unblock_all(kvm);
>> +}
>> +
>>  static void kvm_s390_vcpu_crypto_setup(struct kvm_vcpu *vcpu);
>>
>>  static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>> @@ -832,10 +845,7 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>>  		return -ENXIO;
>>  	}
>>
>> -	kvm_for_each_vcpu(i, vcpu, kvm) {
>> -		kvm_s390_vcpu_crypto_setup(vcpu);
>> -		exit_sie(vcpu);
>> -	}
>> +	kvm_s390_vcpu_crypto_reset_all(kvm);
>>  	mutex_unlock(&kvm->lock);
>>  	return 0;
>>  }
>> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
>> index 1b5621f..981e3ba 100644
>> --- a/arch/s390/kvm/kvm-s390.h
>> +++ b/arch/s390/kvm/kvm-s390.h
>> @@ -410,4 +410,17 @@ static inline int kvm_s390_use_sca_entries(void)
>>  }
>>  void kvm_s390_reinject_machine_check(struct kvm_vcpu *vcpu,
>>  				     struct mcck_volatile_info *mcck_info);
>> +
>> +/**
>> + * kvm_s390_vcpu_crypto_reset_all
>> + *
>> + * Reset the crypto attributes for each vcpu. This can be done while the vcpus
>> + * are running as each vcpu will be removed from SIE before resetting the crypt
>> + * attributes and restored to SIE afterward.
>> + *
>> + * Note: The kvm->lock must be held while calling this function
>> + *
>> + * @kvm: the KVM guest
>> + */
>> +void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm);
>>  #endif
>>
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus
  2018-04-19 21:13 [PATCH] KVM: s390: reset crypto attributes for all vcpus Tony Krowiak
  2018-04-20  8:57 ` Cornelia Huck
  2018-04-20 11:59 ` Janosch Frank
@ 2018-04-20 12:26 ` David Hildenbrand
  2018-04-20 12:28   ` David Hildenbrand
  2018-04-22 15:06   ` Tony Krowiak
  2018-04-21  0:11 ` kbuild test robot
  3 siblings, 2 replies; 11+ messages in thread
From: David Hildenbrand @ 2018-04-20 12:26 UTC (permalink / raw)
  To: Tony Krowiak, linux-s390, linux-kernel, kvm
  Cc: borntraeger, cohuck, pmorel, pasic, pbonzini, rkrcmar

On 19.04.2018 23:13, Tony Krowiak wrote:
> Introduces a new function to reset the crypto attributes for all
> vcpus whether they are running or not. Each vcpu in KVM will
> be removed from SIE prior to resetting the crypto attributes in its
> SIE state description. After all vcpus have had their crypto attributes
> reset the vcpus will be restored to SIE.
> 
> This function is incorporated into the kvm_s390_vm_set_crypto(kvm)
> function to fix a reported issue whereby the crypto key wrapping
> attributes could potentially get out of synch for running vcpus.
> 
> Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>

A reported-by for a code refactoring is strange.

> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
> ---
>  arch/s390/kvm/kvm-s390.c |   18 ++++++++++++++----
>  arch/s390/kvm/kvm-s390.h |   13 +++++++++++++
>  2 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index fa355a6..4fa3037 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -789,6 +789,19 @@ static int kvm_s390_set_mem_control(struct kvm *kvm, struct kvm_device_attr *att
>  	return ret;
>  }
>  
> +void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm)
> + {
> +	int i;
> +	struct kvm_vcpu *vcpu;
> +
> +	kvm_s390_vcpu_block_all(kvm);
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm)
> +	        kvm_s390_vcpu_crypto_setup(vcpu);
> +
> +	kvm_s390_vcpu_unblock_all(kvm);

This code has to be protected by kvm->lock. Can that be guaranteed by
the caller?

> +}
> +
>  static void kvm_s390_vcpu_crypto_setup(struct kvm_vcpu *vcpu);
>  
>  static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
> @@ -832,10 +845,7 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>  		return -ENXIO;
>  	}
>  
> -	kvm_for_each_vcpu(i, vcpu, kvm) {
> -		kvm_s390_vcpu_crypto_setup(vcpu);
> -		exit_sie(vcpu);
> -	}
> +	kvm_s390_vcpu_crypto_reset_all(kvm);
>  	mutex_unlock(&kvm->lock);
>  	return 0;
>  }
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index 1b5621f..981e3ba 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -410,4 +410,17 @@ static inline int kvm_s390_use_sca_entries(void)
>  }
>  void kvm_s390_reinject_machine_check(struct kvm_vcpu *vcpu,
>  				     struct mcck_volatile_info *mcck_info);
> +
> +/**
> + * kvm_s390_vcpu_crypto_reset_all
> + *
> + * Reset the crypto attributes for each vcpu. This can be done while the vcpus
> + * are running as each vcpu will be removed from SIE before resetting the crypt
> + * attributes and restored to SIE afterward.
> + *
> + * Note: The kvm->lock must be held while calling this function
> + *
> + * @kvm: the KVM guest
> + */
> +void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm);
>  #endif
> 


-- 

Thanks,

David / dhildenb

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus
  2018-04-20 12:26 ` David Hildenbrand
@ 2018-04-20 12:28   ` David Hildenbrand
  2018-04-22 15:06   ` Tony Krowiak
  1 sibling, 0 replies; 11+ messages in thread
From: David Hildenbrand @ 2018-04-20 12:28 UTC (permalink / raw)
  To: Tony Krowiak, linux-s390, linux-kernel, kvm
  Cc: borntraeger, cohuck, pmorel, pasic, pbonzini, rkrcmar

On 20.04.2018 14:26, David Hildenbrand wrote:
> On 19.04.2018 23:13, Tony Krowiak wrote:
>> Introduces a new function to reset the crypto attributes for all
>> vcpus whether they are running or not. Each vcpu in KVM will
>> be removed from SIE prior to resetting the crypto attributes in its
>> SIE state description. After all vcpus have had their crypto attributes
>> reset the vcpus will be restored to SIE.
>>
>> This function is incorporated into the kvm_s390_vm_set_crypto(kvm)
>> function to fix a reported issue whereby the crypto key wrapping
>> attributes could potentially get out of synch for running vcpus.
>>
>> Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> 
> A reported-by for a code refactoring is strange.
> 
>> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
>> ---
>>  arch/s390/kvm/kvm-s390.c |   18 ++++++++++++++----
>>  arch/s390/kvm/kvm-s390.h |   13 +++++++++++++
>>  2 files changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index fa355a6..4fa3037 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -789,6 +789,19 @@ static int kvm_s390_set_mem_control(struct kvm *kvm, struct kvm_device_attr *att
>>  	return ret;
>>  }
>>  
>> +void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm)
>> + {
>> +	int i;
>> +	struct kvm_vcpu *vcpu;
>> +
>> +	kvm_s390_vcpu_block_all(kvm);
>> +
>> +	kvm_for_each_vcpu(i, vcpu, kvm)
>> +	        kvm_s390_vcpu_crypto_setup(vcpu);
>> +
>> +	kvm_s390_vcpu_unblock_all(kvm);
> 
> This code has to be protected by kvm->lock. Can that be guaranteed by
> the caller?

Answering my own question: as the caller has access to struct kvm, the
can of course lock it :)

-- 

Thanks,

David / dhildenb

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus
  2018-04-19 21:13 [PATCH] KVM: s390: reset crypto attributes for all vcpus Tony Krowiak
                   ` (2 preceding siblings ...)
  2018-04-20 12:26 ` David Hildenbrand
@ 2018-04-21  0:11 ` kbuild test robot
  2018-04-22 16:42   ` Tony Krowiak
  3 siblings, 1 reply; 11+ messages in thread
From: kbuild test robot @ 2018-04-21  0:11 UTC (permalink / raw)
  To: Tony Krowiak
  Cc: kbuild-all, linux-s390, linux-kernel, kvm, borntraeger, cohuck,
	pmorel, pasic, akrowiak, pbonzini, rkrcmar

[-- Attachment #1: Type: text/plain, Size: 2790 bytes --]

Hi Tony,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on s390/features]
[also build test ERROR on v4.17-rc1 next-20180420]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Tony-Krowiak/KVM-s390-reset-crypto-attributes-for-all-vcpus/20180421-050734
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: s390-allyesconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=s390 

All error/warnings (new ones prefixed by >>):

   arch/s390/kvm/kvm-s390.c: In function 'kvm_s390_vcpu_crypto_reset_all':
>> arch/s390/kvm/kvm-s390.c:800:10: error: implicit declaration of function 'kvm_s390_vcpu_crypto_setup'; did you mean 'kvm_s390_vcpu_crypto_reset_all'? [-Werror=implicit-function-declaration]
             kvm_s390_vcpu_crypto_setup(vcpu);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~
             kvm_s390_vcpu_crypto_reset_all
   arch/s390/kvm/kvm-s390.c: At top level:
>> arch/s390/kvm/kvm-s390.c:805:13: warning: conflicting types for 'kvm_s390_vcpu_crypto_setup'
    static void kvm_s390_vcpu_crypto_setup(struct kvm_vcpu *vcpu);
                ^~~~~~~~~~~~~~~~~~~~~~~~~~
>> arch/s390/kvm/kvm-s390.c:805:13: error: static declaration of 'kvm_s390_vcpu_crypto_setup' follows non-static declaration
   arch/s390/kvm/kvm-s390.c:800:10: note: previous implicit declaration of 'kvm_s390_vcpu_crypto_setup' was here
             kvm_s390_vcpu_crypto_setup(vcpu);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/s390/kvm/kvm-s390.c: In function 'kvm_s390_vm_set_crypto':
   arch/s390/kvm/kvm-s390.c:810:6: warning: unused variable 'i' [-Wunused-variable]
     int i;
         ^
   arch/s390/kvm/kvm-s390.c:809:19: warning: unused variable 'vcpu' [-Wunused-variable]
     struct kvm_vcpu *vcpu;
                      ^~~~
   cc1: some warnings being treated as errors

vim +800 arch/s390/kvm/kvm-s390.c

   791	
   792	void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm)
   793	 {
   794		int i;
   795		struct kvm_vcpu *vcpu;
   796	
   797		kvm_s390_vcpu_block_all(kvm);
   798	
   799		kvm_for_each_vcpu(i, vcpu, kvm)
 > 800		        kvm_s390_vcpu_crypto_setup(vcpu);
   801	
   802		kvm_s390_vcpu_unblock_all(kvm);
   803	}
   804	
 > 805	static void kvm_s390_vcpu_crypto_setup(struct kvm_vcpu *vcpu);
   806	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 49275 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus
  2018-04-20 12:26 ` David Hildenbrand
  2018-04-20 12:28   ` David Hildenbrand
@ 2018-04-22 15:06   ` Tony Krowiak
  2018-04-22 15:53     ` Halil Pasic
  1 sibling, 1 reply; 11+ messages in thread
From: Tony Krowiak @ 2018-04-22 15:06 UTC (permalink / raw)
  To: David Hildenbrand, linux-s390, linux-kernel, kvm
  Cc: borntraeger, cohuck, pmorel, pasic, pbonzini, rkrcmar

On 04/20/2018 08:26 AM, David Hildenbrand wrote:
> On 19.04.2018 23:13, Tony Krowiak wrote:
>> Introduces a new function to reset the crypto attributes for all
>> vcpus whether they are running or not. Each vcpu in KVM will
>> be removed from SIE prior to resetting the crypto attributes in its
>> SIE state description. After all vcpus have had their crypto attributes
>> reset the vcpus will be restored to SIE.
>>
>> This function is incorporated into the kvm_s390_vm_set_crypto(kvm)
>> function to fix a reported issue whereby the crypto key wrapping
>> attributes could potentially get out of synch for running vcpus.
>>
>> Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> A reported-by for a code refactoring is strange.

I was asked to include this.

>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
>> ---
>>   arch/s390/kvm/kvm-s390.c |   18 ++++++++++++++----
>>   arch/s390/kvm/kvm-s390.h |   13 +++++++++++++
>>   2 files changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index fa355a6..4fa3037 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -789,6 +789,19 @@ static int kvm_s390_set_mem_control(struct kvm *kvm, struct kvm_device_attr *att
>>   	return ret;
>>   }
>>   
>> +void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm)
>> + {
>> +	int i;
>> +	struct kvm_vcpu *vcpu;
>> +
>> +	kvm_s390_vcpu_block_all(kvm);
>> +
>> +	kvm_for_each_vcpu(i, vcpu, kvm)
>> +	        kvm_s390_vcpu_crypto_setup(vcpu);
>> +
>> +	kvm_s390_vcpu_unblock_all(kvm);
> This code has to be protected by kvm->lock. Can that be guaranteed by
> the caller?

I can hold the kvm->lock in this function, but if you look at the 
function from which it
is called, kvm_s390_vm_set_crypto(vcpu) below, the kvm->lock is already 
held by that
function to do other work. I suppose the kvm_s390_vm_set_crypto(vcpu) 
instruction could
have released the lock prior to calling 
kvm_s390_vcpu_crypto_reset_all(kvm), but since
this function is called within a block of crypto work, it made sense to 
me to place
the responsibility for locking in the caller and include a comment in 
the comments for
the function definition:

Note: The kvm->lock must be held while calling this function



>
>> +}
>> +
>>   static void kvm_s390_vcpu_crypto_setup(struct kvm_vcpu *vcpu);
>>   
>>   static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>> @@ -832,10 +845,7 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>>   		return -ENXIO;
>>   	}
>>   
>> -	kvm_for_each_vcpu(i, vcpu, kvm) {
>> -		kvm_s390_vcpu_crypto_setup(vcpu);
>> -		exit_sie(vcpu);
>> -	}
>> +	kvm_s390_vcpu_crypto_reset_all(kvm);
>>   	mutex_unlock(&kvm->lock);
>>   	return 0;
>>   }
>> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
>> index 1b5621f..981e3ba 100644
>> --- a/arch/s390/kvm/kvm-s390.h
>> +++ b/arch/s390/kvm/kvm-s390.h
>> @@ -410,4 +410,17 @@ static inline int kvm_s390_use_sca_entries(void)
>>   }
>>   void kvm_s390_reinject_machine_check(struct kvm_vcpu *vcpu,
>>   				     struct mcck_volatile_info *mcck_info);
>> +
>> +/**
>> + * kvm_s390_vcpu_crypto_reset_all
>> + *
>> + * Reset the crypto attributes for each vcpu. This can be done while the vcpus
>> + * are running as each vcpu will be removed from SIE before resetting the crypt
>> + * attributes and restored to SIE afterward.
>> + *
>> + * Note: The kvm->lock must be held while calling this function
>> + *
>> + * @kvm: the KVM guest
>> + */
>> +void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm);
>>   #endif
>>
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus
  2018-04-20 12:15   ` Janosch Frank
@ 2018-04-22 15:10     ` Tony Krowiak
  0 siblings, 0 replies; 11+ messages in thread
From: Tony Krowiak @ 2018-04-22 15:10 UTC (permalink / raw)
  To: Janosch Frank, linux-s390, linux-kernel, kvm
  Cc: borntraeger, cohuck, pmorel, pasic, pbonzini, rkrcmar

On 04/20/2018 08:15 AM, Janosch Frank wrote:
> On 20.04.2018 13:59, Janosch Frank wrote:
>> Thanks, applied.
>>
> Well this does not compile, as you use kvm_s390_vcpu_crypto_setup before
> declaration. Please fix, then I'll take the patch.

Stupid mistake. I'll fix it.

>
>
>> On 19.04.2018 23:13, Tony Krowiak wrote:
>>> Introduces a new function to reset the crypto attributes for all
>>> vcpus whether they are running or not. Each vcpu in KVM will
>>> be removed from SIE prior to resetting the crypto attributes in its
>>> SIE state description. After all vcpus have had their crypto attributes
>>> reset the vcpus will be restored to SIE.
>>>
>>> This function is incorporated into the kvm_s390_vm_set_crypto(kvm)
>>> function to fix a reported issue whereby the crypto key wrapping
>>> attributes could potentially get out of synch for running vcpus.
>>>
>>> Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
>>> ---
>>>   arch/s390/kvm/kvm-s390.c |   18 ++++++++++++++----
>>>   arch/s390/kvm/kvm-s390.h |   13 +++++++++++++
>>>   2 files changed, 27 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index fa355a6..4fa3037 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -789,6 +789,19 @@ static int kvm_s390_set_mem_control(struct kvm *kvm, struct kvm_device_attr *att
>>>   	return ret;
>>>   }
>>>
>>> +void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm)
>>> + {
>>> +	int i;
>>> +	struct kvm_vcpu *vcpu;
>>> +
>>> +	kvm_s390_vcpu_block_all(kvm);
>>> +
>>> +	kvm_for_each_vcpu(i, vcpu, kvm)
>>> +	        kvm_s390_vcpu_crypto_setup(vcpu);
>>> +
>>> +	kvm_s390_vcpu_unblock_all(kvm);
>>> +}
>>> +
>>>   static void kvm_s390_vcpu_crypto_setup(struct kvm_vcpu *vcpu);
>>>
>>>   static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>>> @@ -832,10 +845,7 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>>>   		return -ENXIO;
>>>   	}
>>>
>>> -	kvm_for_each_vcpu(i, vcpu, kvm) {
>>> -		kvm_s390_vcpu_crypto_setup(vcpu);
>>> -		exit_sie(vcpu);
>>> -	}
>>> +	kvm_s390_vcpu_crypto_reset_all(kvm);
>>>   	mutex_unlock(&kvm->lock);
>>>   	return 0;
>>>   }
>>> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
>>> index 1b5621f..981e3ba 100644
>>> --- a/arch/s390/kvm/kvm-s390.h
>>> +++ b/arch/s390/kvm/kvm-s390.h
>>> @@ -410,4 +410,17 @@ static inline int kvm_s390_use_sca_entries(void)
>>>   }
>>>   void kvm_s390_reinject_machine_check(struct kvm_vcpu *vcpu,
>>>   				     struct mcck_volatile_info *mcck_info);
>>> +
>>> +/**
>>> + * kvm_s390_vcpu_crypto_reset_all
>>> + *
>>> + * Reset the crypto attributes for each vcpu. This can be done while the vcpus
>>> + * are running as each vcpu will be removed from SIE before resetting the crypt
>>> + * attributes and restored to SIE afterward.
>>> + *
>>> + * Note: The kvm->lock must be held while calling this function
>>> + *
>>> + * @kvm: the KVM guest
>>> + */
>>> +void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm);
>>>   #endif
>>>
>>
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus
  2018-04-22 15:06   ` Tony Krowiak
@ 2018-04-22 15:53     ` Halil Pasic
  0 siblings, 0 replies; 11+ messages in thread
From: Halil Pasic @ 2018-04-22 15:53 UTC (permalink / raw)
  To: Tony Krowiak, David Hildenbrand, linux-s390, linux-kernel, kvm
  Cc: borntraeger, cohuck, pmorel, pasic, pbonzini, rkrcmar



On 04/22/2018 05:06 PM, Tony Krowiak wrote:
> On 04/20/2018 08:26 AM, David Hildenbrand wrote:
>> On 19.04.2018 23:13, Tony Krowiak wrote:
>>> Introduces a new function to reset the crypto attributes for all
>>> vcpus whether they are running or not. Each vcpu in KVM will
>>> be removed from SIE prior to resetting the crypto attributes in its
>>> SIE state description. After all vcpus have had their crypto attributes
>>> reset the vcpus will be restored to SIE.
>>>
>>> This function is incorporated into the kvm_s390_vm_set_crypto(kvm)
>>> function to fix a reported issue whereby the crypto key wrapping
>>> attributes could potentially get out of synch for running vcpus.
>>>
>>> Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> A reported-by for a code refactoring is strange.
> 
> I was asked to include this.

Is this a refactoring or a fix? I the message you state that you are
fixing an issue (aka bug). If you are not, fixing an issue, but indeed
just refactoring, Suggested-by would be more appropriate.

Regards,
Halil

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus
  2018-04-21  0:11 ` kbuild test robot
@ 2018-04-22 16:42   ` Tony Krowiak
  0 siblings, 0 replies; 11+ messages in thread
From: Tony Krowiak @ 2018-04-22 16:42 UTC (permalink / raw)
  To: kbuild test robot
  Cc: kbuild-all, linux-s390, linux-kernel, kvm, borntraeger, cohuck,
	pmorel, pasic, pbonzini, rkrcmar

On 04/20/2018 08:11 PM, kbuild test robot wrote:
> Hi Tony,
>
> Thank you for the patch! Yet something to improve:

Sorry about this, I must have missed the warnings. It should all be good 
to do with v2 of
the patch.

>
> [auto build test ERROR on s390/features]
> [also build test ERROR on v4.17-rc1 next-20180420]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Tony-Krowiak/KVM-s390-reset-crypto-attributes-for-all-vcpus/20180421-050734
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
> config: s390-allyesconfig (attached as .config)
> compiler: s390x-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>          chmod +x ~/bin/make.cross
>          # save the attached .config to linux build tree
>          make.cross ARCH=s390
>
> All error/warnings (new ones prefixed by >>):
>
>     arch/s390/kvm/kvm-s390.c: In function 'kvm_s390_vcpu_crypto_reset_all':
>>> arch/s390/kvm/kvm-s390.c:800:10: error: implicit declaration of function 'kvm_s390_vcpu_crypto_setup'; did you mean 'kvm_s390_vcpu_crypto_reset_all'? [-Werror=implicit-function-declaration]
>               kvm_s390_vcpu_crypto_setup(vcpu);
>               ^~~~~~~~~~~~~~~~~~~~~~~~~~
>               kvm_s390_vcpu_crypto_reset_all
>     arch/s390/kvm/kvm-s390.c: At top level:
>>> arch/s390/kvm/kvm-s390.c:805:13: warning: conflicting types for 'kvm_s390_vcpu_crypto_setup'
>      static void kvm_s390_vcpu_crypto_setup(struct kvm_vcpu *vcpu);
>                  ^~~~~~~~~~~~~~~~~~~~~~~~~~
>>> arch/s390/kvm/kvm-s390.c:805:13: error: static declaration of 'kvm_s390_vcpu_crypto_setup' follows non-static declaration
>     arch/s390/kvm/kvm-s390.c:800:10: note: previous implicit declaration of 'kvm_s390_vcpu_crypto_setup' was here
>               kvm_s390_vcpu_crypto_setup(vcpu);
>               ^~~~~~~~~~~~~~~~~~~~~~~~~~
>     arch/s390/kvm/kvm-s390.c: In function 'kvm_s390_vm_set_crypto':
>     arch/s390/kvm/kvm-s390.c:810:6: warning: unused variable 'i' [-Wunused-variable]
>       int i;
>           ^
>     arch/s390/kvm/kvm-s390.c:809:19: warning: unused variable 'vcpu' [-Wunused-variable]
>       struct kvm_vcpu *vcpu;
>                        ^~~~
>     cc1: some warnings being treated as errors
>
> vim +800 arch/s390/kvm/kvm-s390.c
>
>     791	
>     792	void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm)
>     793	 {
>     794		int i;
>     795		struct kvm_vcpu *vcpu;
>     796	
>     797		kvm_s390_vcpu_block_all(kvm);
>     798	
>     799		kvm_for_each_vcpu(i, vcpu, kvm)
>   > 800		        kvm_s390_vcpu_crypto_setup(vcpu);
>     801	
>     802		kvm_s390_vcpu_unblock_all(kvm);
>     803	}
>     804	
>   > 805	static void kvm_s390_vcpu_crypto_setup(struct kvm_vcpu *vcpu);
>     806	
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-04-22 16:42 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-19 21:13 [PATCH] KVM: s390: reset crypto attributes for all vcpus Tony Krowiak
2018-04-20  8:57 ` Cornelia Huck
2018-04-20 11:59 ` Janosch Frank
2018-04-20 12:15   ` Janosch Frank
2018-04-22 15:10     ` Tony Krowiak
2018-04-20 12:26 ` David Hildenbrand
2018-04-20 12:28   ` David Hildenbrand
2018-04-22 15:06   ` Tony Krowiak
2018-04-22 15:53     ` Halil Pasic
2018-04-21  0:11 ` kbuild test robot
2018-04-22 16:42   ` Tony Krowiak

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).