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 <[email protected]>
Signed-off-by: Tony Krowiak <[email protected]>
---
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
On Thu, 19 Apr 2018 17:13:52 -0400
Tony Krowiak <[email protected]> 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 <[email protected]>
> Signed-off-by: Tony Krowiak <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
> Signed-off-by: Tony Krowiak <[email protected]>
> ---
> 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
>
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 <[email protected]>
>> Signed-off-by: Tony Krowiak <[email protected]>
>> ---
>> 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
>>
>
>
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 <[email protected]>
A reported-by for a code refactoring is strange.
> Signed-off-by: Tony Krowiak <[email protected]>
> ---
> 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
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 <[email protected]>
>
> A reported-by for a code refactoring is strange.
>
>> Signed-off-by: Tony Krowiak <[email protected]>
>> ---
>> 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
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
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 <[email protected]>
> A reported-by for a code refactoring is strange.
I was asked to include this.
>
>> Signed-off-by: Tony Krowiak <[email protected]>
>> ---
>> 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
>>
>
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 <[email protected]>
>>> Signed-off-by: Tony Krowiak <[email protected]>
>>> ---
>>> 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
>>>
>>
>
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 <[email protected]>
>> 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
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