[PATCH] s390: vfio-ap: trace the update of APCB masks

[PATCH] s390: vfio-ap: trace the update of APCB masks

[Pierre Morel]

Define a tracing function to trace in the KVM trace buffer
and trace the changes of the APCB masks.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c     | 22 ++++++++++++++++++++++
 drivers/s390/crypto/vfio_ap_private.h |  4 ++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index a8a9984..00e632a 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -753,6 +753,28 @@ static void vfio_ap_mdev_copy_masks(struct ap_matrix_mdev *matrix_mdev)
 	memcpy(aqm, matrix_mdev->matrix.aqm, nbytes);
 	nbytes = DIV_ROUND_UP(matrix_mdev->matrix.adm_max + 1, BITS_PER_BYTE);
 	memcpy(adm, matrix_mdev->matrix.adm, nbytes);
+
+	switch (matrix_mdev->kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
+	case CRYCB_FORMAT2:
+		VM_AP_EVENT(matrix_mdev->kvm, 3,
+			    "SET CRYCB: apm %016lx %016lx %016lx %016lx",
+			    apm[0], apm[1], apm[2], apm[3]);
+		VM_AP_EVENT(matrix_mdev->kvm, 3,
+			    "SET CRYCB: aqm %016lx %016lx %016lx %016lx",
+			    aqm[0], aqm[1], aqm[2], aqm[3]);
+		VM_AP_EVENT(matrix_mdev->kvm, 3,
+			    "SET CRYCB: adm %016lx %016lx %016lx %016lx",
+			    adm[0], adm[1], adm[2], adm[3]);
+		break;
+	case CRYCB_FORMAT1:
+	case CRYCB_FORMAT0: /* Both CRYCB format uses APCB format 0 */
+	default: /* Can not happen */
+		VM_AP_EVENT(matrix_mdev->kvm, 3,
+			    "SET CRYCB: apm %016lx aqm %04x adm %04x", apm[0],
+			    *((unsigned short *)aqm), *((unsigned short *)adm));
+		break;
+	}
+
 }
 
 /**
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 5675492..4cd779d 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -19,6 +19,10 @@
 
 #include "ap_bus.h"
 
+#define VM_AP_EVENT(d_kvm, d_loglevel, d_string, d_args...)\
+	debug_sprintf_event(d_kvm->arch.dbf, d_loglevel, d_string "\n", \
+			    d_args)
+
 #define VFIO_AP_MODULE_NAME "vfio_ap"
 #define VFIO_AP_DRV_NAME "vfio_ap"
 
-- 
2.7.4

Re: [PATCH] s390: vfio-ap: trace the update of APCB masks

[Cornelia Huck]

On Fri,  5 Oct 2018 09:33:01 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> Define a tracing function to trace in the KVM trace buffer
> and trace the changes of the APCB masks.

In general, trace events are good :)

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  drivers/s390/crypto/vfio_ap_ops.c     | 22 ++++++++++++++++++++++
>  drivers/s390/crypto/vfio_ap_private.h |  4 ++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index a8a9984..00e632a 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -753,6 +753,28 @@ static void vfio_ap_mdev_copy_masks(struct ap_matrix_mdev *matrix_mdev)
>  	memcpy(aqm, matrix_mdev->matrix.aqm, nbytes);
>  	nbytes = DIV_ROUND_UP(matrix_mdev->matrix.adm_max + 1, BITS_PER_BYTE);
>  	memcpy(adm, matrix_mdev->matrix.adm, nbytes);
> +
> +	switch (matrix_mdev->kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
> +	case CRYCB_FORMAT2:
> +		VM_AP_EVENT(matrix_mdev->kvm, 3,
> +			    "SET CRYCB: apm %016lx %016lx %016lx %016lx",
> +			    apm[0], apm[1], apm[2], apm[3]);
> +		VM_AP_EVENT(matrix_mdev->kvm, 3,
> +			    "SET CRYCB: aqm %016lx %016lx %016lx %016lx",
> +			    aqm[0], aqm[1], aqm[2], aqm[3]);
> +		VM_AP_EVENT(matrix_mdev->kvm, 3,
> +			    "SET CRYCB: adm %016lx %016lx %016lx %016lx",
> +			    adm[0], adm[1], adm[2], adm[3]);
> +		break;
> +	case CRYCB_FORMAT1:
> +	case CRYCB_FORMAT0: /* Both CRYCB format uses APCB format 0 */
> +	default: /* Can not happen */

If it can't happen, trace "SET CRYCB: invalid format '%x'" instead?

> +		VM_AP_EVENT(matrix_mdev->kvm, 3,
> +			    "SET CRYCB: apm %016lx aqm %04x adm %04x", apm[0],
> +			    *((unsigned short *)aqm), *((unsigned short *)adm));
> +		break;
> +	}
> +
>  }
>  
>  /**
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 5675492..4cd779d 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -19,6 +19,10 @@
>  
>  #include "ap_bus.h"
>  
> +#define VM_AP_EVENT(d_kvm, d_loglevel, d_string, d_args...)\
> +	debug_sprintf_event(d_kvm->arch.dbf, d_loglevel, d_string "\n", \
> +			    d_args)
> +
>  #define VFIO_AP_MODULE_NAME "vfio_ap"
>  #define VFIO_AP_DRV_NAME "vfio_ap"
>  

One thing I don't like about this is that you're using the kvm dbf from
driver code, which looks odd. OTOH, these are kvm-specific masks...

Is there anything else that you may want to trace in the vfio-ap
driver, so that you could add a dbf for it?

Re: [PATCH] s390: vfio-ap: trace the update of APCB masks

[David Hildenbrand]

On 05/10/2018 09:50, Cornelia Huck wrote:
> On Fri,  5 Oct 2018 09:33:01 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> Define a tracing function to trace in the KVM trace buffer
>> and trace the changes of the APCB masks.
> 
> In general, trace events are good :)
> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>  drivers/s390/crypto/vfio_ap_ops.c     | 22 ++++++++++++++++++++++
>>  drivers/s390/crypto/vfio_ap_private.h |  4 ++++
>>  2 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index a8a9984..00e632a 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -753,6 +753,28 @@ static void vfio_ap_mdev_copy_masks(struct ap_matrix_mdev *matrix_mdev)
>>  	memcpy(aqm, matrix_mdev->matrix.aqm, nbytes);
>>  	nbytes = DIV_ROUND_UP(matrix_mdev->matrix.adm_max + 1, BITS_PER_BYTE);
>>  	memcpy(adm, matrix_mdev->matrix.adm, nbytes);
>> +
>> +	switch (matrix_mdev->kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
>> +	case CRYCB_FORMAT2:
>> +		VM_AP_EVENT(matrix_mdev->kvm, 3,
>> +			    "SET CRYCB: apm %016lx %016lx %016lx %016lx",
>> +			    apm[0], apm[1], apm[2], apm[3]);
>> +		VM_AP_EVENT(matrix_mdev->kvm, 3,
>> +			    "SET CRYCB: aqm %016lx %016lx %016lx %016lx",
>> +			    aqm[0], aqm[1], aqm[2], aqm[3]);
>> +		VM_AP_EVENT(matrix_mdev->kvm, 3,
>> +			    "SET CRYCB: adm %016lx %016lx %016lx %016lx",
>> +			    adm[0], adm[1], adm[2], adm[3]);
>> +		break;
>> +	case CRYCB_FORMAT1:
>> +	case CRYCB_FORMAT0: /* Both CRYCB format uses APCB format 0 */
>> +	default: /* Can not happen */
> 
> If it can't happen, trace "SET CRYCB: invalid format '%x'" instead?
> 
>> +		VM_AP_EVENT(matrix_mdev->kvm, 3,
>> +			    "SET CRYCB: apm %016lx aqm %04x adm %04x", apm[0],
>> +			    *((unsigned short *)aqm), *((unsigned short *)adm));
>> +		break;
>> +	}
>> +
>>  }
>>  
>>  /**
>> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
>> index 5675492..4cd779d 100644
>> --- a/drivers/s390/crypto/vfio_ap_private.h
>> +++ b/drivers/s390/crypto/vfio_ap_private.h
>> @@ -19,6 +19,10 @@
>>  
>>  #include "ap_bus.h"
>>  
>> +#define VM_AP_EVENT(d_kvm, d_loglevel, d_string, d_args...)\
>> +	debug_sprintf_event(d_kvm->arch.dbf, d_loglevel, d_string "\n", \
>> +			    d_args)
>> +
>>  #define VFIO_AP_MODULE_NAME "vfio_ap"
>>  #define VFIO_AP_DRV_NAME "vfio_ap"
>>  
> 
> One thing I don't like about this is that you're using the kvm dbf from
> driver code, which looks odd. OTOH, these are kvm-specific masks...

Maybe factor out the setting of the actual masks to KVM code and do
tracing there?

> 
> Is there anything else that you may want to trace in the vfio-ap
> driver, so that you could add a dbf for it?
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH] s390: vfio-ap: trace the update of APCB masks

[Christian Borntraeger]

On 10/05/2018 09:54 AM, David Hildenbrand wrote:
> On 05/10/2018 09:50, Cornelia Huck wrote:
>> On Fri,  5 Oct 2018 09:33:01 +0200
>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>
>>> Define a tracing function to trace in the KVM trace buffer
>>> and trace the changes of the APCB masks.
>>
>> In general, trace events are good :)
>>
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>  drivers/s390/crypto/vfio_ap_ops.c     | 22 ++++++++++++++++++++++
>>>  drivers/s390/crypto/vfio_ap_private.h |  4 ++++
>>>  2 files changed, 26 insertions(+)
>>>
>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>>> index a8a9984..00e632a 100644
>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>>> @@ -753,6 +753,28 @@ static void vfio_ap_mdev_copy_masks(struct ap_matrix_mdev *matrix_mdev)
>>>  	memcpy(aqm, matrix_mdev->matrix.aqm, nbytes);
>>>  	nbytes = DIV_ROUND_UP(matrix_mdev->matrix.adm_max + 1, BITS_PER_BYTE);
>>>  	memcpy(adm, matrix_mdev->matrix.adm, nbytes);
>>> +
>>> +	switch (matrix_mdev->kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
>>> +	case CRYCB_FORMAT2:
>>> +		VM_AP_EVENT(matrix_mdev->kvm, 3,
>>> +			    "SET CRYCB: apm %016lx %016lx %016lx %016lx",
>>> +			    apm[0], apm[1], apm[2], apm[3]);
>>> +		VM_AP_EVENT(matrix_mdev->kvm, 3,
>>> +			    "SET CRYCB: aqm %016lx %016lx %016lx %016lx",
>>> +			    aqm[0], aqm[1], aqm[2], aqm[3]);
>>> +		VM_AP_EVENT(matrix_mdev->kvm, 3,
>>> +			    "SET CRYCB: adm %016lx %016lx %016lx %016lx",
>>> +			    adm[0], adm[1], adm[2], adm[3]);
>>> +		break;
>>> +	case CRYCB_FORMAT1:
>>> +	case CRYCB_FORMAT0: /* Both CRYCB format uses APCB format 0 */
>>> +	default: /* Can not happen */
>>
>> If it can't happen, trace "SET CRYCB: invalid format '%x'" instead?
>>
>>> +		VM_AP_EVENT(matrix_mdev->kvm, 3,
>>> +			    "SET CRYCB: apm %016lx aqm %04x adm %04x", apm[0],
>>> +			    *((unsigned short *)aqm), *((unsigned short *)adm));
>>> +		break;
>>> +	}
>>> +
>>>  }
>>>  
>>>  /**
>>> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
>>> index 5675492..4cd779d 100644
>>> --- a/drivers/s390/crypto/vfio_ap_private.h
>>> +++ b/drivers/s390/crypto/vfio_ap_private.h
>>> @@ -19,6 +19,10 @@
>>>  
>>>  #include "ap_bus.h"
>>>  
>>> +#define VM_AP_EVENT(d_kvm, d_loglevel, d_string, d_args...)\
>>> +	debug_sprintf_event(d_kvm->arch.dbf, d_loglevel, d_string "\n", \
>>> +			    d_args)
>>> +
>>>  #define VFIO_AP_MODULE_NAME "vfio_ap"
>>>  #define VFIO_AP_DRV_NAME "vfio_ap"
>>>  
>>
>> One thing I don't like about this is that you're using the kvm dbf from
>> driver code, which looks odd. OTOH, these are kvm-specific masks...
> 
> Maybe factor out the setting of the actual masks to KVM code and do
> tracing there?

We had both versions internally, and this version looked shorter. But Pierre,
can you send your v1 as well and then we can compare both variants.
> 
>>
>> Is there anything else that you may want to trace in the vfio-ap
>> driver, so that you could add a dbf for it?
>>
> 
> 

[PATCH v1 0/2] KVM: s390: Tracing APCB changes

[Pierre Morel]

In the first patch we define kvm_arch_crypto_set_masks,
a new function to centralize the setup the APCB masks
inside the CRYCB SIE satelite and add KVM_EVENT() to
kvm_arch_crypto_set_masks and kvm_arch_crypto_clear_masks.

In the second patch we replace the vfio_ap_mdev_copy_masks()
by the new kvm_arch_crypto_set_masks() function.


Pierre Morel (2):
  KVM: s390: Tracing APCB changes
  s390: vfio-ap: setup APCB mask using KVM dedicated function

 arch/s390/include/asm/kvm_host.h  |  2 ++
 arch/s390/kvm/kvm-s390.c          | 41 +++++++++++++++++++++++++++++++++++++++
 drivers/s390/crypto/vfio_ap_ops.c | 33 +++----------------------------
 3 files changed, 46 insertions(+), 30 deletions(-)

-- 
2.7.4

[PATCH v1 1/2] KVM: s390: Tracing APCB changes

[Pierre Morel]

kvm_arch_crypto_set_masks is a new function to centralize
the setup the APCB masks inside the CRYCB SIE satelite.

To trace APCB mask changes, we add KVM_EVENT() tracing to
both kvm_arch_crypto_set_masks and kvm_arch_crypto_clear_masks.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 arch/s390/include/asm/kvm_host.h |  2 ++
 arch/s390/kvm/kvm-s390.c         | 41 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 36d3531..22aa4da 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -861,6 +861,8 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
 				 struct kvm_async_pf *work);
 
 void kvm_arch_crypto_clear_masks(struct kvm *kvm);
+void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
+			       unsigned long *aqm, unsigned long *adm);
 
 extern int sie64a(struct kvm_s390_sie_block *, u64 *);
 extern char sie_exit;
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index d3ea721..e871e5a 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2057,6 +2057,46 @@ static void kvm_s390_set_crycb_format(struct kvm *kvm)
 		kvm->arch.crypto.crycbd |= CRYCB_FORMAT1;
 }
 
+void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
+			       unsigned long *aqm, unsigned long *adm)
+{
+	struct kvm_s390_crypto_cb *crycb = kvm->arch.crypto.crycb;
+
+	mutex_lock(&kvm->lock);
+	kvm_s390_vcpu_block_all(kvm);
+
+	switch (kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
+	case CRYCB_FORMAT2: /* APCB1 use 256 bits */
+		memcpy(crycb->apcb1.apm, apm, 32);
+		VM_EVENT(kvm, 3, "SET CRYCB: apm %016lx %016lx %016lx %016lx",
+			 apm[0], apm[1], apm[2], apm[3]);
+		memcpy(crycb->apcb1.aqm, aqm, 32);
+		VM_EVENT(kvm, 3, "SET CRYCB: aqm %016lx %016lx %016lx %016lx",
+			 aqm[0], aqm[1], aqm[2], aqm[3]);
+		memcpy(crycb->apcb1.adm, adm, 32);
+		VM_EVENT(kvm, 3, "SET CRYCB: adm %016lx %016lx %016lx %016lx",
+			 adm[0], adm[1], adm[2], adm[3]);
+		break;
+	case CRYCB_FORMAT1:
+	case CRYCB_FORMAT0: /* Fall through both use APCB0 */
+		memcpy(crycb->apcb0.apm, apm, 8);
+		memcpy(crycb->apcb0.aqm, aqm, 2);
+		memcpy(crycb->apcb0.adm, adm, 2);
+		VM_EVENT(kvm, 3, "SET CRYCB: apm %016lx aqm %04x adm %04x",
+			 apm[0], *((unsigned short *)aqm),
+			 *((unsigned short *)adm));
+		break;
+	default:	/* Can not happen */
+		break;
+	}
+
+	/* recreate the shadow crycb for each vcpu */
+	kvm_s390_sync_request_broadcast(kvm, KVM_REQ_VSIE_RESTART);
+	kvm_s390_vcpu_unblock_all(kvm);
+	mutex_unlock(&kvm->lock);
+}
+EXPORT_SYMBOL_GPL(kvm_arch_crypto_set_masks);
+
 void kvm_arch_crypto_clear_masks(struct kvm *kvm)
 {
 	mutex_lock(&kvm->lock);
@@ -2067,6 +2107,7 @@ void kvm_arch_crypto_clear_masks(struct kvm *kvm)
 	memset(&kvm->arch.crypto.crycb->apcb1, 0,
 	       sizeof(kvm->arch.crypto.crycb->apcb1));
 
+	VM_EVENT(kvm, 3, "%s", "CLR CRYCB:");
 	/* recreate the shadow crycb for each vcpu */
 	kvm_s390_sync_request_broadcast(kvm, KVM_REQ_VSIE_RESTART);
 	kvm_s390_vcpu_unblock_all(kvm);
-- 
2.7.4

[PATCH v1 2/2] s390: vfio-ap: setup APCB mask using KVM dedicated function

[Pierre Morel]

We replace the vfio_ap_mdev_copy_masks() by the new
kvm_arch_crypto_set_masks() to be able to use the standard
KVM tracing system.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 33 +++------------------------------
 1 file changed, 3 insertions(+), 30 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index a8a9984..f297779 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -726,35 +726,6 @@ static const struct attribute_group *vfio_ap_mdev_attr_groups[] = {
 	NULL
 };
 
-static void vfio_ap_mdev_copy_masks(struct ap_matrix_mdev *matrix_mdev)
-{
-	int nbytes;
-	unsigned long *apm, *aqm, *adm;
-	struct kvm_s390_crypto_cb *crycb = matrix_mdev->kvm->arch.crypto.crycb;
-
-	switch (matrix_mdev->kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
-	case CRYCB_FORMAT2:
-		apm = (unsigned long *)crycb->apcb1.apm;
-		aqm = (unsigned long *)crycb->apcb1.aqm;
-		adm = (unsigned long *)crycb->apcb1.adm;
-		break;
-	case CRYCB_FORMAT1:
-	case CRYCB_FORMAT0:
-	default:
-		apm = (unsigned long *)crycb->apcb0.apm;
-		aqm = (unsigned long *)crycb->apcb0.aqm;
-		adm = (unsigned long *)crycb->apcb0.adm;
-		break;
-	}
-
-	nbytes = DIV_ROUND_UP(matrix_mdev->matrix.apm_max + 1, BITS_PER_BYTE);
-	memcpy(apm, matrix_mdev->matrix.apm, nbytes);
-	nbytes = DIV_ROUND_UP(matrix_mdev->matrix.aqm_max + 1, BITS_PER_BYTE);
-	memcpy(aqm, matrix_mdev->matrix.aqm, nbytes);
-	nbytes = DIV_ROUND_UP(matrix_mdev->matrix.adm_max + 1, BITS_PER_BYTE);
-	memcpy(adm, matrix_mdev->matrix.adm, nbytes);
-}
-
 /**
  * vfio_ap_mdev_set_kvm
  *
@@ -811,7 +782,9 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
 	if (!matrix_mdev->kvm->arch.crypto.crycbd)
 		return NOTIFY_DONE;
 
-	vfio_ap_mdev_copy_masks(matrix_mdev);
+	kvm_arch_crypto_set_masks(matrix_mdev->kvm, matrix_mdev->matrix.apm,
+				  matrix_mdev->matrix.aqm,
+				  matrix_mdev->matrix.adm);
 
 	return NOTIFY_OK;
 }
-- 
2.7.4

Re: [PATCH v1 0/2] KVM: s390: Tracing APCB changes

[Cornelia Huck]

On Fri,  5 Oct 2018 10:31:08 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> In the first patch we define kvm_arch_crypto_set_masks,
> a new function to centralize the setup the APCB masks
> inside the CRYCB SIE satelite and add KVM_EVENT() to
> kvm_arch_crypto_set_masks and kvm_arch_crypto_clear_masks.
> 
> In the second patch we replace the vfio_ap_mdev_copy_masks()
> by the new kvm_arch_crypto_set_masks() function.
> 
> 
> Pierre Morel (2):
>   KVM: s390: Tracing APCB changes
>   s390: vfio-ap: setup APCB mask using KVM dedicated function
> 
>  arch/s390/include/asm/kvm_host.h  |  2 ++
>  arch/s390/kvm/kvm-s390.c          | 41 +++++++++++++++++++++++++++++++++++++++
>  drivers/s390/crypto/vfio_ap_ops.c | 33 +++----------------------------
>  3 files changed, 46 insertions(+), 30 deletions(-)
> 

I like this version better: it moves the mask-setting next to the
mask-clearing, and, as a bonus, adds a trace event for clearing as
well :)

Re: [PATCH v1 1/2] KVM: s390: Tracing APCB changes

[Cornelia Huck]

On Fri,  5 Oct 2018 10:31:09 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> kvm_arch_crypto_set_masks is a new function to centralize
> the setup the APCB masks inside the CRYCB SIE satelite.

s/satelite/satellite/

> 
> To trace APCB mask changes, we add KVM_EVENT() tracing to
> both kvm_arch_crypto_set_masks and kvm_arch_crypto_clear_masks.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h |  2 ++
>  arch/s390/kvm/kvm-s390.c         | 41 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 36d3531..22aa4da 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -861,6 +861,8 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
>  				 struct kvm_async_pf *work);
>  
>  void kvm_arch_crypto_clear_masks(struct kvm *kvm);
> +void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
> +			       unsigned long *aqm, unsigned long *adm);
>  
>  extern int sie64a(struct kvm_s390_sie_block *, u64 *);
>  extern char sie_exit;
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index d3ea721..e871e5a 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2057,6 +2057,46 @@ static void kvm_s390_set_crycb_format(struct kvm *kvm)
>  		kvm->arch.crypto.crycbd |= CRYCB_FORMAT1;
>  }
>  
> +void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
> +			       unsigned long *aqm, unsigned long *adm)
> +{
> +	struct kvm_s390_crypto_cb *crycb = kvm->arch.crypto.crycb;
> +
> +	mutex_lock(&kvm->lock);
> +	kvm_s390_vcpu_block_all(kvm);
> +
> +	switch (kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
> +	case CRYCB_FORMAT2: /* APCB1 use 256 bits */
> +		memcpy(crycb->apcb1.apm, apm, 32);
> +		VM_EVENT(kvm, 3, "SET CRYCB: apm %016lx %016lx %016lx %016lx",
> +			 apm[0], apm[1], apm[2], apm[3]);
> +		memcpy(crycb->apcb1.aqm, aqm, 32);
> +		VM_EVENT(kvm, 3, "SET CRYCB: aqm %016lx %016lx %016lx %016lx",
> +			 aqm[0], aqm[1], aqm[2], aqm[3]);
> +		memcpy(crycb->apcb1.adm, adm, 32);
> +		VM_EVENT(kvm, 3, "SET CRYCB: adm %016lx %016lx %016lx %016lx",
> +			 adm[0], adm[1], adm[2], adm[3]);
> +		break;
> +	case CRYCB_FORMAT1:
> +	case CRYCB_FORMAT0: /* Fall through both use APCB0 */
> +		memcpy(crycb->apcb0.apm, apm, 8);
> +		memcpy(crycb->apcb0.aqm, aqm, 2);
> +		memcpy(crycb->apcb0.adm, adm, 2);
> +		VM_EVENT(kvm, 3, "SET CRYCB: apm %016lx aqm %04x adm %04x",
> +			 apm[0], *((unsigned short *)aqm),
> +			 *((unsigned short *)adm));
> +		break;
> +	default:	/* Can not happen */
> +		break;
> +	}
> +
> +	/* recreate the shadow crycb for each vcpu */
> +	kvm_s390_sync_request_broadcast(kvm, KVM_REQ_VSIE_RESTART);
> +	kvm_s390_vcpu_unblock_all(kvm);
> +	mutex_unlock(&kvm->lock);


The locking and requests makes me wonder if we missed them before...
were they simply not needed for the prior use case (mdev group
notifier)?

> +}
> +EXPORT_SYMBOL_GPL(kvm_arch_crypto_set_masks);
> +
>  void kvm_arch_crypto_clear_masks(struct kvm *kvm)
>  {
>  	mutex_lock(&kvm->lock);
> @@ -2067,6 +2107,7 @@ void kvm_arch_crypto_clear_masks(struct kvm *kvm)
>  	memset(&kvm->arch.crypto.crycb->apcb1, 0,
>  	       sizeof(kvm->arch.crypto.crycb->apcb1));
>  
> +	VM_EVENT(kvm, 3, "%s", "CLR CRYCB:");
>  	/* recreate the shadow crycb for each vcpu */
>  	kvm_s390_sync_request_broadcast(kvm, KVM_REQ_VSIE_RESTART);
>  	kvm_s390_vcpu_unblock_all(kvm);

Re: [PATCH v1 0/2] KVM: s390: Tracing APCB changes

[David Hildenbrand]

On 05/10/2018 10:44, Cornelia Huck wrote:
> On Fri,  5 Oct 2018 10:31:08 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> In the first patch we define kvm_arch_crypto_set_masks,
>> a new function to centralize the setup the APCB masks
>> inside the CRYCB SIE satelite and add KVM_EVENT() to
>> kvm_arch_crypto_set_masks and kvm_arch_crypto_clear_masks.
>>
>> In the second patch we replace the vfio_ap_mdev_copy_masks()
>> by the new kvm_arch_crypto_set_masks() function.
>>
>>
>> Pierre Morel (2):
>>   KVM: s390: Tracing APCB changes
>>   s390: vfio-ap: setup APCB mask using KVM dedicated function
>>
>>  arch/s390/include/asm/kvm_host.h  |  2 ++
>>  arch/s390/kvm/kvm-s390.c          | 41 +++++++++++++++++++++++++++++++++++++++
>>  drivers/s390/crypto/vfio_ap_ops.c | 33 +++----------------------------
>>  3 files changed, 46 insertions(+), 30 deletions(-)
>>
> 
> I like this version better: it moves the mask-setting next to the
> mask-clearing, and, as a bonus, adds a trace event for clearing as
> well :)
> 

Yes, more code but looks cleaner to me.

-- 

Thanks,

David / dhildenb

Re: [PATCH v1 1/2] KVM: s390: Tracing APCB changes

[Pierre Morel]

On 05/10/2018 10:49, Cornelia Huck wrote:
> On Fri,  5 Oct 2018 10:31:09 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> kvm_arch_crypto_set_masks is a new function to centralize
>> the setup the APCB masks inside the CRYCB SIE satelite.
> 
> s/satelite/satellite/
> 
>>
>> To trace APCB mask changes, we add KVM_EVENT() tracing to
>> both kvm_arch_crypto_set_masks and kvm_arch_crypto_clear_masks.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   arch/s390/include/asm/kvm_host.h |  2 ++
>>   arch/s390/kvm/kvm-s390.c         | 41 ++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 43 insertions(+)
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index 36d3531..22aa4da 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -861,6 +861,8 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
>>   				 struct kvm_async_pf *work);
>>   
>>   void kvm_arch_crypto_clear_masks(struct kvm *kvm);
>> +void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
>> +			       unsigned long *aqm, unsigned long *adm);
>>   
>>   extern int sie64a(struct kvm_s390_sie_block *, u64 *);
>>   extern char sie_exit;
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index d3ea721..e871e5a 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -2057,6 +2057,46 @@ static void kvm_s390_set_crycb_format(struct kvm *kvm)
>>   		kvm->arch.crypto.crycbd |= CRYCB_FORMAT1;
>>   }
>>   
>> +void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
>> +			       unsigned long *aqm, unsigned long *adm)
>> +{
>> +	struct kvm_s390_crypto_cb *crycb = kvm->arch.crypto.crycb;
>> +
>> +	mutex_lock(&kvm->lock);
>> +	kvm_s390_vcpu_block_all(kvm);
>> +
>> +	switch (kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
>> +	case CRYCB_FORMAT2: /* APCB1 use 256 bits */
>> +		memcpy(crycb->apcb1.apm, apm, 32);
>> +		VM_EVENT(kvm, 3, "SET CRYCB: apm %016lx %016lx %016lx %016lx",
>> +			 apm[0], apm[1], apm[2], apm[3]);
>> +		memcpy(crycb->apcb1.aqm, aqm, 32);
>> +		VM_EVENT(kvm, 3, "SET CRYCB: aqm %016lx %016lx %016lx %016lx",
>> +			 aqm[0], aqm[1], aqm[2], aqm[3]);
>> +		memcpy(crycb->apcb1.adm, adm, 32);
>> +		VM_EVENT(kvm, 3, "SET CRYCB: adm %016lx %016lx %016lx %016lx",
>> +			 adm[0], adm[1], adm[2], adm[3]);
>> +		break;
>> +	case CRYCB_FORMAT1:
>> +	case CRYCB_FORMAT0: /* Fall through both use APCB0 */
>> +		memcpy(crycb->apcb0.apm, apm, 8);
>> +		memcpy(crycb->apcb0.aqm, aqm, 2);
>> +		memcpy(crycb->apcb0.adm, adm, 2);
>> +		VM_EVENT(kvm, 3, "SET CRYCB: apm %016lx aqm %04x adm %04x",
>> +			 apm[0], *((unsigned short *)aqm),
>> +			 *((unsigned short *)adm));
>> +		break;
>> +	default:	/* Can not happen */
>> +		break;
>> +	}
>> +
>> +	/* recreate the shadow crycb for each vcpu */
>> +	kvm_s390_sync_request_broadcast(kvm, KVM_REQ_VSIE_RESTART);
>> +	kvm_s390_vcpu_unblock_all(kvm);
>> +	mutex_unlock(&kvm->lock);
> 
> 
> The locking and requests makes me wonder if we missed them before...
> were they simply not needed for the prior use case (mdev group
> notifier)?

Before we used to set the mask before creating the vcpu.
In fact since we still call this function from the initialization
of VFIO AP we still do.

But since this function is more generic we need to be more careful
if it is called when vcpu are running.
We do the same locking mechanism in the kvm_arch_crypto_clear_mask().

> 
>> +}
>> +EXPORT_SYMBOL_GPL(kvm_arch_crypto_set_masks);
>> +
>>   void kvm_arch_crypto_clear_masks(struct kvm *kvm)
>>   {
>>   	mutex_lock(&kvm->lock);
>> @@ -2067,6 +2107,7 @@ void kvm_arch_crypto_clear_masks(struct kvm *kvm)
>>   	memset(&kvm->arch.crypto.crycb->apcb1, 0,
>>   	       sizeof(kvm->arch.crypto.crycb->apcb1));
>>   
>> +	VM_EVENT(kvm, 3, "%s", "CLR CRYCB:");
>>   	/* recreate the shadow crycb for each vcpu */
>>   	kvm_s390_sync_request_broadcast(kvm, KVM_REQ_VSIE_RESTART);
>>   	kvm_s390_vcpu_unblock_all(kvm);
> 


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

Re: [PATCH v1 1/2] KVM: s390: Tracing APCB changes

[Cornelia Huck]

On Fri, 5 Oct 2018 10:57:22 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 05/10/2018 10:49, Cornelia Huck wrote:
> > On Fri,  5 Oct 2018 10:31:09 +0200
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> >   
> >> kvm_arch_crypto_set_masks is a new function to centralize
> >> the setup the APCB masks inside the CRYCB SIE satelite.  
> > 
> > s/satelite/satellite/
> >   
> >>
> >> To trace APCB mask changes, we add KVM_EVENT() tracing to
> >> both kvm_arch_crypto_set_masks and kvm_arch_crypto_clear_masks.
> >>
> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> >> ---
> >>   arch/s390/include/asm/kvm_host.h |  2 ++
> >>   arch/s390/kvm/kvm-s390.c         | 41 ++++++++++++++++++++++++++++++++++++++++
> >>   2 files changed, 43 insertions(+)

(...)

> > The locking and requests makes me wonder if we missed them before...
> > were they simply not needed for the prior use case (mdev group
> > notifier)?  
> 
> Before we used to set the mask before creating the vcpu.
> In fact since we still call this function from the initialization
> of VFIO AP we still do.
> 
> But since this function is more generic we need to be more careful
> if it is called when vcpu are running.
> We do the same locking mechanism in the kvm_arch_crypto_clear_mask().

Yes, that makes sense.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v1 2/2] s390: vfio-ap: setup APCB mask using KVM dedicated function

[Cornelia Huck]

On Fri,  5 Oct 2018 10:31:10 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> We replace the vfio_ap_mdev_copy_masks() by the new
> kvm_arch_crypto_set_masks() to be able to use the standard
> KVM tracing system.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  drivers/s390/crypto/vfio_ap_ops.c | 33 +++------------------------------
>  1 file changed, 3 insertions(+), 30 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v1 2/2] s390: vfio-ap: setup APCB mask using KVM dedicated function

[David Hildenbrand]

On 05/10/2018 10:31, Pierre Morel wrote:
> We replace the vfio_ap_mdev_copy_masks() by the new
> kvm_arch_crypto_set_masks() to be able to use the standard
> KVM tracing system.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  drivers/s390/crypto/vfio_ap_ops.c | 33 +++------------------------------
>  1 file changed, 3 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index a8a9984..f297779 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -726,35 +726,6 @@ static const struct attribute_group *vfio_ap_mdev_attr_groups[] = {
>  	NULL
>  };
>  
> -static void vfio_ap_mdev_copy_masks(struct ap_matrix_mdev *matrix_mdev)
> -{
> -	int nbytes;
> -	unsigned long *apm, *aqm, *adm;
> -	struct kvm_s390_crypto_cb *crycb = matrix_mdev->kvm->arch.crypto.crycb;
> -
> -	switch (matrix_mdev->kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
> -	case CRYCB_FORMAT2:
> -		apm = (unsigned long *)crycb->apcb1.apm;
> -		aqm = (unsigned long *)crycb->apcb1.aqm;
> -		adm = (unsigned long *)crycb->apcb1.adm;
> -		break;
> -	case CRYCB_FORMAT1:
> -	case CRYCB_FORMAT0:
> -	default:
> -		apm = (unsigned long *)crycb->apcb0.apm;
> -		aqm = (unsigned long *)crycb->apcb0.aqm;
> -		adm = (unsigned long *)crycb->apcb0.adm;
> -		break;
> -	}
> -
> -	nbytes = DIV_ROUND_UP(matrix_mdev->matrix.apm_max + 1, BITS_PER_BYTE);
> -	memcpy(apm, matrix_mdev->matrix.apm, nbytes);
> -	nbytes = DIV_ROUND_UP(matrix_mdev->matrix.aqm_max + 1, BITS_PER_BYTE);
> -	memcpy(aqm, matrix_mdev->matrix.aqm, nbytes);
> -	nbytes = DIV_ROUND_UP(matrix_mdev->matrix.adm_max + 1, BITS_PER_BYTE);
> -	memcpy(adm, matrix_mdev->matrix.adm, nbytes);
> -}
> -
>  /**
>   * vfio_ap_mdev_set_kvm
>   *
> @@ -811,7 +782,9 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
>  	if (!matrix_mdev->kvm->arch.crypto.crycbd)
>  		return NOTIFY_DONE;
>  
> -	vfio_ap_mdev_copy_masks(matrix_mdev);
> +	kvm_arch_crypto_set_masks(matrix_mdev->kvm, matrix_mdev->matrix.apm,
> +				  matrix_mdev->matrix.aqm,
> +				  matrix_mdev->matrix.adm);
>  
>  	return NOTIFY_OK;
>  }
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [PATCH v1 0/2] KVM: s390: Tracing APCB changes

[Halil Pasic]

On 10/05/2018 10:31 AM, Pierre Morel wrote:
> In the first patch we define kvm_arch_crypto_set_masks,
> a new function to centralize the setup the APCB masks
> inside the CRYCB SIE satelite and add KVM_EVENT() to
> kvm_arch_crypto_set_masks and kvm_arch_crypto_clear_masks.
> 
> In the second patch we replace the vfio_ap_mdev_copy_masks()
> by the new kvm_arch_crypto_set_masks() function.
> 

FWIW I also like this version better. And accommodating for on-the
fly changes seems to be showing in the right direction anyway.

I did not look into the details but it looks good to me.

Halil

Re: [PATCH v1 0/2] KVM: s390: Tracing APCB changes

[Christian Borntraeger]

Both applied.
(FWIW, this was based on an older version of the ap patch set, I had to fixup the 2 patch)

On 10/05/2018 10:31 AM, Pierre Morel wrote:
> In the first patch we define kvm_arch_crypto_set_masks,
> a new function to centralize the setup the APCB masks
> inside the CRYCB SIE satelite and add KVM_EVENT() to
> kvm_arch_crypto_set_masks and kvm_arch_crypto_clear_masks.
> 
> In the second patch we replace the vfio_ap_mdev_copy_masks()
> by the new kvm_arch_crypto_set_masks() function.
> 
> 
> Pierre Morel (2):
>   KVM: s390: Tracing APCB changes
>   s390: vfio-ap: setup APCB mask using KVM dedicated function
> 
>  arch/s390/include/asm/kvm_host.h  |  2 ++
>  arch/s390/kvm/kvm-s390.c          | 41 +++++++++++++++++++++++++++++++++++++++
>  drivers/s390/crypto/vfio_ap_ops.c | 33 +++----------------------------
>  3 files changed, 46 insertions(+), 30 deletions(-)
>