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 */
-- 
1.7.1

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 */



-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

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 */


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

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.

-- 

Thanks,

David / dhildenb

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.
>