| Message ID | 1524172432-26211-1-git-send-email-akrowiak@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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 >
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 >> > >
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 >
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 :)
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 <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 >> >
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 >>> >> >
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
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
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
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(-)