On Thu, 29 Oct 2020 19:29:35 -0400 Tony Krowiak <akrowiak@xxxxxxxxxxxxx> 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. > 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. > 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? Regards, Halil
