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

[Tony Krowiak]

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 <pmorel@xxxxxxxxxxxxx>
> > > ---
> > > Â 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 <akrowiak@xxxxxxxxxxxxx>
> > > + *ÂÂÂÂÂÂÂÂÂ Pierre Morel <pmorel@xxxxxxxxxxxxx>
> > > ÂÂ */
> > > Â #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