Re: [PATCH v9 09/22] s390: vfio-ap: register matrix device with VFIO mdev framework

[Tony Krowiak]

On 08/14/2018 07:19 AM, Cornelia Huck wrote:
> On Mon, 13 Aug 2018 17:48:06 -0400
> Tony Krowiak <akrowiak@xxxxxxxxxxxxxxxxxx> wrote:
>
> > From: Tony Krowiak <akrowiak@xxxxxxxxxxxxx>
> >
> > Registers the matrix device created by the VFIO AP device
> > driver with the VFIO mediated device framework.
> > Registering the matrix device will create the sysfs
> > structures needed to create mediated matrix devices
> > each of which will be used to configure the AP matrix
> > for a guest and connect it to the VFIO AP device driver.
> >
> > Registering the matrix device with the VFIO mediated device
> > framework will create the following sysfs structures:
> >
> > /sys/devices/vfio_ap/matrix/
> > ...... [mdev_supported_types]
> > ......... [vfio_ap-passthrough]
> > ............ create
> >
> > To create a mediated device for the AP matrix device, write a UUID
> > to the create file:
> >
> > 	uuidgen > create
> >
> > A symbolic link to the mediated device's directory will be created in the
> > devices subdirectory named after the generated $uuid:
> >
> > /sys/devices/vfio_ap/matrix/
> > ...... [mdev_supported_types]
> > ......... [vfio_ap-passthrough]
> > ............ [devices]
> > ............... [$uuid]
> >
> > A symbolic link to the mediated device will also be created
> > in the vfio_ap matrix's directory:
> >
> > /sys/devices/vfio_ap/matrix/[$uuid]
> >
> > Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxx>
> > Reviewed-by: Halil Pasic <pasic@xxxxxxxxxxxxx>
> > Tested-by: Michael Mueller <mimu@xxxxxxxxxxxxx>
> > Tested-by: Farhan Ali <alifm@xxxxxxxxxxxxx>
> > Signed-off-by: Christian Borntraeger <borntraeger@xxxxxxxxxx>
> > ---
> >   MAINTAINERS                           |    1 +
> >   drivers/s390/crypto/Makefile          |    2 +-
> >   drivers/s390/crypto/vfio_ap_drv.c     |   23 ++++++
> >   drivers/s390/crypto/vfio_ap_ops.c     |  124 +++++++++++++++++++++++++++++++++
> >   drivers/s390/crypto/vfio_ap_private.h |   45 ++++++++++++
> >   include/uapi/linux/vfio.h             |    1 +
> >   6 files changed, 195 insertions(+), 1 deletions(-)
> >   create mode 100644 drivers/s390/crypto/vfio_ap_ops.c
>
> > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> > new file mode 100644
> > index 0000000..8018c2d
> > --- /dev/null
> > +++ b/drivers/s390/crypto/vfio_ap_ops.c
> > @@ -0,0 +1,124 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Adjunct processor matrix VFIO device driver callbacks.
> > + *
> > + * Copyright IBM Corp. 2018
> > + *
> > + * Author(s): Tony Krowiak <akrowiak@xxxxxxxxxxxxx>
> > + *	      Halil Pasic <pasic@xxxxxxxxxxxxx>
> > + *	      Pierre Morel <pmorel@xxxxxxxxxxxxx>
> > + */
> > +#include <linux/string.h>
> > +#include <linux/vfio.h>
> > +#include <linux/device.h>
> > +#include <linux/list.h>
> > +#include <linux/ctype.h>
> > +
> > +#include "vfio_ap_private.h"
> > +
> > +#define VFIO_AP_MDEV_TYPE_HWVIRT "passthrough"
> > +#define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
> > +
> > +static void vfio_ap_matrix_init(struct ap_config_info *info,
> > +				struct ap_matrix *matrix)
> > +{
> > +	matrix->apm_max = info->apxa ? info->Na : 63;
> > +	matrix->aqm_max = info->apxa ? info->Nd : 15;
> > +	matrix->adm_max = info->apxa ? info->Nd : 15;
> > +}
> > +
> > +static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
> > +{
> > +	struct ap_matrix_mdev *matrix_mdev;
> > +
> > +	matrix_mdev = kzalloc(sizeof(*matrix_mdev), GFP_KERNEL);
> > +	if (!matrix_mdev)
> > +		return -ENOMEM;
> > +
> > +	matrix_mdev->name = dev_name(mdev_dev(mdev));
> > +	vfio_ap_matrix_init(&matrix_dev.info, &matrix_mdev->matrix);
> > +	mdev_set_drvdata(mdev, matrix_mdev);
> > +
> > +	if (atomic_dec_if_positive(&matrix_dev.available_instances) < 0) {
> > +		kfree(matrix_mdev);
> > +		return -EPERM;
> > +	}
> Maybe move this check to the top of the function?

Please ignore my previous response to your comment. I can't move the call to
atomic_dec_if_positive() to the top of the function because it 
decrements the
available_instances and if the kzalloc() of matrix_mdev fails, then the 
value
would have to then be incremented to remain valid. What I can do is this:

1. Check the value of available_instances using atomic_read() at the top of
   the function and if it is zero, return an error.

2. Replace the call to atomic_dec_if_positive() with a call to atomic_dec()
   to decrement the available_instances.

I agree that it makes sense to return before attempting to allocate the
matrix_mdev if available_instances is zero.

> > +
> > +	mutex_lock(&matrix_dev.lock);
> > +	list_add(&matrix_mdev->list, &matrix_dev.mdev_list);
> > +	mutex_unlock(&matrix_dev.lock);
> > +
> > +	return 0;
> > +}