| Message ID | 20180720131047.6859-1-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/20/2018 09:10 AM, Janosch Frank wrote: > Let's introduce an explicit check if skeys have already been enabled > for the vcpu, so we don't have to check the mm context if we don't > have the storage key facility. > > This let's us check for enablement without having to take the mm > semaphore and thus speedup skey emulation. Didn't the existing if statement already prevent going into the s390_enable_skey function? > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > arch/s390/include/asm/kvm_host.h | 1 + > arch/s390/kvm/priv.c | 5 ++--- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index a2188e309bd6..2916f0a5585c 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -655,6 +655,7 @@ struct kvm_vcpu_arch { > seqcount_t cputm_seqcount; > __u64 cputm_start; > bool gs_enabled; > + bool skey_enabled; > }; > > struct kvm_vm_stat { > diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c > index 83c678266588..e78c381c8a24 100644 > --- a/arch/s390/kvm/priv.c > +++ b/arch/s390/kvm/priv.c > @@ -209,9 +209,7 @@ int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu) > > trace_kvm_s390_skey_related_inst(vcpu); > /* Already enabled? */ > - if (vcpu->kvm->arch.use_skf && > - !(sie_block->ictl & (ICTL_ISKE | ICTL_SSKE | ICTL_RRBE)) && > - !kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS)) > + if (vcpu->arch.skey_enabled) > return 0; > > rc = s390_enable_skey(); > @@ -225,6 +223,7 @@ int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu) > sie_block->ictl |= ICTL_ISKE | ICTL_SSKE | ICTL_RRBE; > else > sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | ICTL_RRBE); > + vcpu->arch.skey_enabled = true; > return 0; > } > > Now we have 2 variables for skeys (use_skf and skey_enabled), shouldn't we combine them into one struct? I feel it would be easier to understand the code. Thanks Farhan
On 20.07.2018 15:36, Farhan Ali wrote: > > > On 07/20/2018 09:10 AM, Janosch Frank wrote: >> Let's introduce an explicit check if skeys have already been enabled >> for the vcpu, so we don't have to check the mm context if we don't >> have the storage key facility. > >> This let's us check for enablement without having to take the mm >> semaphore and thus speedup skey emulation. > > Didn't the existing if statement already prevent going into the > s390_enable_skey function? Only if skf is enabled, this takes also care of !skf. > >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> arch/s390/include/asm/kvm_host.h | 1 + >> arch/s390/kvm/priv.c | 5 ++--- >> 2 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >> index a2188e309bd6..2916f0a5585c 100644 >> --- a/arch/s390/include/asm/kvm_host.h >> +++ b/arch/s390/include/asm/kvm_host.h >> @@ -655,6 +655,7 @@ struct kvm_vcpu_arch { >> seqcount_t cputm_seqcount; >> __u64 cputm_start; >> bool gs_enabled; >> + bool skey_enabled; >> }; >> >> struct kvm_vm_stat { >> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c >> index 83c678266588..e78c381c8a24 100644 >> --- a/arch/s390/kvm/priv.c >> +++ b/arch/s390/kvm/priv.c >> @@ -209,9 +209,7 @@ int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu) >> >> trace_kvm_s390_skey_related_inst(vcpu); >> /* Already enabled? */ >> - if (vcpu->kvm->arch.use_skf && >> - !(sie_block->ictl & (ICTL_ISKE | ICTL_SSKE | ICTL_RRBE)) && >> - !kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS)) >> + if (vcpu->arch.skey_enabled) >> return 0; >> >> rc = s390_enable_skey(); >> @@ -225,6 +223,7 @@ int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu) >> sie_block->ictl |= ICTL_ISKE | ICTL_SSKE | ICTL_RRBE; >> else >> sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | ICTL_RRBE); >> + vcpu->arch.skey_enabled = true; >> return 0; >> } >> >> > > Now we have 2 variables for skeys (use_skf and skey_enabled), shouldn't > we combine them into one struct? I feel it would be easier to understand > the code. use_skf is the indication if we are allowed to use the storage key facility for skey interpretation. vcpu->arch.skey_enabled is a direct indication if we removed the ictls/kss on this vcpu. > > Thanks > Farhan >
On 20.07.2018 15:10, Janosch Frank wrote: > Let's introduce an explicit check if skeys have already been enabled > for the vcpu, so we don't have to check the mm context if we don't > have the storage key facility. > > This let's us check for enablement without having to take the mm > semaphore and thus speedup skey emulation. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > arch/s390/include/asm/kvm_host.h | 1 + > arch/s390/kvm/priv.c | 5 ++--- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index a2188e309bd6..2916f0a5585c 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -655,6 +655,7 @@ struct kvm_vcpu_arch { > seqcount_t cputm_seqcount; > __u64 cputm_start; > bool gs_enabled; > + bool skey_enabled; > }; > > struct kvm_vm_stat { > diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c > index 83c678266588..e78c381c8a24 100644 > --- a/arch/s390/kvm/priv.c > +++ b/arch/s390/kvm/priv.c > @@ -209,9 +209,7 @@ int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu) > > trace_kvm_s390_skey_related_inst(vcpu); > /* Already enabled? */ > - if (vcpu->kvm->arch.use_skf && > - !(sie_block->ictl & (ICTL_ISKE | ICTL_SSKE | ICTL_RRBE)) && > - !kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS)) > + if (vcpu->arch.skey_enabled) > return 0; > > rc = s390_enable_skey(); > @@ -225,6 +223,7 @@ int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu) > sie_block->ictl |= ICTL_ISKE | ICTL_SSKE | ICTL_RRBE; > else > sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | ICTL_RRBE); > + vcpu->arch.skey_enabled = true; > return 0; > } > > I never liked this part of the code. You might no be able to go ahead and turn the if (!vcpu->kvm->arch.use_skf) sie_block->ictl |= ICTL_ISKE | ICTL_SSKE | ICTL_RRBE; else sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | ICTL_RRBE); into a if (vcpu->kvm->arch.use_skf) sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | ICTL_RRBE);
On 20.07.2018 15:57, David Hildenbrand wrote: > On 20.07.2018 15:10, Janosch Frank wrote: >> Let's introduce an explicit check if skeys have already been enabled >> for the vcpu, so we don't have to check the mm context if we don't >> have the storage key facility. >> >> This let's us check for enablement without having to take the mm >> semaphore and thus speedup skey emulation. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> arch/s390/include/asm/kvm_host.h | 1 + >> arch/s390/kvm/priv.c | 5 ++--- >> 2 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >> index a2188e309bd6..2916f0a5585c 100644 >> --- a/arch/s390/include/asm/kvm_host.h >> +++ b/arch/s390/include/asm/kvm_host.h >> @@ -655,6 +655,7 @@ struct kvm_vcpu_arch { >> seqcount_t cputm_seqcount; >> __u64 cputm_start; >> bool gs_enabled; >> + bool skey_enabled; >> }; >> >> struct kvm_vm_stat { >> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c >> index 83c678266588..e78c381c8a24 100644 >> --- a/arch/s390/kvm/priv.c >> +++ b/arch/s390/kvm/priv.c >> @@ -209,9 +209,7 @@ int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu) >> >> trace_kvm_s390_skey_related_inst(vcpu); >> /* Already enabled? */ >> - if (vcpu->kvm->arch.use_skf && >> - !(sie_block->ictl & (ICTL_ISKE | ICTL_SSKE | ICTL_RRBE)) && >> - !kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS)) >> + if (vcpu->arch.skey_enabled) >> return 0; >> >> rc = s390_enable_skey(); >> @@ -225,6 +223,7 @@ int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu) >> sie_block->ictl |= ICTL_ISKE | ICTL_SSKE | ICTL_RRBE; >> else >> sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | ICTL_RRBE); >> + vcpu->arch.skey_enabled = true; >> return 0; >> } >> >> > > I never liked this part of the code. You might no be able to go ahead > and turn the > > if (!vcpu->kvm->arch.use_skf) > sie_block->ictl |= ICTL_ISKE | ICTL_SSKE | ICTL_RRBE; > else > sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | ICTL_RRBE); > > into a > > if (vcpu->kvm->arch.use_skf) > > sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | ICTL_RRBE); > Oh and if you touch it, please remove the nasty local variable "sie_block".
On 07/20/2018 09:40 AM, Janosch Frank wrote: >> Now we have 2 variables for skeys (use_skf and skey_enabled), shouldn't >> we combine them into one struct? I feel it would be easier to understand >> the code. > use_skf is the indication if we are allowed to use the storage key > facility for skey interpretation. vcpu->arch.skey_enabled is a direct > indication if we removed the ictls/kss on this vcpu. > Right, my point being if it made sense to have both the variables in one in one struct. Regardless of that, I think the patch is an improvement over the long if statement, so Acked-by: Farhan Ali <alifm@linux.ibm.com>
On 20.07.2018 16:10, Farhan Ali wrote: > > > On 07/20/2018 09:40 AM, Janosch Frank wrote: >>> Now we have 2 variables for skeys (use_skf and skey_enabled), shouldn't >>> we combine them into one struct? I feel it would be easier to understand >>> the code. >> use_skf is the indication if we are allowed to use the storage key >> facility for skey interpretation. vcpu->arch.skey_enabled is a direct >> indication if we removed the ictls/kss on this vcpu. >> > > Right, my point being if it made sense to have both the variables in one > in one struct. Regardless of that, I think the patch is an improvement > over the long if statement, so use_skf is guest wide, so it doesn't go into a vcpu struct, skey enabled is vcpu specific, so bundling them doesn't make sense. > > Acked-by: Farhan Ali <alifm@linux.ibm.com> > Thanks!
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h index a2188e309bd6..2916f0a5585c 100644 --- a/arch/s390/include/asm/kvm_host.h +++ b/arch/s390/include/asm/kvm_host.h @@ -655,6 +655,7 @@ struct kvm_vcpu_arch { seqcount_t cputm_seqcount; __u64 cputm_start; bool gs_enabled; + bool skey_enabled; }; struct kvm_vm_stat { diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c index 83c678266588..e78c381c8a24 100644 --- a/arch/s390/kvm/priv.c +++ b/arch/s390/kvm/priv.c @@ -209,9 +209,7 @@ int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu) trace_kvm_s390_skey_related_inst(vcpu); /* Already enabled? */ - if (vcpu->kvm->arch.use_skf && - !(sie_block->ictl & (ICTL_ISKE | ICTL_SSKE | ICTL_RRBE)) && - !kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS)) + if (vcpu->arch.skey_enabled) return 0; rc = s390_enable_skey(); @@ -225,6 +223,7 @@ int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu) sie_block->ictl |= ICTL_ISKE | ICTL_SSKE | ICTL_RRBE; else sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | ICTL_RRBE); + vcpu->arch.skey_enabled = true; return 0; }
Let's introduce an explicit check if skeys have already been enabled for the vcpu, so we don't have to check the mm context if we don't have the storage key facility. This let's us check for enablement without having to take the mm semaphore and thus speedup skey emulation. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- arch/s390/include/asm/kvm_host.h | 1 + arch/s390/kvm/priv.c | 5 ++--- 2 files changed, 3 insertions(+), 3 deletions(-)