Re: [PATCH v2 2/3] vfio/ism: Implement vfio_pci driver for ISM devices

[Alexandra Winter <wintera@linux.ibm.com>]

On 24.02.26 13:34, Julian Ruess wrote:
[...]
> diff --git a/drivers/vfio/pci/ism/main.c b/drivers/vfio/pci/ism/main.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..5f9674f6dd1d44888c4e1e416d05edfd89fd09fe
> --- /dev/null
> +++ b/drivers/vfio/pci/ism/main.c
> @@ -0,0 +1,297 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * vfio-ISM driver for s390
> + *
> + * Copyright IBM Corp.
> + */
> +
> +#include "../vfio_pci_priv.h"
> +
> +#define ISM_VFIO_PCI_OFFSET_SHIFT   48
> +#define ISM_VFIO_PCI_OFFSET_TO_INDEX(off) (off >> ISM_VFIO_PCI_OFFSET_SHIFT)
> +#define ISM_VFIO_PCI_INDEX_TO_OFFSET(index) ((u64)(index) << ISM_VFIO_PCI_OFFSET_SHIFT)
> +#define ISM_VFIO_PCI_OFFSET_MASK (((u64)(1) << ISM_VFIO_PCI_OFFSET_SHIFT) - 1)
> +
> +struct ism_vfio_pci_core_device {
> +	struct vfio_pci_core_device core_device;
> +};
> +
> +static int ism_pci_open_device(struct vfio_device *core_vdev)
> +{
> +	struct ism_vfio_pci_core_device *ivdev;
> +	struct vfio_pci_core_device *vdev;


Why do you need 'struct ism_vfio_pci_core_device'?
Unlike other vfio_pci variant drivers your struct only has a single member.
I see it is used below as well, but couldn't you directly use 'struct vfio_pci_core_device'
in all places?


> +	int ret;
> +
> +	ivdev = container_of(core_vdev, struct ism_vfio_pci_core_device,
> +			     core_device.vdev);
> +	vdev = &ivdev->core_device;
> +
> +	ret = vfio_pci_core_enable(vdev);
> +	if (ret)
> +		return ret;
> +
> +	vfio_pci_core_finish_enable(vdev);
> +	return 0;
> +}
> +
> +static ssize_t ism_vfio_pci_do_io_r(struct vfio_pci_core_device *vdev,
> +				    char __user *buf, loff_t off, size_t count,
> +				    int bar)
> +{
> +	struct zpci_dev *zdev = to_zpci(vdev->pdev);
> +	ssize_t ret, done = 0;
> +	u64 req, length, tmp;
> +
> +	while (count) {
> +		if (count >= 8 && IS_ALIGNED(off, 8))
> +			length = 8;
> +		else if (count >= 4 && IS_ALIGNED(off, 4))
> +			length = 4;
> +		else if (count >= 2 && IS_ALIGNED(off, 2))
> +			length = 2;
> +		else
> +			length = 1;

As you know something like
> +		if (count >= 8 && IS_ALIGNED(off, 8)) {
> +			length = 8;
		} else {
			unsigned missing_bytes = 8 - (off % 8);
			length = min(count, missing_bytes);
		}

would suffice to fullfill the requirements for pcilg to BARs.
But your code works as well.

> +		req = ZPCI_CREATE_REQ(READ_ONCE(zdev->fh), bar, length);
> +		/* use pcilg to prevent using MIO instructions */
> +		ret = __zpci_load(&tmp, req, off);
> +		if (ret)
> +			return ret;
> +		if (copy_to_user(buf, &tmp, length))
> +			return -EFAULT;
> +		count -= length;
> +		done += length;
> +		off += length;
> +		buf += length;
> +	}
> +	return done;
> +}
> +
> +static ssize_t ism_vfio_pci_do_io_w(struct vfio_pci_core_device *vdev,
> +				    char __user *buf, loff_t off, size_t count,
> +				    int bar)
> +{
> +	struct zpci_dev *zdev = to_zpci(vdev->pdev);
> +	void *data __free(kfree) = NULL;
> +	ssize_t ret;
> +	u64 req;
> +
> +	if (count > zdev->maxstbl)
> +		return -EINVAL;

I think you also need to check for off+count not crossing 4k


> +	data = kzalloc(count, GFP_KERNEL);


Where do you free 'data' ?

> +	if (!data)
> +		return -ENOMEM;
> +	if (copy_from_user(data, buf, count))
> +		return -EFAULT;
> +	req = ZPCI_CREATE_REQ(READ_ONCE(zdev->fh), bar, count);
 > +	ret = __zpci_store_block(data, req, off);
> +	if (ret)
> +		return ret;
> +	return count;
> +}
> +