Re: [PATCH v3 8/9] s390: ap: Cleanup on removing the AP device

[Pierre Morel]

On 16/02/2019 00:29, Tony Krowiak wrote:
> On 2/14/19 8:51 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ÂÂÂÂ | 92 
> > +++++++++++++++++++++++++++++++++++
> > Â drivers/s390/crypto/vfio_ap_ops.cÂÂÂÂ |Â 2 +-
> > Â drivers/s390/crypto/vfio_ap_private.h |Â 2 +
> > Â 3 files changed, 95 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/s390/crypto/vfio_ap_drv.c 
> > b/drivers/s390/crypto/vfio_ap_drv.c
> > index 03153e6..50428be 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"
> > @@ -64,6 +67,88 @@ static int vfio_ap_queue_dev_probe(struct ap_device 
> > *apdev)
> > ÂÂÂÂÂ return 0;
> > Â }
> > +/*
> > + * vfio_ap_drain_queue
> > + * @q: the queue to drain
> > + *
> > + * This function waits until the queue is empty.
> > + */
> > +static void vfio_ap_drain_queue(struct vfio_ap_queue *q)
> > +{
> > +ÂÂÂ struct ap_queue_status status;
> > +ÂÂÂ int retry = 20;
> > +
> > +ÂÂÂ status = ap_tapq(q->apqn, NULL);
> > +ÂÂÂ while (!status.queue_empty && retry--)Â {
> > +ÂÂÂÂÂÂÂ msleep(200);
> > +ÂÂÂÂÂÂÂ status = ap_tapq(q->apqn, NULL);
> > +ÂÂÂ }
> > +ÂÂÂ if (retry <= 0) {
> > +ÂÂÂÂÂÂÂ pr_warn("%s: queue not empty after zapq on apqn 0x%04x\n",
> > +ÂÂÂÂÂÂÂÂÂÂÂ __func__, q->apqn);
> > +ÂÂÂ }
> > +}
> > +
> > +/*
> > + * vfio_ap_zapq
> > + * @q: The queue to zerro
> > + *
> > + * It is best effort, no return value is done, however on success
> > + * we will drain the queue before getting the queue back to the
> > + * AP bus.
> > + */
> > +static void vfio_ap_zapq(struct vfio_ap_queue *q)
> > +{
> > +ÂÂÂ struct ap_queue_status status;
> > +ÂÂÂ int retry = 20;
> > +
> > +ÂÂÂ do {
> > +ÂÂÂÂÂÂÂ status = ap_zapq(q->apqn);
> > +ÂÂÂÂÂÂÂ switch (status.response_code) {
> > +ÂÂÂÂÂÂÂ case AP_RESPONSE_RESET_IN_PROGRESS:
> > +ÂÂÂÂÂÂÂ case AP_RESPONSE_BUSY:
> > +ÂÂÂÂÂÂÂÂÂÂÂ msleep(20);
> > +ÂÂÂÂÂÂÂÂÂÂÂ break;
> > +ÂÂÂÂÂÂÂ default:
> > +ÂÂÂÂÂÂÂÂÂÂÂ pr_warn("%s: zapq error %02x on apqn 0x%04x\n",
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __func__, status.response_code, q->apqn);
> > +ÂÂÂÂÂÂÂÂÂÂÂ return;
> > +ÂÂÂÂÂÂÂ case AP_RESPONSE_NORMAL:
> > +ÂÂÂÂÂÂÂÂÂÂÂ vfio_ap_drain_queue(q);
>
> I don't think this is necessary. The zeroize is performed on
> each AP-queue entry in an AP queue. My understanding is that when a
> reset or zeroize is pending, any AP instructions subsequently issued
> are rejected with condition-code 3 indicating an AP queue reset is in
> progress. It is also my understanding that once the AP commands 
> currently executing in a given AP queue entry complete, the queue
> entry will be zeroized. So it seems to me that there is no need to
> "drain" the queue, it will have already been done by the zeroize.

My understanding from the specifications is that after a RAPQ or a ZAPQ
we should wait for the queue to be really empty.

> If you agree we don't need to "drain" the queue, then I'd rather just
> make the zapq function in the vfio_ap_ops.c non-static and make it
> available to the driver. There is no sense in duplicating this code. In
> fact, even if you keep the draining function, you still don't need to
> duplicate a zaapq instruction here, you can just call the vfio_ap_ops.c
> version and then drain the queue on AP_RESPONSE_NORMAL.

OK I will see how to optimize this.



> > +ÂÂÂÂÂÂÂ return;
> > +
> > +ÂÂÂ vfio_ap_update_crycb(q);
> > +ÂÂÂ vfio_ap_zapq(q);
> > +
> > +ÂÂÂ vfio_ap_free_irq(q);
>
> If you make the zapq function in vfio_ap_ops.c available to the driver
> rather than duplicating it in this file, you won't need this call
> to vfio_ap_free_irq because it is done as part of the zapq in
> vfio_ap_ops.c.

Another solution.
OK I will look at this.

Thanks,
Pierre


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