[PATCH] KVM: s390: reset crypto attributes for all vcpus

[PATCH] KVM: s390: reset crypto attributes for all vcpus

[Tony Krowiak]

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

Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus

[Cornelia Huck]

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>

Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus

[Janosch Frank]

[-- 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 --]

Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus

[Janosch Frank]

[-- 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 --]

Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus

[David Hildenbrand]

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

Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus

[David Hildenbrand]

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

Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus

[kbuild test robot]

[-- 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 --]

Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus

[Tony Krowiak]

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
>>
>

Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus

[Tony Krowiak]

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
>>>
>>
>

Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus

[Halil Pasic]

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

Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus

[Tony Krowiak]

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