Re: [PATCH 3/7] s390/vfio_ap: use TAPQ to verify reset in progress completes

[Anthony Krowiak]

On 12/15/22 5:51 AM, Harald Freudenberger wrote:
> On 2022-12-13 16:44, Tony Krowiak wrote:
> > To eliminate the repeated calls to the PQAP(ZAPQ) function to verify 
> > that
> > a reset in progress completed successfully and ensure that error 
> > response
> > codes get appropriately logged, let's call the apq_reset_check() 
> > function
> > when the ZAPQ response code indicates that a reset that is already in
> > progress.
> >
> > Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxx>
> > ---
> >  drivers/s390/crypto/vfio_ap_ops.c | 24 +++++++++++++-----------
> >  1 file changed, 13 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/s390/crypto/vfio_ap_ops.c
> > b/drivers/s390/crypto/vfio_ap_ops.c
> > index a5530a46cf31..5bf2d93ae8af 100644
> > --- a/drivers/s390/crypto/vfio_ap_ops.c
> > +++ b/drivers/s390/crypto/vfio_ap_ops.c
> > @@ -33,7 +33,7 @@
> >  static int vfio_ap_mdev_reset_queues(struct ap_queue_table *qtable);
> >  static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
> >  static const struct vfio_device_ops vfio_ap_matrix_dev_ops;
> > -static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q, unsigned
> > int retry);
> > +static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q);
> >
> >  /**
> >   * get_update_locks_for_kvm: Acquire the locks required to 
> > dynamically update a
> > @@ -1632,8 +1632,7 @@ static int apq_reset_check(struct vfio_ap_queue 
> > *q)
> >      return ret;
> >  }
> >
> > -static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q,
> > -                    unsigned int retry)
> > +static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
> >  {
> >      struct ap_queue_status status;
> >      int ret;
> > @@ -1648,12 +1647,15 @@ static int vfio_ap_mdev_reset_queue(struct
> > vfio_ap_queue *q,
> >          ret = 0;
> >          break;
> >      case AP_RESPONSE_RESET_IN_PROGRESS:
> > -        if (retry--) {
> > -            msleep(20);
> > -            goto retry_zapq;
> > -        }
> > -        ret = -EBUSY;
> > -        break;
> > +        /*
> > +         * There is a reset issued by another process in progress. 
> > Let's wait
> > +         * for that to complete. Since we have no idea whether it 
> > was a RAPQ or
> > +         * ZAPQ, then if it completes successfully, let's issue the 
> > ZAPQ.
> > +         */
> > +        ret = apq_reset_check(q);
> > +        if (ret)
> > +            break;
> > +        goto retry_zapq;
> >      case AP_RESPONSE_Q_NOT_AVAIL:
> >      case AP_RESPONSE_DECONFIGURED:
> >      case AP_RESPONSE_CHECKSTOPPED:
> > @@ -1688,7 +1690,7 @@ static int vfio_ap_mdev_reset_queues(struct
> > ap_queue_table *qtable)
> >      struct vfio_ap_queue *q;
> >
> >      hash_for_each(qtable->queues, loop_cursor, q, mdev_qnode) {
> > -        ret = vfio_ap_mdev_reset_queue(q, 1);
> > +        ret = vfio_ap_mdev_reset_queue(q);
> >          /*
> >           * Regardless whether a queue turns out to be busy, or
> >           * is not operational, we need to continue resetting
> > @@ -1931,7 +1933,7 @@ void vfio_ap_mdev_remove_queue(struct ap_device 
> > *apdev)
> >          }
> >      }
> >
> > -    vfio_ap_mdev_reset_queue(q, 1);
> > +    vfio_ap_mdev_reset_queue(q);
> >      dev_set_drvdata(&apdev->device, NULL);
> >      kfree(q);
> >      release_update_locks_for_mdev(matrix_mdev);
>
> Reviewed-by: Harald Freudenberger <freude@xxxxxxxxxxxxx>


Thanks for the review.