Re: [PATCH v10 02/16] s390/vfio-ap: use new AP bus interface to search for queue devices

[Tony Krowiak]

On 9/24/20 10:11 PM, Halil Pasic wrote:
> On Thu, 27 Aug 2020 10:24:07 -0400
> Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
>
> > On 8/25/20 6:13 AM, Cornelia Huck wrote:
> > > On Fri, 21 Aug 2020 15:56:02 -0400
> > > Tony Krowiak<akrowiak@xxxxxxxxxxxxx>  wrote:
> > >
> > > > This patch refactor's the vfio_ap device driver to use the AP bus's
> > > s/refactor's/refactors/
> > Of course, what was I thinking?:)
> >
> > > > ap_get_qdev() function to retrieve the vfio_ap_queue struct containing
> > > > information about a queue that is bound to the vfio_ap device driver.
> > > > The bus's ap_get_qdev() function retrieves the queue device from a
> > > > hashtable keyed by APQN. This is much more efficient than looping over
> > > > the list of devices attached to the AP bus by several orders of
> > > > magnitude.
> > > >
> > > > Signed-off-by: Tony Krowiak<akrowiak@xxxxxxxxxxxxx>
> > > > Reported-by: kernel test robot<lkp@xxxxxxxxx>
> > > > ---
> > > >    drivers/s390/crypto/vfio_ap_drv.c     | 27 ++-------
> > > >    drivers/s390/crypto/vfio_ap_ops.c     | 86 +++++++++++++++------------
> > > >    drivers/s390/crypto/vfio_ap_private.h |  8 ++-
> > > >    3 files changed, 59 insertions(+), 62 deletions(-)
> > > (...)
> > >
> > > > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> > > > index e0bde8518745..ad3925f04f61 100644
> > > > --- a/drivers/s390/crypto/vfio_ap_ops.c
> > > > +++ b/drivers/s390/crypto/vfio_ap_ops.c
> > > > @@ -26,43 +26,26 @@vfio_ap_get_queue()
> > > >    
> > > >    static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
> > > >    
> > > > -static int match_apqn(struct device *dev, const void *data)
> > > > -{
> > > > -	struct vfio_ap_queue *q = dev_get_drvdata(dev);
> > > > -
> > > > -	return (q->apqn == *(int *)(data)) ? 1 : 0;
> > > > -}
> > > > -
> > > >    /**
> > > > - * vfio_ap_get_queue: Retrieve a queue with a specific APQN from a list
> > > > - * @matrix_mdev: the associated mediated matrix
> > > > + * vfio_ap_get_queue: Retrieve a queue with a specific APQN.
> > > >     * @apqn: The queue APQN
> > > >     *
> > > > - * Retrieve a queue with a specific APQN from the list of the
> > > > - * devices of the vfio_ap_drv.
> > > > - * Verify that the APID and the APQI are set in the matrix.
> > > > + * Retrieve a queue with a specific APQN from the AP queue devices attached to
> > > > + * the AP bus.
> > > >     *
> > > > - * Returns the pointer to the associated vfio_ap_queue
> > > > + * Returns the pointer to the vfio_ap_queue with the specified APQN, or NULL.
> > > >     */
> > > > -static struct vfio_ap_queue *vfio_ap_get_queue(
> > > > -					struct ap_matrix_mdev *matrix_mdev,
> > > > -					int apqn)
> > > > +static struct vfio_ap_queue *vfio_ap_get_queue(unsigned long apqn)
> > > >    {
> > > > +	struct ap_queue *queue;
> > > >    	struct vfio_ap_queue *q;
> > > > -	struct device *dev;
> > > >    
> > > > -	if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm))
> > > > -		return NULL;
> > > > -	if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
> > > I think you should add some explanation to the patch description why
> > > testing the matrix bitmasks is not needed anymore.
> > As a result of this comment, I took a closer look at the code to
> > determine the reason for eliminating the matrix_mdev
> > parameter. The reason is because the code below (i.e., find the device
> > and get the driver data) was also repeated in the vfio_ap_irq_disable_apqn()
> > function, so I replaced it with a call to the function above; however, the
> > vfio_ap_irq_disable_apqn() function  does not have a reference to the
> > matrix_mdev, so I eliminated the matrix_mdev parameter. Note that the
> > vfio_ap_irq_disable_apqn() is called for each APQN assigned to a matrix
> > mdev, so there is no need to test the bitmasks there.
> >
> > The other place from which the function above is called is
> > the handle_pqap() function which does have a reference to the
> > matrix_mdev. In order to ensure the integrity of the instruction
> > being intercepted - i.e., PQAP(AQIC) enable/disable IRQ for aN
> > AP queue - the testing of the matrix bitmasks probably ought to
> > be performed, so it will be done there instead of in the
> > vfio_ap_get_queue() function above.
> I'm a little confused. I do agree that in handle_pqap() we do want to
> make sure that we only operate on queues that belong to the given guest
> that issued the PQAP instruction.
>
> AFAICT with this patch set applied, this is not the case any more. Does
> that 'will be done there instead' refer to v11?

I understand your confusion, so here is what I'm going to do
to clear things up. I will leave the signature of the vfio_ap_get_queue()
function the same and leave in the bitmap checking. As per your
comment below, in patch 3 I will replace the call to
vfio_ap_get_queue() with a call to vfio_ap_get_mdev_queue().
Since the vfio_ap_get_mdev_queue() function is mdev-specific,
I can then remove the mdev parameter from the
vfio_ap_get_queue() function since it will no longer be needed.

> Another question is, can we use vfio_ap_get_mdev_queue() in
> handle_pqap() (instead of vfio_ap_get_queue())?

Yes, we can and should do that as it will eliminate both the need to
test the matrix bitmasks and several lines of code; however, that
function is not available until patch 3/16, so that change will be
made there.

>   
> > > +	queue = ap_get_qdev(apqn);
> > > +	if (!queue)
> > >    		return NULL;
> > >    
> > > -	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
> > > -				 &apqn, match_apqn);
> > > -	if (!dev)
> > > -		return NULL;
> > > -	q = dev_get_drvdata(dev);
> > > -	q->matrix_mdev = matrix_mdev;
> > > -	put_device(dev);
> > > +	q = dev_get_drvdata(&queue->ap_dev.device);
> > > +	put_device(&queue->ap_dev.device);
> > >    
> > >    	return q;
> > >    }
> > > (...)