| Message ID | 1521156300-19296-5-git-send-email-akrowiak@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: vfio-ap: guest dedicated crypto adapters | expand |
On 16/03/2018 00:24, Tony Krowiak wrote: > The VFIO AP device exploits interpretive execution of AP > instructions (APIE). APIE is enabled by setting a device attribute > via the KVM_SET_DEVICE_ATTR ioctl. > > Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com> > --- > target/s390x/kvm.c | 16 ++++++++++++++++ > target/s390x/kvm_s390x.h | 2 ++ > 2 files changed, 18 insertions(+), 0 deletions(-) > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index 33e5ec3..2812e28 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -277,6 +277,22 @@ static void kvm_s390_init_dea_kw(void) > } > } > > +int kvm_s390_set_interpret_ap(uint8_t enable) > +{ > + struct kvm_device_attr attribute = { > + .group = KVM_S390_VM_CRYPTO, > + .attr = KVM_S390_VM_CRYPTO_INTERPRET_AP, > + .addr = 1, > + }; > + > + if (!kvm_vm_check_attr(kvm_state, KVM_S390_VM_CRYPTO, > + KVM_S390_VM_CRYPTO_INTERPRET_AP)) { Isn't it enough to have the CPU feature ? What are expecting here? > + return -EOPNOTSUPP; > + } > + > + return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attribute); shouldn't you use the "enable" parameter somewhere? > +} > + > void kvm_s390_crypto_reset(void) > { > if (s390_has_feat(S390_FEAT_MSA_EXT_3)) { > diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h > index 34ee7e7..0d6c6e7 100644 > --- a/target/s390x/kvm_s390x.h > +++ b/target/s390x/kvm_s390x.h > @@ -40,4 +40,6 @@ void kvm_s390_crypto_reset(void); > void kvm_s390_restart_interrupt(S390CPU *cpu); > void kvm_s390_stop_interrupt(S390CPU *cpu); > > +int kvm_s390_set_interpret_ap(uint8_t enable); > + > #endif /* KVM_S390X_H */
On 16/03/2018 00:24, Tony Krowiak wrote: > The VFIO AP device exploits interpretive execution of AP > instructions (APIE). APIE is enabled by setting a device attribute > via the KVM_SET_DEVICE_ATTR ioctl. > > Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com> > --- > target/s390x/kvm.c | 16 ++++++++++++++++ > target/s390x/kvm_s390x.h | 2 ++ > 2 files changed, 18 insertions(+), 0 deletions(-) > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index 33e5ec3..2812e28 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -277,6 +277,22 @@ static void kvm_s390_init_dea_kw(void) > } > } > > +int kvm_s390_set_interpret_ap(uint8_t enable) > +{ > + struct kvm_device_attr attribute = { > + .group = KVM_S390_VM_CRYPTO, > + .attr = KVM_S390_VM_CRYPTO_INTERPRET_AP, > + .addr = 1, > + }; > + > + if (!kvm_vm_check_attr(kvm_state, KVM_S390_VM_CRYPTO, > + KVM_S390_VM_CRYPTO_INTERPRET_AP)) { Isn't it enough to have the CPU feature ? What are expecting here? > + return -EOPNOTSUPP; > + } > + > + return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attribute); Shouldn't you use the "enable" parameter somewhere? > +} > + > void kvm_s390_crypto_reset(void) > { > if (s390_has_feat(S390_FEAT_MSA_EXT_3)) { > diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h > index 34ee7e7..0d6c6e7 100644 > --- a/target/s390x/kvm_s390x.h > +++ b/target/s390x/kvm_s390x.h > @@ -40,4 +40,6 @@ void kvm_s390_crypto_reset(void); > void kvm_s390_restart_interrupt(S390CPU *cpu); > void kvm_s390_stop_interrupt(S390CPU *cpu); > > +int kvm_s390_set_interpret_ap(uint8_t enable); > + > #endif /* KVM_S390X_H */
On 03/16/2018 06:36 AM, Pierre Morel wrote: > On 16/03/2018 00:24, Tony Krowiak wrote: >> The VFIO AP device exploits interpretive execution of AP >> instructions (APIE). APIE is enabled by setting a device attribute >> via the KVM_SET_DEVICE_ATTR ioctl. >> >> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com> >> --- >> target/s390x/kvm.c | 16 ++++++++++++++++ >> target/s390x/kvm_s390x.h | 2 ++ >> 2 files changed, 18 insertions(+), 0 deletions(-) >> >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >> index 33e5ec3..2812e28 100644 >> --- a/target/s390x/kvm.c >> +++ b/target/s390x/kvm.c >> @@ -277,6 +277,22 @@ static void kvm_s390_init_dea_kw(void) >> } >> } >> >> +int kvm_s390_set_interpret_ap(uint8_t enable) >> +{ >> + struct kvm_device_attr attribute = { >> + .group = KVM_S390_VM_CRYPTO, >> + .attr = KVM_S390_VM_CRYPTO_INTERPRET_AP, >> + .addr = 1, >> + }; >> + >> + if (!kvm_vm_check_attr(kvm_state, KVM_S390_VM_CRYPTO, >> + KVM_S390_VM_CRYPTO_INTERPRET_AP)) { > > Isn't it enough to have the CPU feature ? I don't understand this question within this context. The code above checks to see whether the KVM_S390_VM_CRYPTO_INTERPRET_AP attribute is supported. > > What are expecting here? I'm expecting that if the KVM_S390_VM_CRYPTO_INTERPRET_AP attribute can not be set then that is an error condition that should be returned to the caller. > >> + return -EOPNOTSUPP; >> + } >> + >> + return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attribute); > > Shouldn't you use the "enable" parameter somewhere? If attribute.addr is not zero, then that indicates enable. If that is objectionable, I can change it. > >> +} >> + >> void kvm_s390_crypto_reset(void) >> { >> if (s390_has_feat(S390_FEAT_MSA_EXT_3)) { >> diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h >> index 34ee7e7..0d6c6e7 100644 >> --- a/target/s390x/kvm_s390x.h >> +++ b/target/s390x/kvm_s390x.h >> @@ -40,4 +40,6 @@ void kvm_s390_crypto_reset(void); >> void kvm_s390_restart_interrupt(S390CPU *cpu); >> void kvm_s390_stop_interrupt(S390CPU *cpu); >> >> +int kvm_s390_set_interpret_ap(uint8_t enable); >> + >> #endif /* KVM_S390X_H */ > >
On 03/15/2018 07:24 PM, Tony Krowiak wrote: > The VFIO AP device exploits interpretive execution of AP > instructions (APIE). APIE is enabled by setting a device attribute > via the KVM_SET_DEVICE_ATTR ioctl. > > Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com> > --- > target/s390x/kvm.c | 16 ++++++++++++++++ > target/s390x/kvm_s390x.h | 2 ++ > 2 files changed, 18 insertions(+), 0 deletions(-) > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index 33e5ec3..2812e28 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -277,6 +277,22 @@ static void kvm_s390_init_dea_kw(void) > } > } > > +int kvm_s390_set_interpret_ap(uint8_t enable) > +{ > + struct kvm_device_attr attribute = { > + .group = KVM_S390_VM_CRYPTO, > + .attr = KVM_S390_VM_CRYPTO_INTERPRET_AP, > + .addr = 1, > + }; > + > + if (!kvm_vm_check_attr(kvm_state, KVM_S390_VM_CRYPTO, > + KVM_S390_VM_CRYPTO_INTERPRET_AP)) { > + return -EOPNOTSUPP; > + } > + > + return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attribute); > +} I proposed removing the KVM_S390_VM_CRYPTO_INTERPRET_AP device attribute from the kernel in Message ID <1347ed2e-7bdb-e455-971a-cf60899e3c19@linux.vnet.ibm.com> on the kernel mailing list and proposed setting interpretive execution for AP instructions via the mdev open callback. That would eliminate the need for this patch. > + > void kvm_s390_crypto_reset(void) > { > if (s390_has_feat(S390_FEAT_MSA_EXT_3)) { > diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h > index 34ee7e7..0d6c6e7 100644 > --- a/target/s390x/kvm_s390x.h > +++ b/target/s390x/kvm_s390x.h > @@ -40,4 +40,6 @@ void kvm_s390_crypto_reset(void); > void kvm_s390_restart_interrupt(S390CPU *cpu); > void kvm_s390_stop_interrupt(S390CPU *cpu); > > +int kvm_s390_set_interpret_ap(uint8_t enable); > + > #endif /* KVM_S390X_H */
On 16.03.2018 00:24, Tony Krowiak wrote: > The VFIO AP device exploits interpretive execution of AP > instructions (APIE). APIE is enabled by setting a device attribute > via the KVM_SET_DEVICE_ATTR ioctl. > > Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com> > --- > target/s390x/kvm.c | 16 ++++++++++++++++ > target/s390x/kvm_s390x.h | 2 ++ > 2 files changed, 18 insertions(+), 0 deletions(-) > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index 33e5ec3..2812e28 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -277,6 +277,22 @@ static void kvm_s390_init_dea_kw(void) > } > } > > +int kvm_s390_set_interpret_ap(uint8_t enable) > +{ > + struct kvm_device_attr attribute = { > + .group = KVM_S390_VM_CRYPTO, > + .attr = KVM_S390_VM_CRYPTO_INTERPRET_AP, > + .addr = 1, > + }; > + > + if (!kvm_vm_check_attr(kvm_state, KVM_S390_VM_CRYPTO, > + KVM_S390_VM_CRYPTO_INTERPRET_AP)) { > + return -EOPNOTSUPP; > + } > + > + return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attribute); > +} > + > void kvm_s390_crypto_reset(void) > { > if (s390_has_feat(S390_FEAT_MSA_EXT_3)) { > diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h > index 34ee7e7..0d6c6e7 100644 > --- a/target/s390x/kvm_s390x.h > +++ b/target/s390x/kvm_s390x.h > @@ -40,4 +40,6 @@ void kvm_s390_crypto_reset(void); > void kvm_s390_restart_interrupt(S390CPU *cpu); > void kvm_s390_stop_interrupt(S390CPU *cpu); > > +int kvm_s390_set_interpret_ap(uint8_t enable); > + > #endif /* KVM_S390X_H */ > Wonder if a capability - like we use e.g. for SIGP user space interpretation - would be a better fit. We can provide the AP feature to the guest in case: - KVM_S390_VM_CPU_FEAT_AP ("interpretation support") is available - KVM_S390_VM_CRYPTO_INTERPRET_AP ("interception support") is available I am missing the second check in your code. (for now you only rely on KVM_S390_VM_CPU_FEAT_AP) I think you have to change the order of the patches so they work also properly when bisectin.
On 03/26/2018 04:38 AM, David Hildenbrand wrote: > On 16.03.2018 00:24, Tony Krowiak wrote: >> The VFIO AP device exploits interpretive execution of AP >> instructions (APIE). APIE is enabled by setting a device attribute >> via the KVM_SET_DEVICE_ATTR ioctl. >> >> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com> >> --- >> target/s390x/kvm.c | 16 ++++++++++++++++ >> target/s390x/kvm_s390x.h | 2 ++ >> 2 files changed, 18 insertions(+), 0 deletions(-) >> >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >> index 33e5ec3..2812e28 100644 >> --- a/target/s390x/kvm.c >> +++ b/target/s390x/kvm.c >> @@ -277,6 +277,22 @@ static void kvm_s390_init_dea_kw(void) >> } >> } >> >> +int kvm_s390_set_interpret_ap(uint8_t enable) >> +{ >> + struct kvm_device_attr attribute = { >> + .group = KVM_S390_VM_CRYPTO, >> + .attr = KVM_S390_VM_CRYPTO_INTERPRET_AP, >> + .addr = 1, >> + }; >> + >> + if (!kvm_vm_check_attr(kvm_state, KVM_S390_VM_CRYPTO, >> + KVM_S390_VM_CRYPTO_INTERPRET_AP)) { >> + return -EOPNOTSUPP; >> + } >> + >> + return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attribute); >> +} >> + >> void kvm_s390_crypto_reset(void) >> { >> if (s390_has_feat(S390_FEAT_MSA_EXT_3)) { >> diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h >> index 34ee7e7..0d6c6e7 100644 >> --- a/target/s390x/kvm_s390x.h >> +++ b/target/s390x/kvm_s390x.h >> @@ -40,4 +40,6 @@ void kvm_s390_crypto_reset(void); >> void kvm_s390_restart_interrupt(S390CPU *cpu); >> void kvm_s390_stop_interrupt(S390CPU *cpu); >> >> +int kvm_s390_set_interpret_ap(uint8_t enable); >> + >> #endif /* KVM_S390X_H */ >> > Wonder if a capability - like we use e.g. for SIGP user space > interpretation - would be a better fit. > > We can provide the AP feature to the guest in case: > - KVM_S390_VM_CPU_FEAT_AP ("interpretation support") is available > - KVM_S390_VM_CRYPTO_INTERPRET_AP ("interception support") is available I don't think we need this. I have found a way to set ECA_APIE from the vfio_ap device driver, so there is no need to do it from the guest. Stay tuned to this station for v4 of the patch series. > > I am missing the second check in your code. (for now you only rely on > KVM_S390_VM_CPU_FEAT_AP) For what else are you suggesting we need to check? > > I think you have to change the order of the patches so they work also > properly when bisectin. I'll take a look at it. >
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index 33e5ec3..2812e28 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -277,6 +277,22 @@ static void kvm_s390_init_dea_kw(void) } } +int kvm_s390_set_interpret_ap(uint8_t enable) +{ + struct kvm_device_attr attribute = { + .group = KVM_S390_VM_CRYPTO, + .attr = KVM_S390_VM_CRYPTO_INTERPRET_AP, + .addr = 1, + }; + + if (!kvm_vm_check_attr(kvm_state, KVM_S390_VM_CRYPTO, + KVM_S390_VM_CRYPTO_INTERPRET_AP)) { + return -EOPNOTSUPP; + } + + return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attribute); +} + void kvm_s390_crypto_reset(void) { if (s390_has_feat(S390_FEAT_MSA_EXT_3)) { diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h index 34ee7e7..0d6c6e7 100644 --- a/target/s390x/kvm_s390x.h +++ b/target/s390x/kvm_s390x.h @@ -40,4 +40,6 @@ void kvm_s390_crypto_reset(void); void kvm_s390_restart_interrupt(S390CPU *cpu); void kvm_s390_stop_interrupt(S390CPU *cpu); +int kvm_s390_set_interpret_ap(uint8_t enable); + #endif /* KVM_S390X_H */
The VFIO AP device exploits interpretive execution of AP instructions (APIE). APIE is enabled by setting a device attribute via the KVM_SET_DEVICE_ATTR ioctl. Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com> --- target/s390x/kvm.c | 16 ++++++++++++++++ target/s390x/kvm_s390x.h | 2 ++ 2 files changed, 18 insertions(+), 0 deletions(-)