Messages in this thread | | | Subject | Re: [PATCH v11 01/14] s390/vfio-ap: No need to disable IRQ after queue reset | From | Tony Krowiak <> | Date | Fri, 30 Oct 2020 16:37:04 -0400 |
| |
On 10/30/20 1:42 PM, Halil Pasic wrote: > On Thu, 29 Oct 2020 19:29:35 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >>>> @@ -1177,7 +1166,10 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev) >>>> */ >>>> if (ret) >>>> rc = ret; >>>> - vfio_ap_irq_disable_apqn(AP_MKQID(apid, apqi)); >>>> + q = vfio_ap_get_queue(matrix_mdev, >>>> + AP_MKQID(apid, apqi)); >>>> + if (q) >>>> + vfio_ap_free_aqic_resources(q); >>> Is it safe to do vfio_ap_free_aqic_resources() at this point? I don't >>> think so. I mean does the current code (and vfio_ap_mdev_reset_queue() >>> in particular guarantee that the reset is actually done when we arrive >>> here)? BTW, I think we have a similar problem with the current code as >>> well. >> If the return code from the vfio_ap_mdev_reset_queue() function >> is zero, then yes, we are guaranteed the reset was done and the >> queue is empty. > I've read up on this and I disagree. We should discuss this offline.
Maybe you are confusing things here; my statement is specific to the return code from the vfio_ap_mdev_reset_queue() function, not the response code from the PQAP(ZAPQ) instruction. The vfio_ap_mdev_reset_queue() function issues the PQAP(ZAPQ) instruction and if the status response code is 0 indicating the reset was successfully initiated, it waits for the queue to empty. When the queue is empty, it returns 0 to indicate the queue is reset. If the queue does not become empty after a period of time, it will issue a warning (WARN_ON_ONCE) and return 0. In that case, I suppose there is no guarantee the reset was done, so maybe a change needs to be made there such as a non-zero return code.
> >> The function returns a non-zero return code if >> the reset fails or the queue the reset did not complete within a given >> amount of time, so maybe we shouldn't free AQIC resources when >> we get a non-zero return code from the reset function? >> > If the queue is gone, or broken, it won't produce interrupts or poke the > notifier bit, and we should clean up the AQIC resources.
True, which is what the code provided by this patch does; however, the AQIC resources should be cleaned up only if the KVM pointer is not NULL for reasons discussed elsewhere.
> > >> There are three occasions when the vfio_ap_mdev_reset_queues() >> is called: >> 1. When the VFIO_DEVICE_RESET ioctl is invoked from userspace >> (i.e., when the guest is started) >> 2. When the mdev fd is closed (vfio_ap_mdev_release()) >> 3. When the mdev is removed (vfio_ap_mdev_remove()) >> >> The IRQ resources are initialized when the PQAP(AQIC) >> is intercepted to enable interrupts. This would occur after >> the guest boots and the AP bus initializes. So, 1 would >> presumably occur before that happens. I couldn't find >> anywhere in the AP bus or zcrypt code where a PQAP(AQIC) >> is executed to disable interrupts, so my assumption is >> that IRQ disablement is accomplished by a reset on >> the guest. I'll have to ask Harald about that. So, 2 would >> occur when the guest is about to terminate and 3 >> would occur only after the guest is terminated. In any >> case, it seems that IRQ resources should be cleaned up. >> Maybe it would be more appropriate to do that in the >> vfio_ap_mdev_release() and vfio_ap_mdev_remove() >> functions themselves? > I'm a bit confused. But I think you are wrong. What happens when the > guest reIPLs? I guess the subsystem reset should also do the > VFIO_DEVICE_RESET ioctl, and that has to reset the queues and disable > the interrupts. Or?
What did I say that is wrong? I think you are referring to my statement about the VFIO_DEVICE_RESET ioctl. I am not knowledgeable about all of the circumstances under which the VFIO_DEVICE_RESET ioctl is invoked, but I know for a fact that it is invoked when the guest is started as I've verified that via tracing. On the other hand, I suspect you are correct in assuming it is also invoked on a subsystem reset from the guest, so that also argues for cleaning up the IRQ resources after a reset as long as the KVM pointer is valid.
> > Regards, > Halil >
| |