diff mbox series

[v7,18/22] s390: vfio-ap: zeroize the AP queues.

Message ID 20180726195429.31960-19-borntraeger@de.ibm.com (mailing list archive)
State New, archived
Headers show
Series vfio-ap: guest dedicated crypto adapters | expand

Commit Message

Christian Borntraeger July 26, 2018, 7:54 p.m. UTC
From: Tony Krowiak <akrowiak@linux.ibm.com>

Let's call PAPQ(ZAPQ) to zeroize a queue:

* For each queue configured for a mediated matrix device
  when it is released.

* When an AP queue is unbound from the VFIO AP device driver.

Zeroizing a queue resets the queue, clears all pending
messages for the queue entries and disables adapter interruptions
associated with the queue.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
Tested-by: Michael Mueller <mimu@linux.ibm.com>
Tested-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 drivers/s390/crypto/vfio_ap_drv.c     |  6 ++++-
 drivers/s390/crypto/vfio_ap_ops.c     | 32 +++++++++++++++++++++++++++
 drivers/s390/crypto/vfio_ap_private.h | 25 +++++++++++++++++++++
 3 files changed, 62 insertions(+), 1 deletion(-)

Comments

Cornelia Huck July 31, 2018, 12:24 p.m. UTC | #1
On Thu, 26 Jul 2018 21:54:25 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Tony Krowiak <akrowiak@linux.ibm.com>
> 
> Let's call PAPQ(ZAPQ) to zeroize a queue:
> 
> * For each queue configured for a mediated matrix device
>   when it is released.
> 
> * When an AP queue is unbound from the VFIO AP device driver.

OK, that confuses me...

> 
> Zeroizing a queue resets the queue, clears all pending
> messages for the queue entries and disables adapter interruptions
> associated with the queue.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
> Tested-by: Michael Mueller <mimu@linux.ibm.com>
> Tested-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  drivers/s390/crypto/vfio_ap_drv.c     |  6 ++++-
>  drivers/s390/crypto/vfio_ap_ops.c     | 32 +++++++++++++++++++++++++++
>  drivers/s390/crypto/vfio_ap_private.h | 25 +++++++++++++++++++++
>  3 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index 25636e6bf893..936f025fc8e9 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -59,7 +59,11 @@ static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
>  
>  static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
>  {
> -	/* Nothing to do yet */
> +	/*
> +	 * NOTE: We still regard the queue as ours. For taking away queues
> +	 * from vfio_ap the ap bus will provide a callback interface, so we
> +	 * can take away the queues from the guests if needed.
> +	 */

...as we don't seem to do anything here yet?

>  }
>  
>  static int vfio_ap_matrix_dev_create(void)
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 01c429cb51d5..461a74515d3d 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -803,6 +803,37 @@ static int vfio_ap_mdev_open_once(struct ap_matrix_mdev *matrix_mdev)
>  	return ret;
>  }
>  
> +static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev, bool force)
> +{
> +	int ret;
> +	int rc = 0;
> +	unsigned long apid, apqi;
> +	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
> +
> +	if (!matrix_mdev->activated) {
> +		pr_err("%s: reset failed for mdev %s, not activated",
> +		       VFIO_AP_MODULE_NAME, matrix_mdev->name);
> +
> +		return -EPERM;
> +	}

Hm... if we stick with the activation approach, what happens if we:
- open the mdev
- activate the matrix
- deactivate the matrix
- release the mdev, triggering this function

It seems we won't call PAPQ(ZAPQ) in that case -- is that how it is
supposed to work?

> +
> +	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
> +			     matrix_mdev->matrix.apm_max + 1) {
> +		for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
> +				     matrix_mdev->matrix.aqm_max + 1) {
> +			ret = vfio_ap_reset_queue(apid, apqi, 1);
> +			if (ret) {
> +				if (force)
> +					rc = ret;
> +				else
> +					return ret;
> +			}
> +		}
> +	}
> +
> +	return rc;
> +}
> +
>  static int vfio_ap_mdev_open(struct mdev_device *mdev)
>  {
>  	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
> @@ -860,6 +891,7 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
>  
>  	mutex_lock(&matrix_dev.lock);
>  	kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
> +	vfio_ap_mdev_reset_queues(mdev, true);
>  	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>  				 &matrix_mdev->group_notifier);
>  	matrix_mdev->kvm = NULL;
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 34be9afe9ced..f11457ca90f6 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -76,4 +76,29 @@ static inline struct device *to_device(struct ap_matrix_dev *matrix_dev)
>  extern int vfio_ap_mdev_register(void);
>  extern void vfio_ap_mdev_unregister(void);
>  
> +static inline int vfio_ap_reset_queue(unsigned int apid, unsigned int apqi,
> +				      unsigned int retry)
> +{
> +	struct ap_queue_status status;
> +
> +	do {
> +		status = ap_zapq(AP_MKQID(apid, apqi));
> +		switch (status.response_code) {
> +		case AP_RESPONSE_NORMAL:
> +			return 0;
> +		case AP_RESPONSE_RESET_IN_PROGRESS:
> +		case AP_RESPONSE_BUSY:
> +			msleep(20);
> +			break;
> +		default:
> +			pr_warn("%s: error zeroizing %02x.%04x: response code %d\n",
> +				VFIO_AP_MODULE_NAME, apid, apqi,
> +				status.response_code);

Again, introducing a dbf may be better than dumping too many messages
into the syslog.

> +			return -EIO;
> +		}
> +	} while (retry--);
> +
> +	return -EBUSY;
> +}
> +
>  #endif /* _VFIO_AP_PRIVATE_H_ */
Halil Pasic July 31, 2018, 1:12 p.m. UTC | #2
On 07/31/2018 02:24 PM, Cornelia Huck wrote:
>> +static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev, bool force)
>> +{
>> +	int ret;
>> +	int rc = 0;
>> +	unsigned long apid, apqi;
>> +	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>> +
>> +	if (!matrix_mdev->activated) {
>> +		pr_err("%s: reset failed for mdev %s, not activated",
>> +		       VFIO_AP_MODULE_NAME, matrix_mdev->name);
>> +
>> +		return -EPERM;
>> +	}
> Hm... if we stick with the activation approach, what happens if we:
> - open the mdev
> - activate the matrix
> - deactivate the matrix

I guess this step will fail and the matrix remains activated.

Have a look at vfio_ap_mdev_deactivate()

> - release the mdev, triggering this function
> 
> It seems we won't call PAPQ(ZAPQ) in that case -- is that how it is
> supposed to work?
> 

So basically the device remains active until the mdev release is
called if I'm correct.

Halil
Cornelia Huck July 31, 2018, 1:40 p.m. UTC | #3
On Tue, 31 Jul 2018 15:12:59 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On 07/31/2018 02:24 PM, Cornelia Huck wrote:
> >> +static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev, bool force)
> >> +{
> >> +	int ret;
> >> +	int rc = 0;
> >> +	unsigned long apid, apqi;
> >> +	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
> >> +
> >> +	if (!matrix_mdev->activated) {
> >> +		pr_err("%s: reset failed for mdev %s, not activated",
> >> +		       VFIO_AP_MODULE_NAME, matrix_mdev->name);
> >> +
> >> +		return -EPERM;
> >> +	}  
> > Hm... if we stick with the activation approach, what happens if we:
> > - open the mdev
> > - activate the matrix
> > - deactivate the matrix  
> 
> I guess this step will fail and the matrix remains activated.
> 
> Have a look at vfio_ap_mdev_deactivate()

Hm, I don't see it...

> 
> > - release the mdev, triggering this function
> > 
> > It seems we won't call PAPQ(ZAPQ) in that case -- is that how it is
> > supposed to work?
> >   
> 
> So basically the device remains active until the mdev release is
> called if I'm correct.

So, why do we need the activate/deactivate approach, then? Why can't we
do the verification etc. during open?
Halil Pasic July 31, 2018, 2:18 p.m. UTC | #4
On 07/31/2018 03:40 PM, Cornelia Huck wrote:
> On Tue, 31 Jul 2018 15:12:59 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
>> On 07/31/2018 02:24 PM, Cornelia Huck wrote:
>>>> +static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev, bool force)
>>>> +{
>>>> +	int ret;
>>>> +	int rc = 0;
>>>> +	unsigned long apid, apqi;
>>>> +	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>>>> +
>>>> +	if (!matrix_mdev->activated) {
>>>> +		pr_err("%s: reset failed for mdev %s, not activated",
>>>> +		       VFIO_AP_MODULE_NAME, matrix_mdev->name);
>>>> +
>>>> +		return -EPERM;
>>>> +	}
>>> Hm... if we stick with the activation approach, what happens if we:
>>> - open the mdev
>>> - activate the matrix
>>> - deactivate the matrix
>>
>> I guess this step will fail and the matrix remains activated.
>>
>> Have a look at vfio_ap_mdev_deactivate()
> 
> Hm, I don't see it...
> 
static int vfio_ap_mdev_deactivate(struct ap_matrix_mdev *matrix_mdev)
{
         int ret = 0;
                                                                                 
         if (!matrix_mdev->activated)
                 return 0;
                                                                                 
         if (matrix_mdev->kvm) {
                 pr_warn("%s: %s: deactivate failed, mdev %s is in use by guest %s\n",
                         VFIO_AP_MODULE_NAME, __func__, matrix_mdev->name,
                         matrix_mdev->kvm->arch.dbf->name);
                 return -EBUSY;       <===================================== Return -EBUSY and don't deactivate.
                                                   
         }
                                                                                 
         matrix_mdev->activated = false;
                                                                                 
         return ret;
}


>>
>>> - release the mdev, triggering this function
>>>
>>> It seems we won't call PAPQ(ZAPQ) in that case -- is that how it is
>>> supposed to work?
>>>    
>>
>> So basically the device remains active until the mdev release is
>> called if I'm correct.
> 
> So, why do we need the activate/deactivate approach, then? Why can't we
> do the verification etc. during open?
> 

We ca do the verification during open. If there was no activate before
the open we actually try to do one.

With activate however the admin can claim the resources before the
open (that is the guest start). We have a mechanism for both 'let's
overcommit and fail when we hit the wall' and 'let's make sure everything
is reserved before we give it to the guest'. The admin is free to
decide on his policy.

Please take note that activate also has an effect on the availability
of certain operations (e.g. assign/unassign).

Regards,
Halil
Cornelia Huck July 31, 2018, 2:48 p.m. UTC | #5
On Tue, 31 Jul 2018 16:18:03 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On 07/31/2018 03:40 PM, Cornelia Huck wrote:
> > On Tue, 31 Jul 2018 15:12:59 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> >   
> >> On 07/31/2018 02:24 PM, Cornelia Huck wrote:  
> >>>> +static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev, bool force)
> >>>> +{
> >>>> +	int ret;
> >>>> +	int rc = 0;
> >>>> +	unsigned long apid, apqi;
> >>>> +	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
> >>>> +
> >>>> +	if (!matrix_mdev->activated) {
> >>>> +		pr_err("%s: reset failed for mdev %s, not activated",
> >>>> +		       VFIO_AP_MODULE_NAME, matrix_mdev->name);
> >>>> +
> >>>> +		return -EPERM;
> >>>> +	}  
> >>> Hm... if we stick with the activation approach, what happens if we:
> >>> - open the mdev
> >>> - activate the matrix
> >>> - deactivate the matrix  
> >>
> >> I guess this step will fail and the matrix remains activated.
> >>
> >> Have a look at vfio_ap_mdev_deactivate()  
> > 
> > Hm, I don't see it...
> >   
> static int vfio_ap_mdev_deactivate(struct ap_matrix_mdev *matrix_mdev)
> {
>          int ret = 0;
>                                                                                  
>          if (!matrix_mdev->activated)
>                  return 0;
>                                                                                  
>          if (matrix_mdev->kvm) {
>                  pr_warn("%s: %s: deactivate failed, mdev %s is in use by guest %s\n",
>                          VFIO_AP_MODULE_NAME, __func__, matrix_mdev->name,
>                          matrix_mdev->kvm->arch.dbf->name);
>                  return -EBUSY;       <===================================== Return -EBUSY and don't deactivate.

Yes, but what forces ->kvm to be !NULL? I did not see this in the code,
and I'm not sure what I'm missing. (If ->kvm is guaranteed to be !NULL,
why do we check for it during open?)

>                                                    
>          }
>                                                                                  
>          matrix_mdev->activated = false;
>                                                                                  
>          return ret;
> }
> 
> 
> >>  
> >>> - release the mdev, triggering this function
> >>>
> >>> It seems we won't call PAPQ(ZAPQ) in that case -- is that how it is
> >>> supposed to work?
> >>>      
> >>
> >> So basically the device remains active until the mdev release is
> >> called if I'm correct.  
> > 
> > So, why do we need the activate/deactivate approach, then? Why can't we
> > do the verification etc. during open?
> >   
> 
> We ca do the verification during open. If there was no activate before
> the open we actually try to do one.
> 
> With activate however the admin can claim the resources before the
> open (that is the guest start). We have a mechanism for both 'let's
> overcommit and fail when we hit the wall' and 'let's make sure everything
> is reserved before we give it to the guest'. The admin is free to
> decide on his policy.

OK, let's discuss that in the other thread, then. I am not sure that
departing from the general mdev model is worth it.

(I have not yet gotten to replying there.)

> 
> Please take note that activate also has an effect on the availability
> of certain operations (e.g. assign/unassign).
> 
> Regards,
> Halil
>
Pierre Morel July 31, 2018, 3:48 p.m. UTC | #6
On 31/07/2018 16:48, Cornelia Huck wrote:
> On Tue, 31 Jul 2018 16:18:03 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
>
>> On 07/31/2018 03:40 PM, Cornelia Huck wrote:
>>> On Tue, 31 Jul 2018 15:12:59 +0200
>>> Halil Pasic <pasic@linux.ibm.com> wrote:
>>>    
>>>> On 07/31/2018 02:24 PM, Cornelia Huck wrote:
>>>>>> +static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev, bool force)
>>>>>> +{
>>>>>> +	int ret;
>>>>>> +	int rc = 0;
>>>>>> +	unsigned long apid, apqi;
>>>>>> +	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>>>>>> +
>>>>>> +	if (!matrix_mdev->activated) {
>>>>>> +		pr_err("%s: reset failed for mdev %s, not activated",
>>>>>> +		       VFIO_AP_MODULE_NAME, matrix_mdev->name);
>>>>>> +
>>>>>> +		return -EPERM;
>>>>>> +	}
>>>>> Hm... if we stick with the activation approach, what happens if we:
>>>>> - open the mdev
>>>>> - activate the matrix
>>>>> - deactivate the matrix
>>>> I guess this step will fail and the matrix remains activated.
>>>>
>>>> Have a look at vfio_ap_mdev_deactivate()
>>> Hm, I don't see it...
>>>    
>> static int vfio_ap_mdev_deactivate(struct ap_matrix_mdev *matrix_mdev)
>> {
>>           int ret = 0;
>>                                                                                   
>>           if (!matrix_mdev->activated)
>>                   return 0;
>>                                                                                   
>>           if (matrix_mdev->kvm) {
>>                   pr_warn("%s: %s: deactivate failed, mdev %s is in use by guest %s\n",
>>                           VFIO_AP_MODULE_NAME, __func__, matrix_mdev->name,
>>                           matrix_mdev->kvm->arch.dbf->name);
>>                   return -EBUSY;       <===================================== Return -EBUSY and don't deactivate.
> Yes, but what forces ->kvm to be !NULL? I did not see this in the code,
> and I'm not sure what I'm missing. (If ->kvm is guaranteed to be !NULL,
> why do we check for it during open?)

The matrix_mdev->kvm pointer life cycle is, currently:

1) initialized to 0
2) Set during vfio device open by the callback of the 
VFIO_GROUP_NOTIFY_SET_KVM notification
3) Cleared during vfio device close.

Since VFIO-AP only works with KVM, I think that you are right and that 
we could
forget the check on open.

However the callback is called again when the guest is stopping,
we need to handle this too in the next version.

Thanks for the comment,

Regards,

Pierre

>
>>                                                     
>>           }
>>                                                                                   
>>           matrix_mdev->activated = false;
>>                                                                                   
>>           return ret;
>> }
>>
>>
>>>>   
>>>>> - release the mdev, triggering this function
>>>>>
>>>>> It seems we won't call PAPQ(ZAPQ) in that case -- is that how it is
>>>>> supposed to work?
>>>>>       
>>>> So basically the device remains active until the mdev release is
>>>> called if I'm correct.
>>> So, why do we need the activate/deactivate approach, then? Why can't we
>>> do the verification etc. during open?
>>>    
>> We ca do the verification during open. If there was no activate before
>> the open we actually try to do one.
>>
>> With activate however the admin can claim the resources before the
>> open (that is the guest start). We have a mechanism for both 'let's
>> overcommit and fail when we hit the wall' and 'let's make sure everything
>> is reserved before we give it to the guest'. The admin is free to
>> decide on his policy.
> OK, let's discuss that in the other thread, then. I am not sure that
> departing from the general mdev model is worth it.
>
> (I have not yet gotten to replying there.)
>
>> Please take note that activate also has an effect on the availability
>> of certain operations (e.g. assign/unassign).
>>
>> Regards,
>> Halil
>>
Halil Pasic July 31, 2018, 3:49 p.m. UTC | #7
On 07/31/2018 04:48 PM, Cornelia Huck wrote:
> On Tue, 31 Jul 2018 16:18:03 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
>> On 07/31/2018 03:40 PM, Cornelia Huck wrote:
>>> On Tue, 31 Jul 2018 15:12:59 +0200
>>> Halil Pasic <pasic@linux.ibm.com> wrote:
>>>    
>>>> On 07/31/2018 02:24 PM, Cornelia Huck wrote:
>>>>>> +static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev, bool force)
>>>>>> +{
>>>>>> +	int ret;
>>>>>> +	int rc = 0;
>>>>>> +	unsigned long apid, apqi;
>>>>>> +	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>>>>>> +
>>>>>> +	if (!matrix_mdev->activated) {
>>>>>> +		pr_err("%s: reset failed for mdev %s, not activated",
>>>>>> +		       VFIO_AP_MODULE_NAME, matrix_mdev->name);
>>>>>> +
>>>>>> +		return -EPERM;
>>>>>> +	}
>>>>> Hm... if we stick with the activation approach, what happens if we:
>>>>> - open the mdev
>>>>> - activate the matrix
>>>>> - deactivate the matrix
>>>>
>>>> I guess this step will fail and the matrix remains activated.
>>>>
>>>> Have a look at vfio_ap_mdev_deactivate()
>>>
>>> Hm, I don't see it...
>>>    
>> static int vfio_ap_mdev_deactivate(struct ap_matrix_mdev *matrix_mdev)
>> {
>>           int ret = 0;
>>                                                                                   
>>           if (!matrix_mdev->activated)
>>                   return 0;
>>                                                                                   
>>           if (matrix_mdev->kvm) {
>>                   pr_warn("%s: %s: deactivate failed, mdev %s is in use by guest %s\n",
>>                           VFIO_AP_MODULE_NAME, __func__, matrix_mdev->name,
>>                           matrix_mdev->kvm->arch.dbf->name);
>>                   return -EBUSY;       <===================================== Return -EBUSY and don't deactivate.
> 
> Yes, but what forces ->kvm to be !NULL? I did not see this in the code,
> and I'm not sure what I'm missing. (If ->kvm is guaranteed to be !NULL,
> why do we check for it during open?)

The ->kvm should become !NULL as a part of the open. If there is a guest (kvm),
and we strongly hope that vfio_register_notifier() will give us a pointer
to kvm via the callback. The vfio_register_notifier() is written in such
way, that if we can have a pointer to the kvm representing the VM immediately
after the call to it we will. We used to depend on this in v6. If the
vfio_ap_mdev_group_notifier() callback is called substantially later we
still have a problem, as we don't re-check for sharing conflicts before
we do the copy into the crycb in vfio_ap_mdev_group_notifier(). And
we could not prevent the guest from starting but it would end up with
a dysfunctional vfio_ap mdev that looks good.

> 
>>                                                     
>>           }
>>                                                                                   
>>           matrix_mdev->activated = false;
>>                                                                                   
>>           return ret;
>> }
>>
>>
>>>>   
>>>>> - release the mdev, triggering this function
>>>>>
>>>>> It seems we won't call PAPQ(ZAPQ) in that case -- is that how it is
>>>>> supposed to work?
>>>>>       
>>>>
>>>> So basically the device remains active until the mdev release is
>>>> called if I'm correct.
>>>
>>> So, why do we need the activate/deactivate approach, then? Why can't we
>>> do the verification etc. during open?
>>>    
>>
>> We ca do the verification during open. If there was no activate before
>> the open we actually try to do one.
>>
>> With activate however the admin can claim the resources before the
>> open (that is the guest start). We have a mechanism for both 'let's
>> overcommit and fail when we hit the wall' and 'let's make sure everything
>> is reserved before we give it to the guest'. The admin is free to
>> decide on his policy.
> 
> OK, let's discuss that in the other thread, then. I am not sure that
> departing from the general mdev model is worth it.
> 
> (I have not yet gotten to replying there.)
> 

Let's continue the discussion there. I'm hoping Daniel will chime in.
In my understanding, we are inherently different than the rest of the
mdev devices. And I don't think we depart regarding the common stuff
regulated by mdev (e.g. the max instances). But we wanted to discuss
such stuff there...

Regards,
Halil
diff mbox series

Patch

diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index 25636e6bf893..936f025fc8e9 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -59,7 +59,11 @@  static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
 
 static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
 {
-	/* Nothing to do yet */
+	/*
+	 * NOTE: We still regard the queue as ours. For taking away queues
+	 * from vfio_ap the ap bus will provide a callback interface, so we
+	 * can take away the queues from the guests if needed.
+	 */
 }
 
 static int vfio_ap_matrix_dev_create(void)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 01c429cb51d5..461a74515d3d 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -803,6 +803,37 @@  static int vfio_ap_mdev_open_once(struct ap_matrix_mdev *matrix_mdev)
 	return ret;
 }
 
+static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev, bool force)
+{
+	int ret;
+	int rc = 0;
+	unsigned long apid, apqi;
+	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
+
+	if (!matrix_mdev->activated) {
+		pr_err("%s: reset failed for mdev %s, not activated",
+		       VFIO_AP_MODULE_NAME, matrix_mdev->name);
+
+		return -EPERM;
+	}
+
+	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
+			     matrix_mdev->matrix.apm_max + 1) {
+		for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
+				     matrix_mdev->matrix.aqm_max + 1) {
+			ret = vfio_ap_reset_queue(apid, apqi, 1);
+			if (ret) {
+				if (force)
+					rc = ret;
+				else
+					return ret;
+			}
+		}
+	}
+
+	return rc;
+}
+
 static int vfio_ap_mdev_open(struct mdev_device *mdev)
 {
 	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
@@ -860,6 +891,7 @@  static void vfio_ap_mdev_release(struct mdev_device *mdev)
 
 	mutex_lock(&matrix_dev.lock);
 	kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
+	vfio_ap_mdev_reset_queues(mdev, true);
 	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
 				 &matrix_mdev->group_notifier);
 	matrix_mdev->kvm = NULL;
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 34be9afe9ced..f11457ca90f6 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -76,4 +76,29 @@  static inline struct device *to_device(struct ap_matrix_dev *matrix_dev)
 extern int vfio_ap_mdev_register(void);
 extern void vfio_ap_mdev_unregister(void);
 
+static inline int vfio_ap_reset_queue(unsigned int apid, unsigned int apqi,
+				      unsigned int retry)
+{
+	struct ap_queue_status status;
+
+	do {
+		status = ap_zapq(AP_MKQID(apid, apqi));
+		switch (status.response_code) {
+		case AP_RESPONSE_NORMAL:
+			return 0;
+		case AP_RESPONSE_RESET_IN_PROGRESS:
+		case AP_RESPONSE_BUSY:
+			msleep(20);
+			break;
+		default:
+			pr_warn("%s: error zeroizing %02x.%04x: response code %d\n",
+				VFIO_AP_MODULE_NAME, apid, apqi,
+				status.response_code);
+			return -EIO;
+		}
+	} while (retry--);
+
+	return -EBUSY;
+}
+
 #endif /* _VFIO_AP_PRIVATE_H_ */