* 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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-21 0:11 ` kbuild test robot
2018-04-20 11:59 ` Janosch Frank
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ 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] 12+ messages in thread* Re: [PATCH] KVM: s390: reset crypto attributes for all vcpus
@ 2018-04-21 0:11 ` kbuild test robot
0 siblings, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2018-04-21 0:11 UTC (permalink / raw)
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] 12+ 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
-1 siblings, 0 replies; 12+ 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] 12+ messages in thread