[v10,24/26] KVM: s390: device attrs to enable/disable AP interpretation

Commit Message

Tony Krowiak Sept. 12, 2018, 7:43 p.m. UTC

From: Tony Krowiak <[e-mail]>

Introduces two new VM crypto device attributes (KVM_S390_VM_CRYPTO)
to enable or disable AP instruction interpretation from userspace
via the KVM_SET_DEVICE_ATTR ioctl:

* The KVM_S390_VM_CRYPTO_ENABLE_APIE attribute enables hardware
  interpretation of AP instructions executed on the guest.

* The KVM_S390_VM_CRYPTO_DISABLE_APIE attribute disables hardware
  interpretation of AP instructions executed on the guest. In this
  case the instructions will be intercepted and pass through to
  the guest.

Signed-off-by: Tony Krowiak <[e-mail]>
---
 arch/s390/include/asm/kvm_host.h |    1 +
 arch/s390/include/uapi/asm/kvm.h |    2 ++
 arch/s390/kvm/kvm-s390.c         |   27 +++++++++++++++++++++++----
 3 files changed, 26 insertions(+), 4 deletions(-)

Comments

David Hildenbrand Sept. 17, 2018, 8:51 a.m. UTC | #1

Am 12.09.18 um 21:43 schrieb Tony Krowiak:
> From: Tony Krowiak <[e-mail]>
> 
> Introduces two new VM crypto device attributes (KVM_S390_VM_CRYPTO)
> to enable or disable AP instruction interpretation from userspace
> via the KVM_SET_DEVICE_ATTR ioctl:
> 
> * The KVM_S390_VM_CRYPTO_ENABLE_APIE attribute enables hardware
>   interpretation of AP instructions executed on the guest.
> 
> * The KVM_S390_VM_CRYPTO_DISABLE_APIE attribute disables hardware
>   interpretation of AP instructions executed on the guest. In this
>   case the instructions will be intercepted and pass through to
>   the guest.
> 
> Signed-off-by: Tony Krowiak <[e-mail]>
> ---
>  arch/s390/include/asm/kvm_host.h |    1 +
>  arch/s390/include/uapi/asm/kvm.h |    2 ++
>  arch/s390/kvm/kvm-s390.c         |   27 +++++++++++++++++++++++----
>  3 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index b32bd1b..36d3531 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -719,6 +719,7 @@ struct kvm_s390_crypto {
>  	__u32 crycbd;
>  	__u8 aes_kw;
>  	__u8 dea_kw;
> +	__u8 apie;
>  };
>  
>  #define APCB0_MASK_SIZE 1
> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
> index 8c23afc..a8dbd90 100644
> --- a/arch/s390/include/uapi/asm/kvm.h
> +++ b/arch/s390/include/uapi/asm/kvm.h
> @@ -161,6 +161,8 @@ struct kvm_s390_vm_cpu_subfunc {
>  #define KVM_S390_VM_CRYPTO_ENABLE_DEA_KW	1
>  #define KVM_S390_VM_CRYPTO_DISABLE_AES_KW	2
>  #define KVM_S390_VM_CRYPTO_DISABLE_DEA_KW	3
> +#define KVM_S390_VM_CRYPTO_ENABLE_APIE		4
> +#define KVM_S390_VM_CRYPTO_DISABLE_APIE		5
>  
>  /* kvm attributes for migration mode */
>  #define KVM_S390_VM_MIGRATION_STOP	0
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 2cdd980..286c2e0 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -856,12 +856,11 @@ void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm)
>  
>  static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>  {
> -	if (!test_kvm_facility(kvm, 76))
> -		return -EINVAL;
> -
>  	mutex_lock(&kvm->lock);
>  	switch (attr->attr) {
>  	case KVM_S390_VM_CRYPTO_ENABLE_AES_KW:
> +		if (!test_kvm_facility(kvm, 76))
> +			return -EINVAL;
>  		get_random_bytes(
>  			kvm->arch.crypto.crycb->aes_wrapping_key_mask,
>  			sizeof(kvm->arch.crypto.crycb->aes_wrapping_key_mask));
> @@ -869,6 +868,8 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>  		VM_EVENT(kvm, 3, "%s", "ENABLE: AES keywrapping support");
>  		break;
>  	case KVM_S390_VM_CRYPTO_ENABLE_DEA_KW:
> +		if (!test_kvm_facility(kvm, 76))
> +			return -EINVAL;
>  		get_random_bytes(
>  			kvm->arch.crypto.crycb->dea_wrapping_key_mask,
>  			sizeof(kvm->arch.crypto.crycb->dea_wrapping_key_mask));
> @@ -876,17 +877,31 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>  		VM_EVENT(kvm, 3, "%s", "ENABLE: DEA keywrapping support");
>  		break;
>  	case KVM_S390_VM_CRYPTO_DISABLE_AES_KW:
> +		if (!test_kvm_facility(kvm, 76))
> +			return -EINVAL;
>  		kvm->arch.crypto.aes_kw = 0;
>  		memset(kvm->arch.crypto.crycb->aes_wrapping_key_mask, 0,
>  			sizeof(kvm->arch.crypto.crycb->aes_wrapping_key_mask));
>  		VM_EVENT(kvm, 3, "%s", "DISABLE: AES keywrapping support");
>  		break;
>  	case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
> +		if (!test_kvm_facility(kvm, 76))
> +			return -EINVAL;
>  		kvm->arch.crypto.dea_kw = 0;
>  		memset(kvm->arch.crypto.crycb->dea_wrapping_key_mask, 0,
>  			sizeof(kvm->arch.crypto.crycb->dea_wrapping_key_mask));
>  		VM_EVENT(kvm, 3, "%s", "DISABLE: DEA keywrapping support");
>  		break;
> +	case KVM_S390_VM_CRYPTO_ENABLE_APIE:
> +		if (!ap_instructions_available()) {
> +			mutex_unlock(&kvm->lock);
> +			return -EOPNOTSUPP;
> +		}
> +		kvm->arch.crypto.apie = 1;
> +		break;
> +	case KVM_S390_VM_CRYPTO_DISABLE_APIE:
> +		kvm->arch.crypto.apie = 0;
> +		break;
>  	default:
>  		mutex_unlock(&kvm->lock);
>  		return -ENXIO;
> @@ -1493,6 +1508,8 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>  		case KVM_S390_VM_CRYPTO_ENABLE_DEA_KW:
>  		case KVM_S390_VM_CRYPTO_DISABLE_AES_KW:
>  		case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
> +		case KVM_S390_VM_CRYPTO_ENABLE_APIE:
> +		case KVM_S390_VM_CRYPTO_DISABLE_APIE:

As also replied to the QEMU series, could we indicate
KVM_S390_VM_CRYPTO_ENABLE_APIE (and maybe
KVM_S390_VM_CRYPTO_DISABLE_APIE) only with ap_instructions_available(),
so we can avoid the additional KVM_S390_VM_CPU_FEAT_AP?

KVM_S390_VM_CPU_FEAT_AP is right now completely unused in KVM otherwise
(never checked, we only care about apie).

Anthony Krowiak Sept. 21, 2018, 11:40 p.m. UTC | #2

On 09/17/2018 04:51 AM, David Hildenbrand wrote:
> Am 12.09.18 um 21:43 schrieb Tony Krowiak:
>> From: Tony Krowiak <[e-mail]>
>>
>> Introduces two new VM crypto device attributes (KVM_S390_VM_CRYPTO)
>> to enable or disable AP instruction interpretation from userspace
>> via the KVM_SET_DEVICE_ATTR ioctl:
>>
>> * The KVM_S390_VM_CRYPTO_ENABLE_APIE attribute enables hardware
>>    interpretation of AP instructions executed on the guest.
>>
>> * The KVM_S390_VM_CRYPTO_DISABLE_APIE attribute disables hardware
>>    interpretation of AP instructions executed on the guest. In this
>>    case the instructions will be intercepted and pass through to
>>    the guest.
>>
>> Signed-off-by: Tony Krowiak <[e-mail]>
>> ---
>>   arch/s390/include/asm/kvm_host.h |    1 +
>>   arch/s390/include/uapi/asm/kvm.h |    2 ++
>>   arch/s390/kvm/kvm-s390.c         |   27 +++++++++++++++++++++++----
>>   3 files changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index b32bd1b..36d3531 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -719,6 +719,7 @@ struct kvm_s390_crypto {
>>   	__u32 crycbd;
>>   	__u8 aes_kw;
>>   	__u8 dea_kw;
>> +	__u8 apie;
>>   };
>>   
>>   #define APCB0_MASK_SIZE 1
>> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
>> index 8c23afc..a8dbd90 100644
>> --- a/arch/s390/include/uapi/asm/kvm.h
>> +++ b/arch/s390/include/uapi/asm/kvm.h
>> @@ -161,6 +161,8 @@ struct kvm_s390_vm_cpu_subfunc {
>>   #define KVM_S390_VM_CRYPTO_ENABLE_DEA_KW	1
>>   #define KVM_S390_VM_CRYPTO_DISABLE_AES_KW	2
>>   #define KVM_S390_VM_CRYPTO_DISABLE_DEA_KW	3
>> +#define KVM_S390_VM_CRYPTO_ENABLE_APIE		4
>> +#define KVM_S390_VM_CRYPTO_DISABLE_APIE		5
>>   
>>   /* kvm attributes for migration mode */
>>   #define KVM_S390_VM_MIGRATION_STOP	0
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 2cdd980..286c2e0 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -856,12 +856,11 @@ void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm)
>>   
>>   static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>>   {
>> -	if (!test_kvm_facility(kvm, 76))
>> -		return -EINVAL;
>> -
>>   	mutex_lock(&kvm->lock);
>>   	switch (attr->attr) {
>>   	case KVM_S390_VM_CRYPTO_ENABLE_AES_KW:
>> +		if (!test_kvm_facility(kvm, 76))
>> +			return -EINVAL;
>>   		get_random_bytes(
>>   			kvm->arch.crypto.crycb->aes_wrapping_key_mask,
>>   			sizeof(kvm->arch.crypto.crycb->aes_wrapping_key_mask));
>> @@ -869,6 +868,8 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>>   		VM_EVENT(kvm, 3, "%s", "ENABLE: AES keywrapping support");
>>   		break;
>>   	case KVM_S390_VM_CRYPTO_ENABLE_DEA_KW:
>> +		if (!test_kvm_facility(kvm, 76))
>> +			return -EINVAL;
>>   		get_random_bytes(
>>   			kvm->arch.crypto.crycb->dea_wrapping_key_mask,
>>   			sizeof(kvm->arch.crypto.crycb->dea_wrapping_key_mask));
>> @@ -876,17 +877,31 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>>   		VM_EVENT(kvm, 3, "%s", "ENABLE: DEA keywrapping support");
>>   		break;
>>   	case KVM_S390_VM_CRYPTO_DISABLE_AES_KW:
>> +		if (!test_kvm_facility(kvm, 76))
>> +			return -EINVAL;
>>   		kvm->arch.crypto.aes_kw = 0;
>>   		memset(kvm->arch.crypto.crycb->aes_wrapping_key_mask, 0,
>>   			sizeof(kvm->arch.crypto.crycb->aes_wrapping_key_mask));
>>   		VM_EVENT(kvm, 3, "%s", "DISABLE: AES keywrapping support");
>>   		break;
>>   	case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
>> +		if (!test_kvm_facility(kvm, 76))
>> +			return -EINVAL;
>>   		kvm->arch.crypto.dea_kw = 0;
>>   		memset(kvm->arch.crypto.crycb->dea_wrapping_key_mask, 0,
>>   			sizeof(kvm->arch.crypto.crycb->dea_wrapping_key_mask));
>>   		VM_EVENT(kvm, 3, "%s", "DISABLE: DEA keywrapping support");
>>   		break;
>> +	case KVM_S390_VM_CRYPTO_ENABLE_APIE:
>> +		if (!ap_instructions_available()) {
>> +			mutex_unlock(&kvm->lock);
>> +			return -EOPNOTSUPP;
>> +		}
>> +		kvm->arch.crypto.apie = 1;
>> +		break;
>> +	case KVM_S390_VM_CRYPTO_DISABLE_APIE:
>> +		kvm->arch.crypto.apie = 0;
>> +		break;
>>   	default:
>>   		mutex_unlock(&kvm->lock);
>>   		return -ENXIO;
>> @@ -1493,6 +1508,8 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>>   		case KVM_S390_VM_CRYPTO_ENABLE_DEA_KW:
>>   		case KVM_S390_VM_CRYPTO_DISABLE_AES_KW:
>>   		case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
>> +		case KVM_S390_VM_CRYPTO_ENABLE_APIE:
>> +		case KVM_S390_VM_CRYPTO_DISABLE_APIE:
> 
> As also replied to the QEMU series, could we indicate
> KVM_S390_VM_CRYPTO_ENABLE_APIE (and maybe
> KVM_S390_VM_CRYPTO_DISABLE_APIE) only with ap_instructions_available(),
> so we can avoid the additional KVM_S390_VM_CPU_FEAT_AP?
> 
> KVM_S390_VM_CPU_FEAT_AP is right now completely unused in KVM otherwise
> (never checked, we only care about apie).

After much discussion with Halil and a few exchanges with you, we
decided to go ahead and accept your suggestion to get rid of 
KVM_S390_VM_CPU_FEAT and keep the VM device attributes to enable/disable
apie.

To that end, I responded to patches 03/26, 11/26 and 25/26 with fixup!
patches that show the KVM/kernel changes that will be necessary to get
rid of KVM_S390_VM_CPU_FEAT and use apie to control ECA.28. I did that
to generate discussion in v10 rather than waiting until v11 for
comments. I make no guarantees that those fixup! patches will
successfully apply should you have a v10 branch generated from this
patch series you want to update.

>

David Hildenbrand Sept. 24, 2018, 11:23 a.m. UTC | #3

On 22/09/2018 01:40, Tony Krowiak wrote:
> On 09/17/2018 04:51 AM, David Hildenbrand wrote:
>> Am 12.09.18 um 21:43 schrieb Tony Krowiak:
>>> From: Tony Krowiak <[e-mail]>
>>>
>>> Introduces two new VM crypto device attributes (KVM_S390_VM_CRYPTO)
>>> to enable or disable AP instruction interpretation from userspace
>>> via the KVM_SET_DEVICE_ATTR ioctl:
>>>
>>> * The KVM_S390_VM_CRYPTO_ENABLE_APIE attribute enables hardware
>>>    interpretation of AP instructions executed on the guest.
>>>
>>> * The KVM_S390_VM_CRYPTO_DISABLE_APIE attribute disables hardware
>>>    interpretation of AP instructions executed on the guest. In this
>>>    case the instructions will be intercepted and pass through to
>>>    the guest.
>>>
>>> Signed-off-by: Tony Krowiak <[e-mail]>
>>> ---
>>>   arch/s390/include/asm/kvm_host.h |    1 +
>>>   arch/s390/include/uapi/asm/kvm.h |    2 ++
>>>   arch/s390/kvm/kvm-s390.c         |   27 +++++++++++++++++++++++----
>>>   3 files changed, 26 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>> index b32bd1b..36d3531 100644
>>> --- a/arch/s390/include/asm/kvm_host.h
>>> +++ b/arch/s390/include/asm/kvm_host.h
>>> @@ -719,6 +719,7 @@ struct kvm_s390_crypto {
>>>   	__u32 crycbd;
>>>   	__u8 aes_kw;
>>>   	__u8 dea_kw;
>>> +	__u8 apie;
>>>   };
>>>   
>>>   #define APCB0_MASK_SIZE 1
>>> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
>>> index 8c23afc..a8dbd90 100644
>>> --- a/arch/s390/include/uapi/asm/kvm.h
>>> +++ b/arch/s390/include/uapi/asm/kvm.h
>>> @@ -161,6 +161,8 @@ struct kvm_s390_vm_cpu_subfunc {
>>>   #define KVM_S390_VM_CRYPTO_ENABLE_DEA_KW	1
>>>   #define KVM_S390_VM_CRYPTO_DISABLE_AES_KW	2
>>>   #define KVM_S390_VM_CRYPTO_DISABLE_DEA_KW	3
>>> +#define KVM_S390_VM_CRYPTO_ENABLE_APIE		4
>>> +#define KVM_S390_VM_CRYPTO_DISABLE_APIE		5
>>>   
>>>   /* kvm attributes for migration mode */
>>>   #define KVM_S390_VM_MIGRATION_STOP	0
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index 2cdd980..286c2e0 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -856,12 +856,11 @@ void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm)
>>>   
>>>   static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>>>   {
>>> -	if (!test_kvm_facility(kvm, 76))
>>> -		return -EINVAL;
>>> -
>>>   	mutex_lock(&kvm->lock);
>>>   	switch (attr->attr) {
>>>   	case KVM_S390_VM_CRYPTO_ENABLE_AES_KW:
>>> +		if (!test_kvm_facility(kvm, 76))
>>> +			return -EINVAL;
>>>   		get_random_bytes(
>>>   			kvm->arch.crypto.crycb->aes_wrapping_key_mask,
>>>   			sizeof(kvm->arch.crypto.crycb->aes_wrapping_key_mask));
>>> @@ -869,6 +868,8 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>>>   		VM_EVENT(kvm, 3, "%s", "ENABLE: AES keywrapping support");
>>>   		break;
>>>   	case KVM_S390_VM_CRYPTO_ENABLE_DEA_KW:
>>> +		if (!test_kvm_facility(kvm, 76))
>>> +			return -EINVAL;
>>>   		get_random_bytes(
>>>   			kvm->arch.crypto.crycb->dea_wrapping_key_mask,
>>>   			sizeof(kvm->arch.crypto.crycb->dea_wrapping_key_mask));
>>> @@ -876,17 +877,31 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>>>   		VM_EVENT(kvm, 3, "%s", "ENABLE: DEA keywrapping support");
>>>   		break;
>>>   	case KVM_S390_VM_CRYPTO_DISABLE_AES_KW:
>>> +		if (!test_kvm_facility(kvm, 76))
>>> +			return -EINVAL;
>>>   		kvm->arch.crypto.aes_kw = 0;
>>>   		memset(kvm->arch.crypto.crycb->aes_wrapping_key_mask, 0,
>>>   			sizeof(kvm->arch.crypto.crycb->aes_wrapping_key_mask));
>>>   		VM_EVENT(kvm, 3, "%s", "DISABLE: AES keywrapping support");
>>>   		break;
>>>   	case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
>>> +		if (!test_kvm_facility(kvm, 76))
>>> +			return -EINVAL;
>>>   		kvm->arch.crypto.dea_kw = 0;
>>>   		memset(kvm->arch.crypto.crycb->dea_wrapping_key_mask, 0,
>>>   			sizeof(kvm->arch.crypto.crycb->dea_wrapping_key_mask));
>>>   		VM_EVENT(kvm, 3, "%s", "DISABLE: DEA keywrapping support");
>>>   		break;
>>> +	case KVM_S390_VM_CRYPTO_ENABLE_APIE:
>>> +		if (!ap_instructions_available()) {
>>> +			mutex_unlock(&kvm->lock);
>>> +			return -EOPNOTSUPP;
>>> +		}
>>> +		kvm->arch.crypto.apie = 1;
>>> +		break;
>>> +	case KVM_S390_VM_CRYPTO_DISABLE_APIE:
>>> +		kvm->arch.crypto.apie = 0;
>>> +		break;
>>>   	default:
>>>   		mutex_unlock(&kvm->lock);
>>>   		return -ENXIO;
>>> @@ -1493,6 +1508,8 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>>>   		case KVM_S390_VM_CRYPTO_ENABLE_DEA_KW:
>>>   		case KVM_S390_VM_CRYPTO_DISABLE_AES_KW:
>>>   		case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
>>> +		case KVM_S390_VM_CRYPTO_ENABLE_APIE:
>>> +		case KVM_S390_VM_CRYPTO_DISABLE_APIE:
>>
>> As also replied to the QEMU series, could we indicate
>> KVM_S390_VM_CRYPTO_ENABLE_APIE (and maybe
>> KVM_S390_VM_CRYPTO_DISABLE_APIE) only with ap_instructions_available(),
>> so we can avoid the additional KVM_S390_VM_CPU_FEAT_AP?
>>
>> KVM_S390_VM_CPU_FEAT_AP is right now completely unused in KVM otherwise
>> (never checked, we only care about apie).
> 
> After much discussion with Halil and a few exchanges with you, we
> decided to go ahead and accept your suggestion to get rid of 
> KVM_S390_VM_CPU_FEAT and keep the VM device attributes to enable/disable
> apie.
> 
> To that end, I responded to patches 03/26, 11/26 and 25/26 with fixup!
> patches that show the KVM/kernel changes that will be necessary to get
> rid of KVM_S390_VM_CPU_FEAT and use apie to control ECA.28. I did that
> to generate discussion in v10 rather than waiting until v11 for
> comments. I make no guarantees that those fixup! patches will
> successfully apply should you have a v10 branch generated from this
> patch series you want to update.
> 

Will you also fixup this patch to expose KVM_S390_VM_CRYPTO_ENABLE_APIE
only if supported by HW? (ap_instructions_available)

Anthony Krowiak Sept. 24, 2018, 4:25 p.m. UTC | #4

On 09/24/2018 07:23 AM, David Hildenbrand wrote:
> On 22/09/2018 01:40, Tony Krowiak wrote:
>> On 09/17/2018 04:51 AM, David Hildenbrand wrote:
>>> Am 12.09.18 um 21:43 schrieb Tony Krowiak:
>>>> From: Tony Krowiak <[e-mail]>
>>>>
>>>> Introduces two new VM crypto device attributes (KVM_S390_VM_CRYPTO)
>>>> to enable or disable AP instruction interpretation from userspace
>>>> via the KVM_SET_DEVICE_ATTR ioctl:
>>>>
>>>> * The KVM_S390_VM_CRYPTO_ENABLE_APIE attribute enables hardware
>>>>     interpretation of AP instructions executed on the guest.
>>>>
>>>> * The KVM_S390_VM_CRYPTO_DISABLE_APIE attribute disables hardware
>>>>     interpretation of AP instructions executed on the guest. In this
>>>>     case the instructions will be intercepted and pass through to
>>>>     the guest.
>>>>
>>>> Signed-off-by: Tony Krowiak <[e-mail]>
>>>> ---
>>>>    arch/s390/include/asm/kvm_host.h |    1 +
>>>>    arch/s390/include/uapi/asm/kvm.h |    2 ++
>>>>    arch/s390/kvm/kvm-s390.c         |   27 +++++++++++++++++++++++----
>>>>    3 files changed, 26 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>>> index b32bd1b..36d3531 100644
>>>> --- a/arch/s390/include/asm/kvm_host.h
>>>> +++ b/arch/s390/include/asm/kvm_host.h
>>>> @@ -719,6 +719,7 @@ struct kvm_s390_crypto {
>>>>    	__u32 crycbd;
>>>>    	__u8 aes_kw;
>>>>    	__u8 dea_kw;
>>>> +	__u8 apie;
>>>>    };
>>>>    
>>>>    #define APCB0_MASK_SIZE 1
>>>> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
>>>> index 8c23afc..a8dbd90 100644
>>>> --- a/arch/s390/include/uapi/asm/kvm.h
>>>> +++ b/arch/s390/include/uapi/asm/kvm.h
>>>> @@ -161,6 +161,8 @@ struct kvm_s390_vm_cpu_subfunc {
>>>>    #define KVM_S390_VM_CRYPTO_ENABLE_DEA_KW	1
>>>>    #define KVM_S390_VM_CRYPTO_DISABLE_AES_KW	2
>>>>    #define KVM_S390_VM_CRYPTO_DISABLE_DEA_KW	3
>>>> +#define KVM_S390_VM_CRYPTO_ENABLE_APIE		4
>>>> +#define KVM_S390_VM_CRYPTO_DISABLE_APIE		5
>>>>    
>>>>    /* kvm attributes for migration mode */
>>>>    #define KVM_S390_VM_MIGRATION_STOP	0
>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>> index 2cdd980..286c2e0 100644
>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>> @@ -856,12 +856,11 @@ void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm)
>>>>    
>>>>    static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>>>>    {
>>>> -	if (!test_kvm_facility(kvm, 76))
>>>> -		return -EINVAL;
>>>> -
>>>>    	mutex_lock(&kvm->lock);
>>>>    	switch (attr->attr) {
>>>>    	case KVM_S390_VM_CRYPTO_ENABLE_AES_KW:
>>>> +		if (!test_kvm_facility(kvm, 76))
>>>> +			return -EINVAL;
>>>>    		get_random_bytes(
>>>>    			kvm->arch.crypto.crycb->aes_wrapping_key_mask,
>>>>    			sizeof(kvm->arch.crypto.crycb->aes_wrapping_key_mask));
>>>> @@ -869,6 +868,8 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>>>>    		VM_EVENT(kvm, 3, "%s", "ENABLE: AES keywrapping support");
>>>>    		break;
>>>>    	case KVM_S390_VM_CRYPTO_ENABLE_DEA_KW:
>>>> +		if (!test_kvm_facility(kvm, 76))
>>>> +			return -EINVAL;
>>>>    		get_random_bytes(
>>>>    			kvm->arch.crypto.crycb->dea_wrapping_key_mask,
>>>>    			sizeof(kvm->arch.crypto.crycb->dea_wrapping_key_mask));
>>>> @@ -876,17 +877,31 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>>>>    		VM_EVENT(kvm, 3, "%s", "ENABLE: DEA keywrapping support");
>>>>    		break;
>>>>    	case KVM_S390_VM_CRYPTO_DISABLE_AES_KW:
>>>> +		if (!test_kvm_facility(kvm, 76))
>>>> +			return -EINVAL;
>>>>    		kvm->arch.crypto.aes_kw = 0;
>>>>    		memset(kvm->arch.crypto.crycb->aes_wrapping_key_mask, 0,
>>>>    			sizeof(kvm->arch.crypto.crycb->aes_wrapping_key_mask));
>>>>    		VM_EVENT(kvm, 3, "%s", "DISABLE: AES keywrapping support");
>>>>    		break;
>>>>    	case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
>>>> +		if (!test_kvm_facility(kvm, 76))
>>>> +			return -EINVAL;
>>>>    		kvm->arch.crypto.dea_kw = 0;
>>>>    		memset(kvm->arch.crypto.crycb->dea_wrapping_key_mask, 0,
>>>>    			sizeof(kvm->arch.crypto.crycb->dea_wrapping_key_mask));
>>>>    		VM_EVENT(kvm, 3, "%s", "DISABLE: DEA keywrapping support");
>>>>    		break;
>>>> +	case KVM_S390_VM_CRYPTO_ENABLE_APIE:
>>>> +		if (!ap_instructions_available()) {
>>>> +			mutex_unlock(&kvm->lock);
>>>> +			return -EOPNOTSUPP;
>>>> +		}
>>>> +		kvm->arch.crypto.apie = 1;
>>>> +		break;
>>>> +	case KVM_S390_VM_CRYPTO_DISABLE_APIE:
>>>> +		kvm->arch.crypto.apie = 0;
>>>> +		break;
>>>>    	default:
>>>>    		mutex_unlock(&kvm->lock);
>>>>    		return -ENXIO;
>>>> @@ -1493,6 +1508,8 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>>>>    		case KVM_S390_VM_CRYPTO_ENABLE_DEA_KW:
>>>>    		case KVM_S390_VM_CRYPTO_DISABLE_AES_KW:
>>>>    		case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
>>>> +		case KVM_S390_VM_CRYPTO_ENABLE_APIE:
>>>> +		case KVM_S390_VM_CRYPTO_DISABLE_APIE:
>>>
>>> As also replied to the QEMU series, could we indicate
>>> KVM_S390_VM_CRYPTO_ENABLE_APIE (and maybe
>>> KVM_S390_VM_CRYPTO_DISABLE_APIE) only with ap_instructions_available(),
>>> so we can avoid the additional KVM_S390_VM_CPU_FEAT_AP?
>>>
>>> KVM_S390_VM_CPU_FEAT_AP is right now completely unused in KVM otherwise
>>> (never checked, we only care about apie).
>>
>> After much discussion with Halil and a few exchanges with you, we
>> decided to go ahead and accept your suggestion to get rid of
>> KVM_S390_VM_CPU_FEAT and keep the VM device attributes to enable/disable
>> apie.
>>
>> To that end, I responded to patches 03/26, 11/26 and 25/26 with fixup!
>> patches that show the KVM/kernel changes that will be necessary to get
>> rid of KVM_S390_VM_CPU_FEAT and use apie to control ECA.28. I did that
>> to generate discussion in v10 rather than waiting until v11 for
>> comments. I make no guarantees that those fixup! patches will
>> successfully apply should you have a v10 branch generated from this
>> patch series you want to update.
>>
> 
> Will you also fixup this patch to expose KVM_S390_VM_CRYPTO_ENABLE_APIE
> only if supported by HW? (ap_instructions_available)

Given that this patch DOES expose KVM_S390_VM_CRYPTO_ENABLE_APIE only if 
supported by HW, I assume you are talking about
KVM_S390_VM_CRYPTO_DISABLE_APIE. I didn't check 
ap_instructions_available() for disabling APIE because I didn't
think it necessary given that ECA.28 will be set to 0 (intercept) by 
default, whether AP instructions are installed or not; so why not allow 
disabling apie. I suppose from the perspective of consistency, since the 
kvm_s390_vm_has_attr() function checks ap_instructions_available() for 
both attributes, then it probably makes sense to add that check to 
KVM_S390_VM_CRYPTO_DISABLE_APIE here. Then again, we could make a change 
in ap_instructions_available() to allow KVM_S390_VM_CRYPTO_DISABLE_APIE 
regardless of whether AP instructions are available. It boils down to 
whether APIE needs to be dynamically disabled at some point when it has 
been enabled. The only case I can think of where that may be necessary 
is if a guest is migrated to a system without AP instructions. I don't 
think that can happen and may even be protected against precisely 
because the VM attributes won't be available on the target system due to 
no AP instructions. What say you?

>

Anthony Krowiak Sept. 24, 2018, 6:42 p.m. UTC | #5

On 09/24/2018 12:25 PM, Tony Krowiak wrote:
> On 09/24/2018 07:23 AM, David Hildenbrand wrote:

(...)

>> Will you also fixup this patch to expose KVM_S390_VM_CRYPTO_ENABLE_APIE
>> only if supported by HW? (ap_instructions_available)
> 
> Given that this patch DOES expose KVM_S390_VM_CRYPTO_ENABLE_APIE only if 
> supported by HW, I assume you are talking about
> KVM_S390_VM_CRYPTO_DISABLE_APIE. I didn't check 
> ap_instructions_available() for disabling APIE because I didn't
> think it necessary given that ECA.28 will be set to 0 (intercept) by 
> default, whether AP instructions are installed or not; so why not allow 
> disabling apie. I suppose from the perspective of consistency, since the 
> kvm_s390_vm_has_attr() function checks ap_instructions_available() for 
> both attributes, then it probably makes sense to add that check to 
> KVM_S390_VM_CRYPTO_DISABLE_APIE here. Then again, we could make a change 
> in ap_instructions_available() to allow KVM_S390_VM_CRYPTO_DISABLE_APIE 
> regardless of whether AP instructions are available. It boils down to 
> whether APIE needs to be dynamically disabled at some point when it has 
> been enabled. The only case I can think of where that may be necessary 
> is if a guest is migrated to a system without AP instructions. I don't 
> think that can happen and may even be protected against precisely 
> because the VM attributes won't be available on the target system due to 
> no AP instructions. What say you?
> 
David,

I'm sorry, I misinterpreted what you were asking for. Check out the 
fixup! patch below and let me know if that is what you are looking for.
If so, I will integrate that change and post v11 tomorrow (Tuesday 9/24).

-----------------------------------8<-----------------------------------

From: Tony Krowiak <[e-mail]>
Date: Mon, 24 Sep 2018 14:18:37 -0400
Subject: [FIXUP v10] fixup! KVM: s390: device attrs to enable/disable AP
  interpretation

---
  arch/s390/kvm/kvm-s390.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 6654bb1fc26a..a528558baa78 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -901,6 +901,10 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, 
struct kvm_device_attr *attr)
  		kvm->arch.crypto.apie = 1;
  		break;
  	case KVM_S390_VM_CRYPTO_DISABLE_APIE:
+		if (!ap_instructions_available()) {
+			mutex_unlock(&kvm->lock);
+			return -EOPNOTSUPP;
+		}
  		kvm->arch.crypto.apie = 0;
  		break;
  	default:
@@ -1509,9 +1513,11 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, 
struct kvm_device_attr *attr)
  		case KVM_S390_VM_CRYPTO_ENABLE_DEA_KW:
  		case KVM_S390_VM_CRYPTO_DISABLE_AES_KW:
  		case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
+			ret = 0;
+			break;
  		case KVM_S390_VM_CRYPTO_ENABLE_APIE:
  		case KVM_S390_VM_CRYPTO_DISABLE_APIE:
-			ret = 0;
+			ret = ap_instructions_available();
  			break;
  		default:
  			ret = -ENXIO;
@@ -2620,6 +2626,7 @@ static void kvm_s390_vcpu_crypto_setup(struct 
kvm_vcpu *vcpu)

  	vcpu->arch.sie_block->crycbd = vcpu->kvm->arch.crypto.crycbd;
  	vcpu->arch.sie_block->ecb3 &= ~(ECB3_AES | ECB3_DEA);
+	vcpu->kvm->arch.crypto.apie &= ~ECA_APIE;

  	if (vcpu->kvm->arch.crypto.apie)
  		vcpu->arch.sie_block->eca |= ECA_APIE;

David Hildenbrand Sept. 24, 2018, 6:46 p.m. UTC | #6

On 24/09/2018 18:25, Tony Krowiak wrote:
> On 09/24/2018 07:23 AM, David Hildenbrand wrote:
>> On 22/09/2018 01:40, Tony Krowiak wrote:
>>> On 09/17/2018 04:51 AM, David Hildenbrand wrote:
>>>> Am 12.09.18 um 21:43 schrieb Tony Krowiak:
>>>>> From: Tony Krowiak <[e-mail]>
>>>>>
>>>>> Introduces two new VM crypto device attributes (KVM_S390_VM_CRYPTO)
>>>>> to enable or disable AP instruction interpretation from userspace
>>>>> via the KVM_SET_DEVICE_ATTR ioctl:
>>>>>
>>>>> * The KVM_S390_VM_CRYPTO_ENABLE_APIE attribute enables hardware
>>>>>     interpretation of AP instructions executed on the guest.
>>>>>
>>>>> * The KVM_S390_VM_CRYPTO_DISABLE_APIE attribute disables hardware
>>>>>     interpretation of AP instructions executed on the guest. In this
>>>>>     case the instructions will be intercepted and pass through to
>>>>>     the guest.
>>>>>
>>>>> Signed-off-by: Tony Krowiak <[e-mail]>
>>>>> ---
>>>>>    arch/s390/include/asm/kvm_host.h |    1 +
>>>>>    arch/s390/include/uapi/asm/kvm.h |    2 ++
>>>>>    arch/s390/kvm/kvm-s390.c         |   27 +++++++++++++++++++++++----
>>>>>    3 files changed, 26 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>>>> index b32bd1b..36d3531 100644
>>>>> --- a/arch/s390/include/asm/kvm_host.h
>>>>> +++ b/arch/s390/include/asm/kvm_host.h
>>>>> @@ -719,6 +719,7 @@ struct kvm_s390_crypto {
>>>>>    	__u32 crycbd;
>>>>>    	__u8 aes_kw;
>>>>>    	__u8 dea_kw;
>>>>> +	__u8 apie;
>>>>>    };
>>>>>    
>>>>>    #define APCB0_MASK_SIZE 1
>>>>> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
>>>>> index 8c23afc..a8dbd90 100644
>>>>> --- a/arch/s390/include/uapi/asm/kvm.h
>>>>> +++ b/arch/s390/include/uapi/asm/kvm.h
>>>>> @@ -161,6 +161,8 @@ struct kvm_s390_vm_cpu_subfunc {
>>>>>    #define KVM_S390_VM_CRYPTO_ENABLE_DEA_KW	1
>>>>>    #define KVM_S390_VM_CRYPTO_DISABLE_AES_KW	2
>>>>>    #define KVM_S390_VM_CRYPTO_DISABLE_DEA_KW	3
>>>>> +#define KVM_S390_VM_CRYPTO_ENABLE_APIE		4
>>>>> +#define KVM_S390_VM_CRYPTO_DISABLE_APIE		5
>>>>>    
>>>>>    /* kvm attributes for migration mode */
>>>>>    #define KVM_S390_VM_MIGRATION_STOP	0
>>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>>> index 2cdd980..286c2e0 100644
>>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>>> @@ -856,12 +856,11 @@ void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm)
>>>>>    
>>>>>    static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>>>>>    {
>>>>> -	if (!test_kvm_facility(kvm, 76))
>>>>> -		return -EINVAL;
>>>>> -
>>>>>    	mutex_lock(&kvm->lock);
>>>>>    	switch (attr->attr) {
>>>>>    	case KVM_S390_VM_CRYPTO_ENABLE_AES_KW:
>>>>> +		if (!test_kvm_facility(kvm, 76))
>>>>> +			return -EINVAL;
>>>>>    		get_random_bytes(
>>>>>    			kvm->arch.crypto.crycb->aes_wrapping_key_mask,
>>>>>    			sizeof(kvm->arch.crypto.crycb->aes_wrapping_key_mask));
>>>>> @@ -869,6 +868,8 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>>>>>    		VM_EVENT(kvm, 3, "%s", "ENABLE: AES keywrapping support");
>>>>>    		break;
>>>>>    	case KVM_S390_VM_CRYPTO_ENABLE_DEA_KW:
>>>>> +		if (!test_kvm_facility(kvm, 76))
>>>>> +			return -EINVAL;
>>>>>    		get_random_bytes(
>>>>>    			kvm->arch.crypto.crycb->dea_wrapping_key_mask,
>>>>>    			sizeof(kvm->arch.crypto.crycb->dea_wrapping_key_mask));
>>>>> @@ -876,17 +877,31 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>>>>>    		VM_EVENT(kvm, 3, "%s", "ENABLE: DEA keywrapping support");
>>>>>    		break;
>>>>>    	case KVM_S390_VM_CRYPTO_DISABLE_AES_KW:
>>>>> +		if (!test_kvm_facility(kvm, 76))
>>>>> +			return -EINVAL;
>>>>>    		kvm->arch.crypto.aes_kw = 0;
>>>>>    		memset(kvm->arch.crypto.crycb->aes_wrapping_key_mask, 0,
>>>>>    			sizeof(kvm->arch.crypto.crycb->aes_wrapping_key_mask));
>>>>>    		VM_EVENT(kvm, 3, "%s", "DISABLE: AES keywrapping support");
>>>>>    		break;
>>>>>    	case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
>>>>> +		if (!test_kvm_facility(kvm, 76))
>>>>> +			return -EINVAL;
>>>>>    		kvm->arch.crypto.dea_kw = 0;
>>>>>    		memset(kvm->arch.crypto.crycb->dea_wrapping_key_mask, 0,
>>>>>    			sizeof(kvm->arch.crypto.crycb->dea_wrapping_key_mask));
>>>>>    		VM_EVENT(kvm, 3, "%s", "DISABLE: DEA keywrapping support");
>>>>>    		break;
>>>>> +	case KVM_S390_VM_CRYPTO_ENABLE_APIE:
>>>>> +		if (!ap_instructions_available()) {
>>>>> +			mutex_unlock(&kvm->lock);
>>>>> +			return -EOPNOTSUPP;
>>>>> +		}
>>>>> +		kvm->arch.crypto.apie = 1;
>>>>> +		break;
>>>>> +	case KVM_S390_VM_CRYPTO_DISABLE_APIE:
>>>>> +		kvm->arch.crypto.apie = 0;
>>>>> +		break;
>>>>>    	default:
>>>>>    		mutex_unlock(&kvm->lock);
>>>>>    		return -ENXIO;
>>>>> @@ -1493,6 +1508,8 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>>>>>    		case KVM_S390_VM_CRYPTO_ENABLE_DEA_KW:
>>>>>    		case KVM_S390_VM_CRYPTO_DISABLE_AES_KW:
>>>>>    		case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
>>>>> +		case KVM_S390_VM_CRYPTO_ENABLE_APIE:
>>>>> +		case KVM_S390_VM_CRYPTO_DISABLE_APIE:
>>>>
>>>> As also replied to the QEMU series, could we indicate
>>>> KVM_S390_VM_CRYPTO_ENABLE_APIE (and maybe
>>>> KVM_S390_VM_CRYPTO_DISABLE_APIE) only with ap_instructions_available(),
>>>> so we can avoid the additional KVM_S390_VM_CPU_FEAT_AP?
>>>>
>>>> KVM_S390_VM_CPU_FEAT_AP is right now completely unused in KVM otherwise
>>>> (never checked, we only care about apie).
>>>
>>> After much discussion with Halil and a few exchanges with you, we
>>> decided to go ahead and accept your suggestion to get rid of
>>> KVM_S390_VM_CPU_FEAT and keep the VM device attributes to enable/disable
>>> apie.
>>>
>>> To that end, I responded to patches 03/26, 11/26 and 25/26 with fixup!
>>> patches that show the KVM/kernel changes that will be necessary to get
>>> rid of KVM_S390_VM_CPU_FEAT and use apie to control ECA.28. I did that
>>> to generate discussion in v10 rather than waiting until v11 for
>>> comments. I make no guarantees that those fixup! patches will
>>> successfully apply should you have a v10 branch generated from this
>>> patch series you want to update.
>>>
>>
>> Will you also fixup this patch to expose KVM_S390_VM_CRYPTO_ENABLE_APIE
>> only if supported by HW? (ap_instructions_available)
> 
> Given that this patch DOES expose KVM_S390_VM_CRYPTO_ENABLE_APIE only if 
> supported by HW, I assume you are talking about
> KVM_S390_VM_CRYPTO_DISABLE_APIE. I didn't check 
> ap_instructions_available() for disabling APIE because I didn't
> think it necessary given that ECA.28 will be set to 0 (intercept) by 
> default, whether AP instructions are installed or not; so why not allow 
> disabling apie. I suppose from the perspective of consistency, since the 
> kvm_s390_vm_has_attr() function checks ap_instructions_available() for 
> both attributes, then it probably makes sense to add that check to 
> KVM_S390_VM_CRYPTO_DISABLE_APIE here. Then again, we could make a change 
> in ap_instructions_available() to allow KVM_S390_VM_CRYPTO_DISABLE_APIE 
> regardless of whether AP instructions are available. It boils down to 
> whether APIE needs to be dynamically disabled at some point when it has 
> been enabled. The only case I can think of where that may be necessary 
> is if a guest is migrated to a system without AP instructions. I don't 
> think that can happen and may even be protected against precisely 
> because the VM attributes won't be available on the target system due to 
> no AP instructions. What say you?
> 
>>
> 

Just so we're on the same page, I am talking about exposing, I talk
about indicating the attribute:

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 03c23045527f..40924fe05bdf 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1491,6 +1491,11 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm,
struct kvm_device_attr *attr)
                case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
                        ret = 0;
                        break;
+               case KVM_S390_VM_CRYPTO_ENABLE_APIE:
+               case KVM_S390_VM_CRYPTO_DISABLE_APIE:
+                       ret = -ENXIO;
+                       if (ap_instructions_available())
+                               ret = 0;
                default:
                        ret = -ENXIO;
                        break;

KVM_S390_VM_CRYPTO_DISABLE_APIE can either be handled like
KVM_S390_VM_CRYPTO_ENABLE_APIE (return -EOPNOTSUPP) when setting or
always be allowed. I'll leave that up to you. But as it is completely
useless without ap_instructions_available() /
KVM_S390_VM_CRYPTO_ENABLE_APIE , we might as well also just not expose
it then.

David Hildenbrand Sept. 24, 2018, 6:51 p.m. UTC | #7

On 24/09/2018 20:42, Tony Krowiak wrote:
> On 09/24/2018 12:25 PM, Tony Krowiak wrote:
>> On 09/24/2018 07:23 AM, David Hildenbrand wrote:
> 
> (...)
> 
>>> Will you also fixup this patch to expose KVM_S390_VM_CRYPTO_ENABLE_APIE
>>> only if supported by HW? (ap_instructions_available)
>>
>> Given that this patch DOES expose KVM_S390_VM_CRYPTO_ENABLE_APIE only if 
>> supported by HW, I assume you are talking about
>> KVM_S390_VM_CRYPTO_DISABLE_APIE. I didn't check 
>> ap_instructions_available() for disabling APIE because I didn't
>> think it necessary given that ECA.28 will be set to 0 (intercept) by 
>> default, whether AP instructions are installed or not; so why not allow 
>> disabling apie. I suppose from the perspective of consistency, since the 
>> kvm_s390_vm_has_attr() function checks ap_instructions_available() for 
>> both attributes, then it probably makes sense to add that check to 
>> KVM_S390_VM_CRYPTO_DISABLE_APIE here. Then again, we could make a change 
>> in ap_instructions_available() to allow KVM_S390_VM_CRYPTO_DISABLE_APIE 
>> regardless of whether AP instructions are available. It boils down to 
>> whether APIE needs to be dynamically disabled at some point when it has 
>> been enabled. The only case I can think of where that may be necessary 
>> is if a guest is migrated to a system without AP instructions. I don't 
>> think that can happen and may even be protected against precisely 
>> because the VM attributes won't be available on the target system due to 
>> no AP instructions. What say you?
>>
> David,
> 
> I'm sorry, I misinterpreted what you were asking for. Check out the 
> fixup! patch below and let me know if that is what you are looking for.
> If so, I will integrate that change and post v11 tomorrow (Tuesday 9/24).
> 
> -----------------------------------8<-----------------------------------
> 
> From: Tony Krowiak <[e-mail]>
> Date: Mon, 24 Sep 2018 14:18:37 -0400
> Subject: [FIXUP v10] fixup! KVM: s390: device attrs to enable/disable AP
>   interpretation
> 
> ---
>   arch/s390/kvm/kvm-s390.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 6654bb1fc26a..a528558baa78 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -901,6 +901,10 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, 
> struct kvm_device_attr *attr)
>   		kvm->arch.crypto.apie = 1;
>   		break;
>   	case KVM_S390_VM_CRYPTO_DISABLE_APIE:
> +		if (!ap_instructions_available()) {
> +			mutex_unlock(&kvm->lock);
> +			return -EOPNOTSUPP;
> +		}
>   		kvm->arch.crypto.apie = 0;
>   		break;
>   	default:
> @@ -1509,9 +1513,11 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, 
> struct kvm_device_attr *attr)
>   		case KVM_S390_VM_CRYPTO_ENABLE_DEA_KW:
>   		case KVM_S390_VM_CRYPTO_DISABLE_AES_KW:
>   		case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
> +			ret = 0;
> +			break;
>   		case KVM_S390_VM_CRYPTO_ENABLE_APIE:
>   		case KVM_S390_VM_CRYPTO_DISABLE_APIE:
> -			ret = 0;
> +			ret = ap_instructions_available();
>   			break;
>   		default:
>   			ret = -ENXIO;
> @@ -2620,6 +2626,7 @@ static void kvm_s390_vcpu_crypto_setup(struct 
> kvm_vcpu *vcpu)
> 
>   	vcpu->arch.sie_block->crycbd = vcpu->kvm->arch.crypto.crycbd;
>   	vcpu->arch.sie_block->ecb3 &= ~(ECB3_AES | ECB3_DEA);
> +	vcpu->kvm->arch.crypto.apie &= ~ECA_APIE;

Did you mean to set vcpu->arch.sie_block->eca here?

> 
>   	if (vcpu->kvm->arch.crypto.apie)
>   		vcpu->arch.sie_block->eca |= ECA_APIE;
> 

Apart from that, just what I had in mind :)

David Hildenbrand Sept. 25, 2018, 7:32 a.m. UTC | #8

On 24/09/2018 20:42, Tony Krowiak wrote:
> On 09/24/2018 12:25 PM, Tony Krowiak wrote:
>> On 09/24/2018 07:23 AM, David Hildenbrand wrote:
> 
> (...)
> 
>>> Will you also fixup this patch to expose KVM_S390_VM_CRYPTO_ENABLE_APIE
>>> only if supported by HW? (ap_instructions_available)
>>
>> Given that this patch DOES expose KVM_S390_VM_CRYPTO_ENABLE_APIE only if 
>> supported by HW, I assume you are talking about
>> KVM_S390_VM_CRYPTO_DISABLE_APIE. I didn't check 
>> ap_instructions_available() for disabling APIE because I didn't
>> think it necessary given that ECA.28 will be set to 0 (intercept) by 
>> default, whether AP instructions are installed or not; so why not allow 
>> disabling apie. I suppose from the perspective of consistency, since the 
>> kvm_s390_vm_has_attr() function checks ap_instructions_available() for 
>> both attributes, then it probably makes sense to add that check to 
>> KVM_S390_VM_CRYPTO_DISABLE_APIE here. Then again, we could make a change 
>> in ap_instructions_available() to allow KVM_S390_VM_CRYPTO_DISABLE_APIE 
>> regardless of whether AP instructions are available. It boils down to 
>> whether APIE needs to be dynamically disabled at some point when it has 
>> been enabled. The only case I can think of where that may be necessary 
>> is if a guest is migrated to a system without AP instructions. I don't 
>> think that can happen and may even be protected against precisely 
>> because the VM attributes won't be available on the target system due to 
>> no AP instructions. What say you?
>>
> David,
> 
> I'm sorry, I misinterpreted what you were asking for. Check out the 
> fixup! patch below and let me know if that is what you are looking for.
> If so, I will integrate that change and post v11 tomorrow (Tuesday 9/24).
> 
> -----------------------------------8<-----------------------------------
> 
> From: Tony Krowiak <[e-mail]>
> Date: Mon, 24 Sep 2018 14:18:37 -0400
> Subject: [FIXUP v10] fixup! KVM: s390: device attrs to enable/disable AP
>   interpretation
> 
> ---
>   arch/s390/kvm/kvm-s390.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 6654bb1fc26a..a528558baa78 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -901,6 +901,10 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, 
> struct kvm_device_attr *attr)
>   		kvm->arch.crypto.apie = 1;
>   		break;
>   	case KVM_S390_VM_CRYPTO_DISABLE_APIE:
> +		if (!ap_instructions_available()) {
> +			mutex_unlock(&kvm->lock);
> +			return -EOPNOTSUPP;
> +		}
>   		kvm->arch.crypto.apie = 0;
>   		break;
>   	default:
> @@ -1509,9 +1513,11 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, 
> struct kvm_device_attr *attr)
>   		case KVM_S390_VM_CRYPTO_ENABLE_DEA_KW:
>   		case KVM_S390_VM_CRYPTO_DISABLE_AES_KW:
>   		case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
> +			ret = 0;
> +			break;
>   		case KVM_S390_VM_CRYPTO_ENABLE_APIE:
>   		case KVM_S390_VM_CRYPTO_DISABLE_APIE:
> -			ret = 0;
> +			ret = ap_instructions_available();

Just a little remark, I guess we want to report 0 if available and
-ENXIO if not.

Anthony Krowiak Sept. 25, 2018, 1:24 p.m. UTC | #9

On 09/24/2018 02:51 PM, David Hildenbrand wrote:
> On 24/09/2018 20:42, Tony Krowiak wrote:
>> On 09/24/2018 12:25 PM, Tony Krowiak wrote:
>>> On 09/24/2018 07:23 AM, David Hildenbrand wrote:
>>
>> (...)
>>
>>>> Will you also fixup this patch to expose KVM_S390_VM_CRYPTO_ENABLE_APIE
>>>> only if supported by HW? (ap_instructions_available)
>>>
>>> Given that this patch DOES expose KVM_S390_VM_CRYPTO_ENABLE_APIE only if
>>> supported by HW, I assume you are talking about
>>> KVM_S390_VM_CRYPTO_DISABLE_APIE. I didn't check
>>> ap_instructions_available() for disabling APIE because I didn't
>>> think it necessary given that ECA.28 will be set to 0 (intercept) by
>>> default, whether AP instructions are installed or not; so why not allow
>>> disabling apie. I suppose from the perspective of consistency, since the
>>> kvm_s390_vm_has_attr() function checks ap_instructions_available() for
>>> both attributes, then it probably makes sense to add that check to
>>> KVM_S390_VM_CRYPTO_DISABLE_APIE here. Then again, we could make a change
>>> in ap_instructions_available() to allow KVM_S390_VM_CRYPTO_DISABLE_APIE
>>> regardless of whether AP instructions are available. It boils down to
>>> whether APIE needs to be dynamically disabled at some point when it has
>>> been enabled. The only case I can think of where that may be necessary
>>> is if a guest is migrated to a system without AP instructions. I don't
>>> think that can happen and may even be protected against precisely
>>> because the VM attributes won't be available on the target system due to
>>> no AP instructions. What say you?
>>>
>> David,
>>
>> I'm sorry, I misinterpreted what you were asking for. Check out the
>> fixup! patch below and let me know if that is what you are looking for.
>> If so, I will integrate that change and post v11 tomorrow (Tuesday 9/24).
>>
>> -----------------------------------8<-----------------------------------
>>
>> From: Tony Krowiak <[e-mail]>
>> Date: Mon, 24 Sep 2018 14:18:37 -0400
>> Subject: [FIXUP v10] fixup! KVM: s390: device attrs to enable/disable AP
>>    interpretation
>>
>> ---
>>    arch/s390/kvm/kvm-s390.c | 9 ++++++++-
>>    1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 6654bb1fc26a..a528558baa78 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -901,6 +901,10 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm,
>> struct kvm_device_attr *attr)
>>    		kvm->arch.crypto.apie = 1;
>>    		break;
>>    	case KVM_S390_VM_CRYPTO_DISABLE_APIE:
>> +		if (!ap_instructions_available()) {
>> +			mutex_unlock(&kvm->lock);
>> +			return -EOPNOTSUPP;
>> +		}
>>    		kvm->arch.crypto.apie = 0;
>>    		break;
>>    	default:
>> @@ -1509,9 +1513,11 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm,
>> struct kvm_device_attr *attr)
>>    		case KVM_S390_VM_CRYPTO_ENABLE_DEA_KW:
>>    		case KVM_S390_VM_CRYPTO_DISABLE_AES_KW:
>>    		case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
>> +			ret = 0;
>> +			break;
>>    		case KVM_S390_VM_CRYPTO_ENABLE_APIE:
>>    		case KVM_S390_VM_CRYPTO_DISABLE_APIE:
>> -			ret = 0;
>> +			ret = ap_instructions_available();
>>    			break;
>>    		default:
>>    			ret = -ENXIO;
>> @@ -2620,6 +2626,7 @@ static void kvm_s390_vcpu_crypto_setup(struct
>> kvm_vcpu *vcpu)
>>
>>    	vcpu->arch.sie_block->crycbd = vcpu->kvm->arch.crypto.crycbd;
>>    	vcpu->arch.sie_block->ecb3 &= ~(ECB3_AES | ECB3_DEA);
>> +	vcpu->kvm->arch.crypto.apie &= ~ECA_APIE;
> 
> Did you mean to set vcpu->arch.sie_block->eca here?

Yes!

> 
>>
>>    	if (vcpu->kvm->arch.crypto.apie)
>>    		vcpu->arch.sie_block->eca |= ECA_APIE;
>>
> 
> Apart from that, just what I had in mind :)
>

Anthony Krowiak Sept. 25, 2018, 1:26 p.m. UTC | #10

On 09/25/2018 03:32 AM, David Hildenbrand wrote:
> On 24/09/2018 20:42, Tony Krowiak wrote:
>> On 09/24/2018 12:25 PM, Tony Krowiak wrote:
>>> On 09/24/2018 07:23 AM, David Hildenbrand wrote:
>>
>> (...)
>>
>>>> Will you also fixup this patch to expose KVM_S390_VM_CRYPTO_ENABLE_APIE
>>>> only if supported by HW? (ap_instructions_available)
>>>
>>> Given that this patch DOES expose KVM_S390_VM_CRYPTO_ENABLE_APIE only if
>>> supported by HW, I assume you are talking about
>>> KVM_S390_VM_CRYPTO_DISABLE_APIE. I didn't check
>>> ap_instructions_available() for disabling APIE because I didn't
>>> think it necessary given that ECA.28 will be set to 0 (intercept) by
>>> default, whether AP instructions are installed or not; so why not allow
>>> disabling apie. I suppose from the perspective of consistency, since the
>>> kvm_s390_vm_has_attr() function checks ap_instructions_available() for
>>> both attributes, then it probably makes sense to add that check to
>>> KVM_S390_VM_CRYPTO_DISABLE_APIE here. Then again, we could make a change
>>> in ap_instructions_available() to allow KVM_S390_VM_CRYPTO_DISABLE_APIE
>>> regardless of whether AP instructions are available. It boils down to
>>> whether APIE needs to be dynamically disabled at some point when it has
>>> been enabled. The only case I can think of where that may be necessary
>>> is if a guest is migrated to a system without AP instructions. I don't
>>> think that can happen and may even be protected against precisely
>>> because the VM attributes won't be available on the target system due to
>>> no AP instructions. What say you?
>>>
>> David,
>>
>> I'm sorry, I misinterpreted what you were asking for. Check out the
>> fixup! patch below and let me know if that is what you are looking for.
>> If so, I will integrate that change and post v11 tomorrow (Tuesday 9/24).
>>
>> -----------------------------------8<-----------------------------------
>>
>> From: Tony Krowiak <[e-mail]>
>> Date: Mon, 24 Sep 2018 14:18:37 -0400
>> Subject: [FIXUP v10] fixup! KVM: s390: device attrs to enable/disable AP
>>    interpretation
>>
>> ---
>>    arch/s390/kvm/kvm-s390.c | 9 ++++++++-
>>    1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 6654bb1fc26a..a528558baa78 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -901,6 +901,10 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm,
>> struct kvm_device_attr *attr)
>>    		kvm->arch.crypto.apie = 1;
>>    		break;
>>    	case KVM_S390_VM_CRYPTO_DISABLE_APIE:
>> +		if (!ap_instructions_available()) {
>> +			mutex_unlock(&kvm->lock);
>> +			return -EOPNOTSUPP;
>> +		}
>>    		kvm->arch.crypto.apie = 0;
>>    		break;
>>    	default:
>> @@ -1509,9 +1513,11 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm,
>> struct kvm_device_attr *attr)
>>    		case KVM_S390_VM_CRYPTO_ENABLE_DEA_KW:
>>    		case KVM_S390_VM_CRYPTO_DISABLE_AES_KW:
>>    		case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
>> +			ret = 0;
>> +			break;
>>    		case KVM_S390_VM_CRYPTO_ENABLE_APIE:
>>    		case KVM_S390_VM_CRYPTO_DISABLE_APIE:
>> -			ret = 0;
>> +			ret = ap_instructions_available();
> 
> Just a little remark, I guess we want to report 0 if available and
> -ENXIO if not.

That makes sense ... I'll fix it.

>

Anthony Krowiak Sept. 25, 2018, 1:31 p.m. UTC | #11

On 09/24/2018 02:46 PM, David Hildenbrand wrote:
> On 24/09/2018 18:25, Tony Krowiak wrote:
>> On 09/24/2018 07:23 AM, David Hildenbrand wrote:
>>> On 22/09/2018 01:40, Tony Krowiak wrote:
>>>> On 09/17/2018 04:51 AM, David Hildenbrand wrote:
>>>>> Am 12.09.18 um 21:43 schrieb Tony Krowiak:
>>>>>> From: Tony Krowiak <[e-mail]>
>>>>>>
>>>>>> Introduces two new VM crypto device attributes (KVM_S390_VM_CRYPTO)
>>>>>> to enable or disable AP instruction interpretation from userspace
>>>>>> via the KVM_SET_DEVICE_ATTR ioctl:
>>>>>>
>>>>>> * The KVM_S390_VM_CRYPTO_ENABLE_APIE attribute enables hardware
>>>>>>      interpretation of AP instructions executed on the guest.
>>>>>>
>>>>>> * The KVM_S390_VM_CRYPTO_DISABLE_APIE attribute disables hardware
>>>>>>      interpretation of AP instructions executed on the guest. In this
>>>>>>      case the instructions will be intercepted and pass through to
>>>>>>      the guest.
>>>>>>
>>>>>> Signed-off-by: Tony Krowiak <[e-mail]>
>>>>>> ---
>>>>>>     arch/s390/include/asm/kvm_host.h |    1 +
>>>>>>     arch/s390/include/uapi/asm/kvm.h |    2 ++
>>>>>>     arch/s390/kvm/kvm-s390.c         |   27 +++++++++++++++++++++++----
>>>>>>     3 files changed, 26 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>>>>> index b32bd1b..36d3531 100644
>>>>>> --- a/arch/s390/include/asm/kvm_host.h
>>>>>> +++ b/arch/s390/include/asm/kvm_host.h
>>>>>> @@ -719,6 +719,7 @@ struct kvm_s390_crypto {
>>>>>>     	__u32 crycbd;
>>>>>>     	__u8 aes_kw;
>>>>>>     	__u8 dea_kw;
>>>>>> +	__u8 apie;
>>>>>>     };
>>>>>>     
>>>>>>     #define APCB0_MASK_SIZE 1
>>>>>> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
>>>>>> index 8c23afc..a8dbd90 100644
>>>>>> --- a/arch/s390/include/uapi/asm/kvm.h
>>>>>> +++ b/arch/s390/include/uapi/asm/kvm.h
>>>>>> @@ -161,6 +161,8 @@ struct kvm_s390_vm_cpu_subfunc {
>>>>>>     #define KVM_S390_VM_CRYPTO_ENABLE_DEA_KW	1
>>>>>>     #define KVM_S390_VM_CRYPTO_DISABLE_AES_KW	2
>>>>>>     #define KVM_S390_VM_CRYPTO_DISABLE_DEA_KW	3
>>>>>> +#define KVM_S390_VM_CRYPTO_ENABLE_APIE		4
>>>>>> +#define KVM_S390_VM_CRYPTO_DISABLE_APIE		5
>>>>>>     
>>>>>>     /* kvm attributes for migration mode */
>>>>>>     #define KVM_S390_VM_MIGRATION_STOP	0
>>>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>>>> index 2cdd980..286c2e0 100644
>>>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>>>> @@ -856,12 +856,11 @@ void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm)
>>>>>>     
>>>>>>     static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>>>>>>     {
>>>>>> -	if (!test_kvm_facility(kvm, 76))
>>>>>> -		return -EINVAL;
>>>>>> -
>>>>>>     	mutex_lock(&kvm->lock);
>>>>>>     	switch (attr->attr) {
>>>>>>     	case KVM_S390_VM_CRYPTO_ENABLE_AES_KW:
>>>>>> +		if (!test_kvm_facility(kvm, 76))
>>>>>> +			return -EINVAL;
>>>>>>     		get_random_bytes(
>>>>>>     			kvm->arch.crypto.crycb->aes_wrapping_key_mask,
>>>>>>     			sizeof(kvm->arch.crypto.crycb->aes_wrapping_key_mask));
>>>>>> @@ -869,6 +868,8 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>>>>>>     		VM_EVENT(kvm, 3, "%s", "ENABLE: AES keywrapping support");
>>>>>>     		break;
>>>>>>     	case KVM_S390_VM_CRYPTO_ENABLE_DEA_KW:
>>>>>> +		if (!test_kvm_facility(kvm, 76))
>>>>>> +			return -EINVAL;
>>>>>>     		get_random_bytes(
>>>>>>     			kvm->arch.crypto.crycb->dea_wrapping_key_mask,
>>>>>>     			sizeof(kvm->arch.crypto.crycb->dea_wrapping_key_mask));
>>>>>> @@ -876,17 +877,31 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
>>>>>>     		VM_EVENT(kvm, 3, "%s", "ENABLE: DEA keywrapping support");
>>>>>>     		break;
>>>>>>     	case KVM_S390_VM_CRYPTO_DISABLE_AES_KW:
>>>>>> +		if (!test_kvm_facility(kvm, 76))
>>>>>> +			return -EINVAL;
>>>>>>     		kvm->arch.crypto.aes_kw = 0;
>>>>>>     		memset(kvm->arch.crypto.crycb->aes_wrapping_key_mask, 0,
>>>>>>     			sizeof(kvm->arch.crypto.crycb->aes_wrapping_key_mask));
>>>>>>     		VM_EVENT(kvm, 3, "%s", "DISABLE: AES keywrapping support");
>>>>>>     		break;
>>>>>>     	case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
>>>>>> +		if (!test_kvm_facility(kvm, 76))
>>>>>> +			return -EINVAL;
>>>>>>     		kvm->arch.crypto.dea_kw = 0;
>>>>>>     		memset(kvm->arch.crypto.crycb->dea_wrapping_key_mask, 0,
>>>>>>     			sizeof(kvm->arch.crypto.crycb->dea_wrapping_key_mask));
>>>>>>     		VM_EVENT(kvm, 3, "%s", "DISABLE: DEA keywrapping support");
>>>>>>     		break;
>>>>>> +	case KVM_S390_VM_CRYPTO_ENABLE_APIE:
>>>>>> +		if (!ap_instructions_available()) {
>>>>>> +			mutex_unlock(&kvm->lock);
>>>>>> +			return -EOPNOTSUPP;
>>>>>> +		}
>>>>>> +		kvm->arch.crypto.apie = 1;
>>>>>> +		break;
>>>>>> +	case KVM_S390_VM_CRYPTO_DISABLE_APIE:
>>>>>> +		kvm->arch.crypto.apie = 0;
>>>>>> +		break;
>>>>>>     	default:
>>>>>>     		mutex_unlock(&kvm->lock);
>>>>>>     		return -ENXIO;
>>>>>> @@ -1493,6 +1508,8 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>>>>>>     		case KVM_S390_VM_CRYPTO_ENABLE_DEA_KW:
>>>>>>     		case KVM_S390_VM_CRYPTO_DISABLE_AES_KW:
>>>>>>     		case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
>>>>>> +		case KVM_S390_VM_CRYPTO_ENABLE_APIE:
>>>>>> +		case KVM_S390_VM_CRYPTO_DISABLE_APIE:
>>>>>
>>>>> As also replied to the QEMU series, could we indicate
>>>>> KVM_S390_VM_CRYPTO_ENABLE_APIE (and maybe
>>>>> KVM_S390_VM_CRYPTO_DISABLE_APIE) only with ap_instructions_available(),
>>>>> so we can avoid the additional KVM_S390_VM_CPU_FEAT_AP?
>>>>>
>>>>> KVM_S390_VM_CPU_FEAT_AP is right now completely unused in KVM otherwise
>>>>> (never checked, we only care about apie).
>>>>
>>>> After much discussion with Halil and a few exchanges with you, we
>>>> decided to go ahead and accept your suggestion to get rid of
>>>> KVM_S390_VM_CPU_FEAT and keep the VM device attributes to enable/disable
>>>> apie.
>>>>
>>>> To that end, I responded to patches 03/26, 11/26 and 25/26 with fixup!
>>>> patches that show the KVM/kernel changes that will be necessary to get
>>>> rid of KVM_S390_VM_CPU_FEAT and use apie to control ECA.28. I did that
>>>> to generate discussion in v10 rather than waiting until v11 for
>>>> comments. I make no guarantees that those fixup! patches will
>>>> successfully apply should you have a v10 branch generated from this
>>>> patch series you want to update.
>>>>
>>>
>>> Will you also fixup this patch to expose KVM_S390_VM_CRYPTO_ENABLE_APIE
>>> only if supported by HW? (ap_instructions_available)
>>
>> Given that this patch DOES expose KVM_S390_VM_CRYPTO_ENABLE_APIE only if
>> supported by HW, I assume you are talking about
>> KVM_S390_VM_CRYPTO_DISABLE_APIE. I didn't check
>> ap_instructions_available() for disabling APIE because I didn't
>> think it necessary given that ECA.28 will be set to 0 (intercept) by
>> default, whether AP instructions are installed or not; so why not allow
>> disabling apie. I suppose from the perspective of consistency, since the
>> kvm_s390_vm_has_attr() function checks ap_instructions_available() for
>> both attributes, then it probably makes sense to add that check to
>> KVM_S390_VM_CRYPTO_DISABLE_APIE here. Then again, we could make a change
>> in ap_instructions_available() to allow KVM_S390_VM_CRYPTO_DISABLE_APIE
>> regardless of whether AP instructions are available. It boils down to
>> whether APIE needs to be dynamically disabled at some point when it has
>> been enabled. The only case I can think of where that may be necessary
>> is if a guest is migrated to a system without AP instructions. I don't
>> think that can happen and may even be protected against precisely
>> because the VM attributes won't be available on the target system due to
>> no AP instructions. What say you?
>>
>>>
>>
> 
> Just so we're on the same page, I am talking about exposing, I talk
> about indicating the attribute:
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 03c23045527f..40924fe05bdf 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -1491,6 +1491,11 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm,
> struct kvm_device_attr *attr)
>                  case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
>                          ret = 0;
>                          break;
> +               case KVM_S390_VM_CRYPTO_ENABLE_APIE:
> +               case KVM_S390_VM_CRYPTO_DISABLE_APIE:
> +                       ret = -ENXIO;
> +                       if (ap_instructions_available())
> +                               ret = 0;
>                  default:
>                          ret = -ENXIO;
>                          break;
> 
> KVM_S390_VM_CRYPTO_DISABLE_APIE can either be handled like
> KVM_S390_VM_CRYPTO_ENABLE_APIE (return -EOPNOTSUPP) when setting or
> always be allowed. I'll leave that up to you. But as it is completely
> useless without ap_instructions_available() /
> KVM_S390_VM_CRYPTO_ENABLE_APIE , we might as well also just not expose
> it then.

We are on the same page.

>

Patch

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index b32bd1b..36d3531 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -719,6 +719,7 @@  struct kvm_s390_crypto {
 	__u32 crycbd;
 	__u8 aes_kw;
 	__u8 dea_kw;
+	__u8 apie;
 };
 
 #define APCB0_MASK_SIZE 1
diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
index 8c23afc..a8dbd90 100644
--- a/arch/s390/include/uapi/asm/kvm.h
+++ b/arch/s390/include/uapi/asm/kvm.h
@@ -161,6 +161,8 @@  struct kvm_s390_vm_cpu_subfunc {
 #define KVM_S390_VM_CRYPTO_ENABLE_DEA_KW	1
 #define KVM_S390_VM_CRYPTO_DISABLE_AES_KW	2
 #define KVM_S390_VM_CRYPTO_DISABLE_DEA_KW	3
+#define KVM_S390_VM_CRYPTO_ENABLE_APIE		4
+#define KVM_S390_VM_CRYPTO_DISABLE_APIE		5
 
 /* kvm attributes for migration mode */
 #define KVM_S390_VM_MIGRATION_STOP	0
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 2cdd980..286c2e0 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -856,12 +856,11 @@  void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm)
 
 static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
 {
-	if (!test_kvm_facility(kvm, 76))
-		return -EINVAL;
-
 	mutex_lock(&kvm->lock);
 	switch (attr->attr) {
 	case KVM_S390_VM_CRYPTO_ENABLE_AES_KW:
+		if (!test_kvm_facility(kvm, 76))
+			return -EINVAL;
 		get_random_bytes(
 			kvm->arch.crypto.crycb->aes_wrapping_key_mask,
 			sizeof(kvm->arch.crypto.crycb->aes_wrapping_key_mask));
@@ -869,6 +868,8 @@  static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
 		VM_EVENT(kvm, 3, "%s", "ENABLE: AES keywrapping support");
 		break;
 	case KVM_S390_VM_CRYPTO_ENABLE_DEA_KW:
+		if (!test_kvm_facility(kvm, 76))
+			return -EINVAL;
 		get_random_bytes(
 			kvm->arch.crypto.crycb->dea_wrapping_key_mask,
 			sizeof(kvm->arch.crypto.crycb->dea_wrapping_key_mask));
@@ -876,17 +877,31 @@  static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
 		VM_EVENT(kvm, 3, "%s", "ENABLE: DEA keywrapping support");
 		break;
 	case KVM_S390_VM_CRYPTO_DISABLE_AES_KW:
+		if (!test_kvm_facility(kvm, 76))
+			return -EINVAL;
 		kvm->arch.crypto.aes_kw = 0;
 		memset(kvm->arch.crypto.crycb->aes_wrapping_key_mask, 0,
 			sizeof(kvm->arch.crypto.crycb->aes_wrapping_key_mask));
 		VM_EVENT(kvm, 3, "%s", "DISABLE: AES keywrapping support");
 		break;
 	case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
+		if (!test_kvm_facility(kvm, 76))
+			return -EINVAL;
 		kvm->arch.crypto.dea_kw = 0;
 		memset(kvm->arch.crypto.crycb->dea_wrapping_key_mask, 0,
 			sizeof(kvm->arch.crypto.crycb->dea_wrapping_key_mask));
 		VM_EVENT(kvm, 3, "%s", "DISABLE: DEA keywrapping support");
 		break;
+	case KVM_S390_VM_CRYPTO_ENABLE_APIE:
+		if (!ap_instructions_available()) {
+			mutex_unlock(&kvm->lock);
+			return -EOPNOTSUPP;
+		}
+		kvm->arch.crypto.apie = 1;
+		break;
+	case KVM_S390_VM_CRYPTO_DISABLE_APIE:
+		kvm->arch.crypto.apie = 0;
+		break;
 	default:
 		mutex_unlock(&kvm->lock);
 		return -ENXIO;
@@ -1493,6 +1508,8 @@  static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
 		case KVM_S390_VM_CRYPTO_ENABLE_DEA_KW:
 		case KVM_S390_VM_CRYPTO_DISABLE_AES_KW:
 		case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW:
+		case KVM_S390_VM_CRYPTO_ENABLE_APIE:
+		case KVM_S390_VM_CRYPTO_DISABLE_APIE:
 			ret = 0;
 			break;
 		default:
@@ -2062,6 +2079,7 @@  static u64 kvm_s390_get_initial_cpuid(void)
 static void kvm_s390_crypto_init(struct kvm *kvm)
 {
 	kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
+	kvm->arch.crypto.apie = 0;
 	kvm_s390_set_crycb_format(kvm);
 
 	if (!test_kvm_facility(kvm, 76))
@@ -2602,8 +2620,9 @@  static void kvm_s390_vcpu_crypto_setup(struct kvm_vcpu *vcpu)
 
 	vcpu->arch.sie_block->crycbd = vcpu->kvm->arch.crypto.crycbd;
 	vcpu->arch.sie_block->ecb3 &= ~(ECB3_AES | ECB3_DEA);
+	vcpu->arch.sie_block->eca &= ~ECA_APIE;
 
-	if (test_kvm_cpu_feat(vcpu->kvm, KVM_S390_VM_CPU_FEAT_AP))
+	if (vcpu->kvm->arch.crypto.apie)
 		vcpu->arch.sie_block->eca |= ECA_APIE;
 
 	/* Set up protected key support */