[v4,6/7] s390: ap: Cleanup on removing the AP device

Commit Message

Pierre Morel Feb. 22, 2019, 3:29 p.m. UTC

When the device is remove, we must make sure to
clear the interruption and reset the AP device.

We also need to clear the CRYCB of the guest.

Signed-off-by: Pierre Morel <[e-mail]>
---
 drivers/s390/crypto/vfio_ap_drv.c     | 35 +++++++++++++++++++++++++++++++++++
 drivers/s390/crypto/vfio_ap_ops.c     |  3 ++-
 drivers/s390/crypto/vfio_ap_private.h |  3 +++
 3 files changed, 40 insertions(+), 1 deletion(-)

Comments

Anthony Krowiak Feb. 26, 2019, 6:27 p.m. UTC | #1

On 2/22/19 10:29 AM, Pierre Morel wrote:
> When the device is remove, we must make sure to
> clear the interruption and reset the AP device.
> 
> We also need to clear the CRYCB of the guest.
> 
> Signed-off-by: Pierre Morel <[e-mail]>
> ---
>   drivers/s390/crypto/vfio_ap_drv.c     | 35 +++++++++++++++++++++++++++++++++++
>   drivers/s390/crypto/vfio_ap_ops.c     |  3 ++-
>   drivers/s390/crypto/vfio_ap_private.h |  3 +++
>   3 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index eca0ffc..e5d91ff 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -5,6 +5,7 @@
>    * Copyright IBM Corp. 2018
>    *
>    * Author(s): Tony Krowiak <[e-mail]>
> + *	      Pierre Morel <[e-mail]>
>    */
>   
>   #include <linux/module.h>
> @@ -12,6 +13,8 @@
>   #include <linux/slab.h>
>   #include <linux/string.h>
>   #include <asm/facility.h>
> +#include <linux/bitops.h>
> +#include <linux/kvm_host.h>
>   #include "vfio_ap_private.h"
>   
>   #define VFIO_AP_ROOT_NAME "vfio_ap"
> @@ -61,6 +64,33 @@ static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
>   }
>   
>   /**
> + * vfio_ap_update_crycb
> + * @q: A pointer to the queue being removed
> + *
> + * We clear the APID of the queue, making this queue unusable for the guest.
> + * After this function we can reset the queue without to fear a race with
> + * the guest to access the queue again.
> + * We do not fear race with the host as we still get the device.
> + */
> +static void vfio_ap_update_crycb(struct vfio_ap_queue *q)
> +{
> +	struct ap_matrix_mdev *matrix_mdev = q->matrix_mdev;
> +
> +	if (!matrix_mdev)
> +		return;
> +
> +	clear_bit_inv(AP_QID_CARD(q->apqn), matrix_mdev->matrix.apm);
> +
> +	if (!matrix_mdev->kvm)
> +		return;
> +
> +	kvm_arch_crypto_set_masks(matrix_mdev->kvm,
> +				  matrix_mdev->matrix.apm,
> +				  matrix_mdev->matrix.aqm,
> +				  matrix_mdev->matrix.adm);
> +}
> +
> +/**
>    * vfio_ap_queue_dev_remove:
>    *
>    * Free the associated vfio_ap_queue structure
> @@ -70,6 +100,11 @@ static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
>   	struct vfio_ap_queue *q;
>   
>   	q = dev_get_drvdata(&apdev->device);
> +	if (!q)
> +		return;
> +
> +	vfio_ap_update_crycb(q);
> +	vfio_ap_mdev_reset_queue(q);

The reset is unnecessary because once the card is removed from the
CRYCB, the ZAPQ may fail with because the queue may not exist anymore.
Besides, once the card is removed from the guest's CRYCB, the bus
running in the guest will do a reset.

>   	list_del(&q->list);
>   	kfree(q);
>   }
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 0196065..5b9bb33 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -59,6 +59,7 @@ int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
>   			if (retry <= 0)
>   				pr_warn("%s: queue 0x%04x not empty\n",
>   					__func__, q->apqn);
> +			vfio_ap_free_irq(q);
>   			return 0;
>   		case AP_RESPONSE_RESET_IN_PROGRESS:
>   		case AP_RESPONSE_BUSY:
> @@ -83,7 +84,7 @@ int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
>    * Unregister the ISC from the GIB alert
>    * Clear the vfio_ap_queue intern fields
>    */
> -static void vfio_ap_free_irq(struct vfio_ap_queue *q)
> +void vfio_ap_free_irq(struct vfio_ap_queue *q)
>   {
>   	if (!q)
>   		return;
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index e2fd2c0..cc18215 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -4,6 +4,7 @@
>    *
>    * Author(s): Tony Krowiak <[e-mail]>
>    *	      Halil Pasic <[e-mail]>
> + *	      Pierre Morel <[e-mail]>
>    *
>    * Copyright IBM Corp. 2018
>    */
> @@ -98,4 +99,6 @@ struct vfio_ap_queue {
>   	int	apqn;
>   	unsigned char isc;
>   };
> +void vfio_ap_free_irq(struct vfio_ap_queue *q);
> +int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q);
>   #endif /* _VFIO_AP_PRIVATE_H_ */
>

Pierre Morel Feb. 27, 2019, 9:58 a.m. UTC | #2

On 26/02/2019 19:27, Tony Krowiak wrote:
> On 2/22/19 10:29 AM, Pierre Morel wrote:
>> When the device is remove, we must make sure to
>> clear the interruption and reset the AP device.
>>
>> We also need to clear the CRYCB of the guest.
>>
>> Signed-off-by: Pierre Morel <[e-mail]>
>> ---
>>   drivers/s390/crypto/vfio_ap_drv.c     | 35 
>> +++++++++++++++++++++++++++++++++++
>>   drivers/s390/crypto/vfio_ap_ops.c     |  3 ++-
>>   drivers/s390/crypto/vfio_ap_private.h |  3 +++
>>   3 files changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c 
>> b/drivers/s390/crypto/vfio_ap_drv.c
>> index eca0ffc..e5d91ff 100644
>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>> @@ -5,6 +5,7 @@
>>    * Copyright IBM Corp. 2018
>>    *
>>    * Author(s): Tony Krowiak <[e-mail]>
>> + *          Pierre Morel <[e-mail]>
>>    */
>>   #include <linux/module.h>
>> @@ -12,6 +13,8 @@
>>   #include <linux/slab.h>
>>   #include <linux/string.h>
>>   #include <asm/facility.h>
>> +#include <linux/bitops.h>
>> +#include <linux/kvm_host.h>
>>   #include "vfio_ap_private.h"
>>   #define VFIO_AP_ROOT_NAME "vfio_ap"
>> @@ -61,6 +64,33 @@ static int vfio_ap_queue_dev_probe(struct ap_device 
>> *apdev)
>>   }
>>   /**
>> + * vfio_ap_update_crycb
>> + * @q: A pointer to the queue being removed
>> + *
>> + * We clear the APID of the queue, making this queue unusable for the 
>> guest.
>> + * After this function we can reset the queue without to fear a race 
>> with
>> + * the guest to access the queue again.
>> + * We do not fear race with the host as we still get the device.
>> + */
>> +static void vfio_ap_update_crycb(struct vfio_ap_queue *q)
>> +{
>> +    struct ap_matrix_mdev *matrix_mdev = q->matrix_mdev;
>> +
>> +    if (!matrix_mdev)
>> +        return;
>> +
>> +    clear_bit_inv(AP_QID_CARD(q->apqn), matrix_mdev->matrix.apm);
>> +
>> +    if (!matrix_mdev->kvm)
>> +        return;
>> +
>> +    kvm_arch_crypto_set_masks(matrix_mdev->kvm,
>> +                  matrix_mdev->matrix.apm,
>> +                  matrix_mdev->matrix.aqm,
>> +                  matrix_mdev->matrix.adm);
>> +}
>> +
>> +/**
>>    * vfio_ap_queue_dev_remove:
>>    *
>>    * Free the associated vfio_ap_queue structure
>> @@ -70,6 +100,11 @@ static void vfio_ap_queue_dev_remove(struct 
>> ap_device *apdev)
>>       struct vfio_ap_queue *q;
>>       q = dev_get_drvdata(&apdev->device);
>> +    if (!q)
>> +        return;
>> +
>> +    vfio_ap_update_crycb(q);
>> +    vfio_ap_mdev_reset_queue(q);
> 
> The reset is unnecessary because once the card is removed from the
> CRYCB, the ZAPQ may fail with because the queue may not exist anymore.

The code here is run inside the host. So the queue is still available 
for ZAPQ.


> Besides, once the card is removed from the guest's CRYCB, the bus
> running in the guest will do a reset.

I already answered this:
- The AP bus reset the queue before calling the driver probe
- which means that the guest can still access the queue after the reset 
done by the bus.
- we need to first clear the CRYCB to remove the queue from the guest 
before we reset the queue.


Regards,
Pierre

Cornelia Huck March 4, 2019, 1:02 p.m. UTC | #3

On Tue, 26 Feb 2019 13:27:57 -0500
Tony Krowiak <[e-mail]> wrote:

> On 2/22/19 10:29 AM, Pierre Morel wrote:
> > When the device is remove, we must make sure to
> > clear the interruption and reset the AP device.
> > 
> > We also need to clear the CRYCB of the guest.
> > 
> > Signed-off-by: Pierre Morel <[e-mail]>
> > ---
> >   drivers/s390/crypto/vfio_ap_drv.c     | 35 +++++++++++++++++++++++++++++++++++
> >   drivers/s390/crypto/vfio_ap_ops.c     |  3 ++-
> >   drivers/s390/crypto/vfio_ap_private.h |  3 +++
> >   3 files changed, 40 insertions(+), 1 deletion(-)
(...)
> >   /**
> > + * vfio_ap_update_crycb
> > + * @q: A pointer to the queue being removed
> > + *
> > + * We clear the APID of the queue, making this queue unusable for the guest.
> > + * After this function we can reset the queue without to fear a race with
> > + * the guest to access the queue again.
> > + * We do not fear race with the host as we still get the device.
> > + */
> > +static void vfio_ap_update_crycb(struct vfio_ap_queue *q)
> > +{
> > +	struct ap_matrix_mdev *matrix_mdev = q->matrix_mdev;
> > +
> > +	if (!matrix_mdev)
> > +		return;
> > +
> > +	clear_bit_inv(AP_QID_CARD(q->apqn), matrix_mdev->matrix.apm);
> > +
> > +	if (!matrix_mdev->kvm)
> > +		return;
> > +
> > +	kvm_arch_crypto_set_masks(matrix_mdev->kvm,
> > +				  matrix_mdev->matrix.apm,
> > +				  matrix_mdev->matrix.aqm,
> > +				  matrix_mdev->matrix.adm);
> > +}
> > +
> > +/**
> >    * vfio_ap_queue_dev_remove:
> >    *
> >    * Free the associated vfio_ap_queue structure
> > @@ -70,6 +100,11 @@ static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
> >   	struct vfio_ap_queue *q;
> >   
> >   	q = dev_get_drvdata(&apdev->device);
> > +	if (!q)
> > +		return;
> > +
> > +	vfio_ap_update_crycb(q);
> > +	vfio_ap_mdev_reset_queue(q);  
> 
> The reset is unnecessary because once the card is removed from the
> CRYCB, the ZAPQ may fail with because the queue may not exist anymore.
> Besides, once the card is removed from the guest's CRYCB, the bus
> running in the guest will do a reset.

You cannot rely on whatever a sane guest would do, any needed cleanup
needs to be done by the host.

(No idea what actually needs to be done here :)

> 
> >   	list_del(&q->list);
> >   	kfree(q);
> >   }

Anthony Krowiak March 8, 2019, 10:43 p.m. UTC | #4

On 2/22/19 10:29 AM, Pierre Morel wrote:
> When the device is remove, we must make sure to
> clear the interruption and reset the AP device.
> 
> We also need to clear the CRYCB of the guest.
> 
> Signed-off-by: Pierre Morel <[e-mail]>
> ---
>   drivers/s390/crypto/vfio_ap_drv.c     | 35 +++++++++++++++++++++++++++++++++++
>   drivers/s390/crypto/vfio_ap_ops.c     |  3 ++-
>   drivers/s390/crypto/vfio_ap_private.h |  3 +++
>   3 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index eca0ffc..e5d91ff 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -5,6 +5,7 @@
>    * Copyright IBM Corp. 2018
>    *
>    * Author(s): Tony Krowiak <[e-mail]>
> + *	      Pierre Morel <[e-mail]>
>    */
>   
>   #include <linux/module.h>
> @@ -12,6 +13,8 @@
>   #include <linux/slab.h>
>   #include <linux/string.h>
>   #include <asm/facility.h>
> +#include <linux/bitops.h>
> +#include <linux/kvm_host.h>
>   #include "vfio_ap_private.h"
>   
>   #define VFIO_AP_ROOT_NAME "vfio_ap"
> @@ -61,6 +64,33 @@ static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
>   }
>   
>   /**
> + * vfio_ap_update_crycb
> + * @q: A pointer to the queue being removed
> + *
> + * We clear the APID of the queue, making this queue unusable for the guest.
> + * After this function we can reset the queue without to fear a race with
> + * the guest to access the queue again.
> + * We do not fear race with the host as we still get the device.
> + */
> +static void vfio_ap_update_crycb(struct vfio_ap_queue *q)
> +{
> +	struct ap_matrix_mdev *matrix_mdev = q->matrix_mdev;
> +
> +	if (!matrix_mdev)
> +		return;
> +
> +	clear_bit_inv(AP_QID_CARD(q->apqn), matrix_mdev->matrix.apm);
> +
> +	if (!matrix_mdev->kvm)
> +		return;
> +
> +	kvm_arch_crypto_set_masks(matrix_mdev->kvm,
> +				  matrix_mdev->matrix.apm,
> +				  matrix_mdev->matrix.aqm,
> +				  matrix_mdev->matrix.adm);
> +}
> +
> +/**
>    * vfio_ap_queue_dev_remove:
>    *
>    * Free the associated vfio_ap_queue structure
> @@ -70,6 +100,11 @@ static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
>   	struct vfio_ap_queue *q;
>   
>   	q = dev_get_drvdata(&apdev->device);
> +	if (!q)
> +		return;
> +
> +	vfio_ap_update_crycb(q);
> +	vfio_ap_mdev_reset_queue(q);

Since the bit corresponding to the APID is cleared in the
vfio_ap_update_crycb() above, shouldn't all queues on that
card also be reset?

>   	list_del(&q->list);
>   	kfree(q);
>   }
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 0196065..5b9bb33 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -59,6 +59,7 @@ int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
>   			if (retry <= 0)
>   				pr_warn("%s: queue 0x%04x not empty\n",
>   					__func__, q->apqn);
> +			vfio_ap_free_irq(q);

Shouldn't this be done for the response codes that terminate this loop
such as those caught by the default case?

>   			return 0;
>   		case AP_RESPONSE_RESET_IN_PROGRESS:
>   		case AP_RESPONSE_BUSY:
> @@ -83,7 +84,7 @@ int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
>    * Unregister the ISC from the GIB alert
>    * Clear the vfio_ap_queue intern fields
>    */
> -static void vfio_ap_free_irq(struct vfio_ap_queue *q)
> +void vfio_ap_free_irq(struct vfio_ap_queue *q)
>   {
>   	if (!q)
>   		return;
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index e2fd2c0..cc18215 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -4,6 +4,7 @@
>    *
>    * Author(s): Tony Krowiak <[e-mail]>
>    *	      Halil Pasic <[e-mail]>
> + *	      Pierre Morel <[e-mail]>
>    *
>    * Copyright IBM Corp. 2018
>    */
> @@ -98,4 +99,6 @@ struct vfio_ap_queue {
>   	int	apqn;
>   	unsigned char isc;
>   };
> +void vfio_ap_free_irq(struct vfio_ap_queue *q);
> +int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q);
>   #endif /* _VFIO_AP_PRIVATE_H_ */
>

Pierre Morel March 11, 2019, 8:31 a.m. UTC | #5

On 08/03/2019 23:43, Tony Krowiak wrote:
> On 2/22/19 10:29 AM, Pierre Morel wrote:
>> When the device is remove, we must make sure to
>> clear the interruption and reset the AP device.
>>
>> We also need to clear the CRYCB of the guest.
>>
>> Signed-off-by: Pierre Morel <[e-mail]>
>> ---
>>   drivers/s390/crypto/vfio_ap_drv.c     | 35 
>> +++++++++++++++++++++++++++++++++++
>>   drivers/s390/crypto/vfio_ap_ops.c     |  3 ++-
>>   drivers/s390/crypto/vfio_ap_private.h |  3 +++
>>   3 files changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c 
>> b/drivers/s390/crypto/vfio_ap_drv.c
>> index eca0ffc..e5d91ff 100644
>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>> @@ -5,6 +5,7 @@
>>    * Copyright IBM Corp. 2018
>>    *
>>    * Author(s): Tony Krowiak <[e-mail]>
>> + *          Pierre Morel <[e-mail]>
>>    */
>>   #include <linux/module.h>
>> @@ -12,6 +13,8 @@
>>   #include <linux/slab.h>
>>   #include <linux/string.h>
>>   #include <asm/facility.h>
>> +#include <linux/bitops.h>
>> +#include <linux/kvm_host.h>
>>   #include "vfio_ap_private.h"
>>   #define VFIO_AP_ROOT_NAME "vfio_ap"
>> @@ -61,6 +64,33 @@ static int vfio_ap_queue_dev_probe(struct ap_device 
>> *apdev)
>>   }
>>   /**
>> + * vfio_ap_update_crycb
>> + * @q: A pointer to the queue being removed
>> + *
>> + * We clear the APID of the queue, making this queue unusable for the 
>> guest.
>> + * After this function we can reset the queue without to fear a race 
>> with
>> + * the guest to access the queue again.
>> + * We do not fear race with the host as we still get the device.
>> + */
>> +static void vfio_ap_update_crycb(struct vfio_ap_queue *q)
>> +{
>> +    struct ap_matrix_mdev *matrix_mdev = q->matrix_mdev;
>> +
>> +    if (!matrix_mdev)
>> +        return;
>> +
>> +    clear_bit_inv(AP_QID_CARD(q->apqn), matrix_mdev->matrix.apm);
>> +
>> +    if (!matrix_mdev->kvm)
>> +        return;
>> +
>> +    kvm_arch_crypto_set_masks(matrix_mdev->kvm,
>> +                  matrix_mdev->matrix.apm,
>> +                  matrix_mdev->matrix.aqm,
>> +                  matrix_mdev->matrix.adm);
>> +}
>> +
>> +/**
>>    * vfio_ap_queue_dev_remove:
>>    *
>>    * Free the associated vfio_ap_queue structure
>> @@ -70,6 +100,11 @@ static void vfio_ap_queue_dev_remove(struct 
>> ap_device *apdev)
>>       struct vfio_ap_queue *q;
>>       q = dev_get_drvdata(&apdev->device);
>> +    if (!q)
>> +        return;
>> +
>> +    vfio_ap_update_crycb(q);
>> +    vfio_ap_mdev_reset_queue(q);
> 
> Since the bit corresponding to the APID is cleared in the
> vfio_ap_update_crycb() above, shouldn't all queues on that
> card also be reset?

I do not think so.
The remove function will be called in a loop for all queues by the bus.
No need to clear all queues.


> 
>>       list_del(&q->list);
>>       kfree(q);
>>   }
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index 0196065..5b9bb33 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -59,6 +59,7 @@ int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
>>               if (retry <= 0)
>>                   pr_warn("%s: queue 0x%04x not empty\n",
>>                       __func__, q->apqn);
>> +            vfio_ap_free_irq(q);
> 
> Shouldn't this be done for the response codes that terminate this loop
> such as those caught by the default case?

I do not think so, the error code is returned and the caller may want to 
reset the queue again.
I think that doing the free inside the call to reset is not right.
I will investigate in this direction.

Regards,
Pierre

Anthony Krowiak March 12, 2019, 9:53 p.m. UTC | #6

On 3/11/19 4:31 AM, Pierre Morel wrote:
> On 08/03/2019 23:43, Tony Krowiak wrote:
>> On 2/22/19 10:29 AM, Pierre Morel wrote:
>>> When the device is remove, we must make sure to
>>> clear the interruption and reset the AP device.
>>>
>>> We also need to clear the CRYCB of the guest.
>>>
>>> Signed-off-by: Pierre Morel <[e-mail]>
>>> ---
>>>   drivers/s390/crypto/vfio_ap_drv.c     | 35 
>>> +++++++++++++++++++++++++++++++++++
>>>   drivers/s390/crypto/vfio_ap_ops.c     |  3 ++-
>>>   drivers/s390/crypto/vfio_ap_private.h |  3 +++
>>>   3 files changed, 40 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c 
>>> b/drivers/s390/crypto/vfio_ap_drv.c
>>> index eca0ffc..e5d91ff 100644
>>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>>> @@ -5,6 +5,7 @@
>>>    * Copyright IBM Corp. 2018
>>>    *
>>>    * Author(s): Tony Krowiak <[e-mail]>
>>> + *          Pierre Morel <[e-mail]>
>>>    */
>>>   #include <linux/module.h>
>>> @@ -12,6 +13,8 @@
>>>   #include <linux/slab.h>
>>>   #include <linux/string.h>
>>>   #include <asm/facility.h>
>>> +#include <linux/bitops.h>
>>> +#include <linux/kvm_host.h>
>>>   #include "vfio_ap_private.h"
>>>   #define VFIO_AP_ROOT_NAME "vfio_ap"
>>> @@ -61,6 +64,33 @@ static int vfio_ap_queue_dev_probe(struct 
>>> ap_device *apdev)
>>>   }
>>>   /**
>>> + * vfio_ap_update_crycb
>>> + * @q: A pointer to the queue being removed
>>> + *
>>> + * We clear the APID of the queue, making this queue unusable for 
>>> the guest.
>>> + * After this function we can reset the queue without to fear a race 
>>> with
>>> + * the guest to access the queue again.
>>> + * We do not fear race with the host as we still get the device.
>>> + */
>>> +static void vfio_ap_update_crycb(struct vfio_ap_queue *q)
>>> +{
>>> +    struct ap_matrix_mdev *matrix_mdev = q->matrix_mdev;
>>> +
>>> +    if (!matrix_mdev)
>>> +        return;
>>> +

You should probably check whether the APID has been cleared before
proceeding. Take the case where an AP with multiple queues is removed
from the configuration via the SE or SCLP. The AP bus is going to invoke
the vfio_ap_queue_dev_remove() function for each of the queues. The APID
will get cleared on the first remove, so it is not only unnecessary to
clear it on subsequent removes, it is kind of nasty to keep resetting
the masks in the guest's CRYCB (below) each time the remove callback is
invoked.

>>> +    clear_bit_inv(AP_QID_CARD(q->apqn), matrix_mdev->matrix.apm);
>>> +
>>> +    if (!matrix_mdev->kvm)
>>> +        return;
>>> +
>>> +    kvm_arch_crypto_set_masks(matrix_mdev->kvm,
>>> +                  matrix_mdev->matrix.apm,
>>> +                  matrix_mdev->matrix.aqm,
>>> +                  matrix_mdev->matrix.adm);
>>> +}
>>> +
>>> +/**
>>>    * vfio_ap_queue_dev_remove:
>>>    *
>>>    * Free the associated vfio_ap_queue structure
>>> @@ -70,6 +100,11 @@ static void vfio_ap_queue_dev_remove(struct 
>>> ap_device *apdev)
>>>       struct vfio_ap_queue *q;
>>>       q = dev_get_drvdata(&apdev->device);
>>> +    if (!q)
>>> +        return;
>>> +
>>> +    vfio_ap_update_crycb(q);
>>> +    vfio_ap_mdev_reset_queue(q);
>>
>> Since the bit corresponding to the APID is cleared in the
>> vfio_ap_update_crycb() above, shouldn't all queues on that
>> card also be reset?
> 
> I do not think so.
> The remove function will be called in a loop for all queues by the bus.
> No need to clear all queues.
> 
> 
>>
>>>       list_del(&q->list);
>>>       kfree(q);
>>>   }
>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>>> b/drivers/s390/crypto/vfio_ap_ops.c
>>> index 0196065..5b9bb33 100644
>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>>> @@ -59,6 +59,7 @@ int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
>>>               if (retry <= 0)
>>>                   pr_warn("%s: queue 0x%04x not empty\n",
>>>                       __func__, q->apqn);
>>> +            vfio_ap_free_irq(q);
>>
>> Shouldn't this be done for the response codes that terminate this loop
>> such as those caught by the default case?
> 
> I do not think so, the error code is returned and the caller may want to 
> reset the queue again.
> I think that doing the free inside the call to reset is not right.
> I will investigate in this direction.
> 
> Regards,
> Pierre
>

Pierre Morel March 13, 2019, 10:15 a.m. UTC | #7

On 12/03/2019 22:53, Tony Krowiak wrote:
> On 3/11/19 4:31 AM, Pierre Morel wrote:
>> On 08/03/2019 23:43, Tony Krowiak wrote:
>>> On 2/22/19 10:29 AM, Pierre Morel wrote:
>>>> When the device is remove, we must make sure to
>>>> clear the interruption and reset the AP device.
>>>>
>>>> We also need to clear the CRYCB of the guest.
>>>>
>>>> Signed-off-by: Pierre Morel <[e-mail]>
>>>> ---
>>>>   drivers/s390/crypto/vfio_ap_drv.c     | 35 
>>>> +++++++++++++++++++++++++++++++++++
>>>>   drivers/s390/crypto/vfio_ap_ops.c     |  3 ++-
>>>>   drivers/s390/crypto/vfio_ap_private.h |  3 +++
>>>>   3 files changed, 40 insertions(+), 1 deletion(-)
>>>>

...snip...

>>>> + * vfio_ap_update_crycb
>>>> + * @q: A pointer to the queue being removed
>>>> + *
>>>> + * We clear the APID of the queue, making this queue unusable for 
>>>> the guest.
>>>> + * After this function we can reset the queue without to fear a 
>>>> race with
>>>> + * the guest to access the queue again.
>>>> + * We do not fear race with the host as we still get the devic
>>>> + */
>>>> +static void vfio_ap_update_crycb(struct vfio_ap_queue *q)
>>>> +{
>>>> +    struct ap_matrix_mdev *matrix_mdev = q->matrix_mdev;
>>>> +
>>>> +    if (!matrix_mdev)
>>>> +        return;
>>>> +
> 
> You should probably check whether the APID has been cleared before
> proceeding. Take the case where an AP with multiple queues is removed
> from the configuration via the SE or SCLP. The AP bus is going to invoke
> the vfio_ap_queue_dev_remove() function for each of the queues. The APID
> will get cleared on the first remove, so it is not only unnecessary to
> clear it on subsequent removes, it is kind of nasty to keep resetting
> the masks in the guest's CRYCB (below) each time the remove callback is
> invoked.

This can not happen.
The only way to clear the APM is when the matrix is not associated with KVM.

This case is tested and the masks are not changed.

> 
>>>> +    clear_bit_inv(AP_QID_CARD(q->apqn), matrix_mdev->matrix.apm);
>>>> +
>>>> +    if (!matrix_mdev->kvm)
>>>> +        return;
>>>> +
>>>> +    kvm_arch_crypto_set_masks(matrix_mdev->kvm,
>>>> +                  matrix_mdev->matrix.apm,
>>>> +                  matrix_mdev->matrix.aqm,
>>>> +                  matrix_mdev->matrix.adm);
>>>> +}

Patch

diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index eca0ffc..e5d91ff 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -5,6 +5,7 @@ 
  * Copyright IBM Corp. 2018
  *
  * Author(s): Tony Krowiak <[e-mail]>
+ *	      Pierre Morel <[e-mail]>
  */
 
 #include <linux/module.h>
@@ -12,6 +13,8 @@ 
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <asm/facility.h>
+#include <linux/bitops.h>
+#include <linux/kvm_host.h>
 #include "vfio_ap_private.h"
 
 #define VFIO_AP_ROOT_NAME "vfio_ap"
@@ -61,6 +64,33 @@  static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
 }
 
 /**
+ * vfio_ap_update_crycb
+ * @q: A pointer to the queue being removed
+ *
+ * We clear the APID of the queue, making this queue unusable for the guest.
+ * After this function we can reset the queue without to fear a race with
+ * the guest to access the queue again.
+ * We do not fear race with the host as we still get the device.
+ */
+static void vfio_ap_update_crycb(struct vfio_ap_queue *q)
+{
+	struct ap_matrix_mdev *matrix_mdev = q->matrix_mdev;
+
+	if (!matrix_mdev)
+		return;
+
+	clear_bit_inv(AP_QID_CARD(q->apqn), matrix_mdev->matrix.apm);
+
+	if (!matrix_mdev->kvm)
+		return;
+
+	kvm_arch_crypto_set_masks(matrix_mdev->kvm,
+				  matrix_mdev->matrix.apm,
+				  matrix_mdev->matrix.aqm,
+				  matrix_mdev->matrix.adm);
+}
+
+/**
  * vfio_ap_queue_dev_remove:
  *
  * Free the associated vfio_ap_queue structure
@@ -70,6 +100,11 @@  static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
 	struct vfio_ap_queue *q;
 
 	q = dev_get_drvdata(&apdev->device);
+	if (!q)
+		return;
+
+	vfio_ap_update_crycb(q);
+	vfio_ap_mdev_reset_queue(q);
 	list_del(&q->list);
 	kfree(q);
 }
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 0196065..5b9bb33 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -59,6 +59,7 @@  int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
 			if (retry <= 0)
 				pr_warn("%s: queue 0x%04x not empty\n",
 					__func__, q->apqn);
+			vfio_ap_free_irq(q);
 			return 0;
 		case AP_RESPONSE_RESET_IN_PROGRESS:
 		case AP_RESPONSE_BUSY:
@@ -83,7 +84,7 @@  int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
  * Unregister the ISC from the GIB alert
  * Clear the vfio_ap_queue intern fields
  */
-static void vfio_ap_free_irq(struct vfio_ap_queue *q)
+void vfio_ap_free_irq(struct vfio_ap_queue *q)
 {
 	if (!q)
 		return;
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index e2fd2c0..cc18215 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -4,6 +4,7 @@ 
  *
  * Author(s): Tony Krowiak <[e-mail]>
  *	      Halil Pasic <[e-mail]>
+ *	      Pierre Morel <[e-mail]>
  *
  * Copyright IBM Corp. 2018
  */
@@ -98,4 +99,6 @@  struct vfio_ap_queue {
 	int	apqn;
 	unsigned char isc;
 };
+void vfio_ap_free_irq(struct vfio_ap_queue *q);
+int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q);
 #endif /* _VFIO_AP_PRIVATE_H_ */