Re: [PATCH v6 16/17] powerpc/vas: Implement a simple FTW driver

Hi Suka,

Some comments inline ...


Sukadev Bhattiprolu [off-list ref] writes:

The Fast Thread Wake-up (FTW) driver provides user space applications an
interface to the Core-to-Core functionality in POWER9. The driver provides
the device node/ioctl API to applications and uses the external interfaces
to the VAS driver to interact with the VAS hardware.

A follow-on patch provides detailed description of the API for the driver.

Signed-off-by: Sukadev Bhattiprolu <redacted>
---
 MAINTAINERS                             |   1 +
 arch/powerpc/platforms/powernv/Kconfig  |  16 ++
 arch/powerpc/platforms/powernv/Makefile |   1 +
 arch/powerpc/platforms/powernv/nx-ftw.c | 486 ++++++++++++++++++++++++++++++++

AFAICS this has nothing to do with NX, so why is it called nx-ftw ?

Also aren't we going to want to use this on pseries eventually? If so
should it go in arch/powerpc/sysdev ?

Missing license header.

+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/export.h>
+#include <asm/cputable.h>
+#include <linux/device.h>
+#include <linux/debugfs.h>
+#include <linux/cdev.h>
+#include <linux/mutex.h>
+#include <linux/fs.h>
+#include <linux/mm.h>
+#include <linux/slab.h>
+#include <linux/sched.h>
+#include <linux/uaccess.h>
+#include <linux/bootmem.h>
+#include <asm/opal-api.h>
+#include <asm/opal.h>
+#include <asm/page.h>
+#include <asm/vas.h>
+#include <asm/reg.h>

Please try and trim the list to what you need.

+
+/*
+ * NX-FTW is a device driver used to provide user space access to the
+ * Core-to-Core aka Fast Thread Wakeup (FTW) functionality provided by
+ * the Virtual Accelerator Subsystem (VAS) in POWER9 systems. See also
+ * arch/powerpc/platforms/powernv/vas*.
+ *
+ * The driver creates the device node /dev/crypto/nx-ftw that can be
+ * used as follows:
+ *
+ *	fd = open("/dev/crypto/nx-ftw", O_RDWR);
+ *	rc = ioctl(fd, VAS_RX_WIN_OPEN, &rxattr);
+ *	rc = ioctl(fd, VAS_TX_WIN_OPEN, &txattr);
+ *	paste_addr = mmap(NULL, PAGE_SIZE, prot, MAP_SHARED, fd, 0ULL).
+ *	vas_copy(&crb, 0, 1);
+ *	vas_paste(paste_addr, 0, 1);
+ *
+ * where "vas_copy" and "vas_paste" are defined in copy-paste.h.
+ */
+
+static char		*nxftw_dev_name = "nx-ftw";
+static atomic_t		nxftw_instid = ATOMIC_INIT(0);
+static dev_t		nxftw_devt;
+static struct dentry	*nxftw_debugfs;
+static struct class	*nxftw_dbgfs_class;

The class doesn't go in debugfs, which is what "dbgfs" says to me.

+/*
+ * Wrapper object for the nx-ftw device node - there is just one

Just "device".

"device node" is ambiguous vs device tree.

+ * instance of this node for the whole system.

So why not put the globals above in here also?

+ */
+struct nxftw_dev {
+	struct cdev cdev;
+	struct device *device;
+	char *name;
+	atomic_t refcount;
+} nxftw_device;
+
+/*
+ * One instance per open of a nx-ftw device. Each nxftw_instance is
+ * associated with a VAS window, after the caller issues VAS_RX_WIN_OPEN
+ * or VAS_TX_WIN_OPEN ioctl.
+ */
+struct nxftw_instance {
+	int instance;
+	bool tx_win;
+	struct vas_window *window;
+};
+
+#define VAS_DEFAULT_VAS_ID	0
+#define POWERNV_LPID		0	/* TODO: For VM/KVM guests? */

mfspr(SPRN_LPID)

would seem to do the trick?

+static char *nxftw_devnode(struct device *dev, umode_t *mode)
+{
+	return kasprintf(GFP_KERNEL, "crypto/%s", dev_name(dev));

This isn't a crypto device?

+}
+
+static int nxftw_open(struct inode *inode, struct file *fp)
+{
+	int minor;
+	struct nxftw_instance *nxti;

instance would be a better name.

+	minor = MINOR(inode->i_rdev);

Not used?

+	nxti = kzalloc(sizeof(*nxti), GFP_KERNEL);
+	if (!nxti)
+		return -ENOMEM;
+
+	nxti->instance = atomic_inc_return(&nxftw_instid);

And this would read better if the variable was "id". eg.

	instance->id = atomic_inc_return(&next_instance_id);

+	nxti->window = NULL;
+
+	fp->private_data = nxti;
+	return 0;
+}
+
+static int validate_txwin_user_attr(struct vas_tx_win_open_attr *uattr)
+{
+	int i;
+
+	if (uattr->version != 1)
+		return -EINVAL;
+
+	if (uattr->flags & ~VAS_FLAGS_HIGH_PRI)
+		return -EINVAL;
+
+	if (uattr->reserved1 || uattr->reserved2)
+		return -EINVAL;
+
+	for (i = 0; i < sizeof(uattr->reserved3) / sizeof(uint64_t); i++) {
+		if (uattr->reserved3[i])
+			return -EINVAL;
+	}

That struct is a mess and needs to be reworked.

+	return 0;
+}
+
+static bool validate_rxwin_user_attr(struct vas_rx_win_open_attr *uattr)
+{
+	int i;
+
+	if (uattr->version != 1)
+		return -EINVAL;
+
+	for (i = 0; i < sizeof(uattr->reserved) / sizeof(uint64_t); i++) {
+		if (uattr->reserved[i])
+			return -EINVAL;
+	}

Ditto.

+	return 0;
+}
+
+#ifdef vas_debug

This is dead code, which makes it very easy for it to get out of sync
with the vas_rx_win_attr for example.

Better to just make these pr_debug() in the only caller, that way they
get type checked.

+static inline void dump_rx_win_attr(struct vas_rx_win_attr *attr)
+{
+	pr_err("NX-FTW: user %d, nx %d, fault %d, ntfy %d, intr %d early %d\n",
+			attr->user_win ? 1 : 0,
+			attr->nx_win ? 1 : 0,
+			attr->fault_win ? 1 : 0,
+			attr->notify_disable ? 1 : 0,
+			attr->intr_disable ? 1 : 0,
+			attr->notify_early ? 1 : 0);
+
+	pr_err("NX-FTW: rx_fifo %p, rx_fifo_size %d, max value 0x%x\n",
+			attr->rx_fifo, attr->rx_fifo_size,
+			VAS_RX_FIFO_SIZE_MAX);
+
+}
+#else
+static inline void dump_rx_win_attr(struct vas_rx_win_attr *attr)
+{
+}
+#endif
+
+static int nxftw_ioc_open_rx_window(struct file *fp, unsigned long arg)
+{
+	int rc;
+	struct vas_rx_win_open_attr uattr;
+	struct vas_rx_win_attr rxattr;
+	struct nxftw_instance *nxti = fp->private_data;
+	struct vas_window *win;

	struct vas_rx_win_open_attr uattr;
	struct vas_rx_win_attr rxattr;
	struct nxftw_instance *nxti;
	struct vas_window *win;
	int rc;

        nxti = fp->private_data;

Ah much better :)

Aka. reverse-christmas-tree.

+
+	rc = copy_from_user(&uattr, (void *)arg, sizeof(uattr));

Nicer would be:

	void __user *uptr = (void *)arg;

	rc = copy_from_user(&uattr, uptr, sizeof(uattr));

+	if (rc) {
+		pr_devel("%s(): copy_from_user() returns %d\n", __func__, rc);
+		return -EFAULT;
+	}
+
+	rc = validate_rxwin_user_attr(&uattr);
+	if (rc)
+		return rc;
+
+	memset(&rxattr, 0, sizeof(rxattr));
+
+	rxattr.lnotify_lpid = POWERNV_LPID;
+
+	/*
+	 * Only caller can own the window for now. Not sure if there is need
+	 * for process P1 to make P2 the owner of a window. If so, we need to
+	 * find P2, make sure we have permissions, get a reference etc.
+	 */
+	rxattr.lnotify_pid = mfspr(SPRN_PID);
+	rxattr.lnotify_tid = mfspr(SPRN_TIDR);
+	rxattr.rx_fifo = NULL;
+	rxattr.rx_fifo_size = 0;
+	rxattr.intr_disable = true;
+	rxattr.user_win = true;

vas_init_rx_win_attr() ?

+
+	dump_rx_win_attr(&rxattr);
+
+	/*
+	 * TODO: Rather than the default vas id, choose an instance of VAS
+	 *	 based on the chip the caller is running.
+	 */

Seems like that will be a common pattern so maybe the vas core should
handle it for callers who want it.

+	win = vas_rx_win_open(VAS_DEFAULT_VAS_ID, VAS_COP_TYPE_FTW, &rxattr);
+	if (IS_ERR(win)) {
+		pr_devel("%s() vas_rx_win_open() failed, %ld\n", __func__,
+				PTR_ERR(win));
+		return PTR_ERR(win);
+	}
+
+	nxti->window = win;
+	uattr.rx_win_handle = vas_win_id(win);
+
+	rc = copy_to_user((void *)arg, &uattr, sizeof(uattr));
+	if (rc) {
+		pr_devel("%s(): copy_to_user() failed, %d\n", __func__, rc);
+		return -EFAULT;
+	}

You defined the ioctl as:

  #define VAS_RX_WIN_OPEN		_IOW('v', 2, struct vas_rx_win_open_attr)

But you're reading and writing from the user arg, so it should be _IOWR.

+
+	return 0;
+}
+
+static int nxftw_ioc_open_tx_window(struct file *fp, unsigned long arg)
+{
+	int rc;
+	enum vas_cop_type cop;
+	struct vas_window *win;
+	struct vas_tx_win_open_attr uattr;
+	struct vas_tx_win_attr txattr;

Those two struct names are quite confusing.

+	struct nxftw_instance *nxti = fp->private_data;
+
+	rc = copy_from_user(&uattr, (void *)arg, sizeof(uattr));
+	if (rc) {
+		pr_devel("%s(): copy_from_user() failed, %d\n", __func__, rc);
+		return -EFAULT;
+	}

All you use is rx_win_handle, so why does this ioctl take the whole struct?

+	cop = VAS_COP_TYPE_FTW;
+
+	rc = validate_txwin_user_attr(&uattr);
+	if (rc)
+		return rc;
+
+	pr_devel("Pid %d: Opening txwin, cop %d, PIDR %ld\n",
+				task_pid_nr(current), cop, mfspr(SPRN_PID));
+
+	vas_init_tx_win_attr(&txattr, cop);
+
+	txattr.lpid = POWERNV_LPID;
+	txattr.pidr = mfspr(SPRN_PID);
+	txattr.pid = task_pid_nr(current);

Why is that in txattr?

The pid can be freed and given to another process so it's fishy to be
saving the pid without also holding a reference on the task.

+	txattr.user_win = true;

Has been done for us.

+	txattr.pswid = uattr.rx_win_handle;
+
+	win = vas_tx_win_open(VAS_DEFAULT_VAS_ID, cop, &txattr);
+	if (IS_ERR(win)) {
+		pr_devel("%s() vas_tx_win_open() failed, %ld\n", __func__,
+					PTR_ERR(win));
+		return PTR_ERR(win);
+	}
+	nxti->window = win;
+	nxti->tx_win = true;

is_tx would be clearer IMHO.

+	return 0;
+}
+
+static int nxftw_release(struct inode *inode, struct file *fp)
+{
+	struct nxftw_instance *nxti;
+
+	nxti = fp->private_data;
+
+	vas_win_close(nxti->window);
+	nxti->window = NULL;
+
+	kfree(nxti);
+	fp->private_data = NULL;

Flipping the order of those would be preferable though it's not actually
a bug.

+	atomic_dec(&nxftw_instid);
+
+	return 0;
+}
+
+static ssize_t nxftw_write(struct file *fp, const char __user *buf,
+			size_t len, loff_t *offsetp)
+{
+	return -ENOTSUPP;
+}
+
+static ssize_t nxftw_read(struct file *fp, char __user *buf, size_t len,
+			loff_t *offsetp)
+{
+	return -ENOTSUPP;
+}

Do you need those?

+static int nxftw_vma_fault(struct vm_fault *vmf)
+{
+	u64 offset;
+	unsigned long vaddr;
+	uint64_t pbaddr_start;
+	struct nxftw_instance *nxti;
+	struct vm_area_struct *vma = vmf->vma;
+
+	nxti = vma->vm_private_data;
+	offset = vmf->pgoff << PAGE_SHIFT;
+	vaddr = (unsigned long)vmf->address;
+
+	pbaddr_start = vas_win_paste_addr(nxti->window);
+
+	pr_devel("%s() instance %d, pbaddr 0x%llx, vaddr 0x%lx,"
+			"offset %llx, pgoff 0x%lx, vma-start 0x%zx,"
+			"size %zd\n", __func__, nxti->instance,
+			pbaddr_start, vaddr, offset, vmf->pgoff,
+			vma->vm_start, vma->vm_end-vma->vm_start);
+
+	vm_insert_pfn(vma, vaddr, (pbaddr_start + offset) >> PAGE_SHIFT);
+
+	return VM_FAULT_NOPAGE;
+}
+
+const struct vm_operations_struct nxftw_vm_ops = {
+	.fault = nxftw_vma_fault,
+};

Is there some particular reason you need to implement those, you appear
to be just mapping a page into the address space. Can't you just use
remap_pfn_range() in your mmap routine?

+static int nxftw_mmap(struct file *fp, struct vm_area_struct *vma)
+{
+	struct nxftw_instance *nxti = fp->private_data;
+
+	if ((vma->vm_end - vma->vm_start) > PAGE_SIZE) {
+		pr_devel("%s(): size 0x%zx, PAGE_SIZE 0x%zx\n", __func__,
+				(vma->vm_end - vma->vm_start), PAGE_SIZE);
+		return -EINVAL;
+	}
+
+	/* Ensure instance has an open send window */
+	if (!nxti->window || !nxti->tx_win) {
+		pr_devel("%s(): No send window open?\n", __func__);
+		return -EINVAL;
+	}
+
+	/* flags, page_prot from cxl_mmap(), except we want cachable */
+	vma->vm_flags |= VM_IO | VM_PFNMAP;
+	vma->vm_page_prot = pgprot_cached(vma->vm_page_prot);
+
+	vma->vm_ops = &nxftw_vm_ops;
+	vma->vm_private_data = nxti;

ie. here.

See eg. opal-prd.c for an example.

+	return 0;
+}
+
+static long nxftw_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
+{
+	struct nxftw_instance *nxti;
+
+	nxti = fp->private_data;

Not used.

+
+	pr_devel("%s() cmd 0x%x, TX_WIN_OPEN 0x%lx\n", __func__, cmd,
+			VAS_TX_WIN_OPEN);

Can we drop that?

+	switch (cmd) {
+
+	case VAS_TX_WIN_OPEN:
+		return nxftw_ioc_open_tx_window(fp, arg);
+
+	case VAS_RX_WIN_OPEN:
+		return nxftw_ioc_open_rx_window(fp, arg);
+
+	default:
+		return -EINVAL;
+	}
+}
+
+const struct file_operations nxftw_fops = {
+	.owner = THIS_MODULE,
+	.open = nxftw_open,
+	.release = nxftw_release,
+	.read = nxftw_read,
+	.write = nxftw_write,
+	.mmap = nxftw_mmap,
+	.unlocked_ioctl = nxftw_ioctl,
+};
+
+
+int nxftw_file_init(void)
+{
+	int rc;
+	dev_t devno;
+
+	rc = alloc_chrdev_region(&nxftw_devt, 1, 1, "nx-ftw");
+	if (rc) {
+		pr_err("Unable to allocate nxftw major number: %i\n", rc);
+		return rc;
+	}
+
+	pr_devel("NX-FTW device allocated, dev [%i,%i]\n", MAJOR(nxftw_devt),
+			MINOR(nxftw_devt));
+
+	nxftw_dbgfs_class = class_create(THIS_MODULE, "nxftw");
+	if (IS_ERR(nxftw_dbgfs_class)) {
+		pr_err("Unable to create NX-FTW class\n");
+		rc = PTR_ERR(nxftw_dbgfs_class);
+		goto err;
+	}
+	nxftw_dbgfs_class->devnode = nxftw_devnode;
+
+	cdev_init(&nxftw_device.cdev, &nxftw_fops);
+
+	devno = MKDEV(MAJOR(nxftw_devt), 0);
+	if (cdev_add(&nxftw_device.cdev, devno, 1)) {
+		pr_err("NX-FTW: cdev_add() failed\n");
+		goto err;
+	}
+
+	nxftw_device.device = device_create(nxftw_dbgfs_class, NULL,
+			devno, NULL, nxftw_dev_name, MINOR(devno));
+	if (IS_ERR(nxftw_device.device)) {
+		pr_err("Unable to create nxftw-%d\n", MINOR(devno));
+		goto err;
+	}
+
+	pr_devel("%s: Added dev [%d,%d]\n", __func__, MAJOR(devno),
+			MINOR(devno));
+	return 0;
+
+err:
+	unregister_chrdev_region(nxftw_devt, 1);
+	return rc;
+}
+
+void nxftw_file_exit(void)
+{
+	dev_t devno;
+
+	pr_devel("NX-FTW: %s entered\n", __func__);
+
+	cdev_del(&nxftw_device.cdev);
+	devno = MKDEV(MAJOR(nxftw_devt), MINOR(nxftw_devt));
+	device_destroy(nxftw_dbgfs_class, devno);
+
+	class_destroy(nxftw_dbgfs_class);
+	unregister_chrdev_region(nxftw_devt, 1);
+}
+
+
+/*
+ * Create a debugfs entry. Not sure what for yet, though
+ */

Please just drop it.

+int __init nxftw_debugfs_init(void)
+{
+	struct dentry *ent;
+
+	ent = debugfs_create_dir("nxftw", NULL);
+	if (IS_ERR(ent)) {
+		pr_devel("nxftw: %s(): error creating dbgfs dir\n", __func__);
+		return PTR_ERR(ent);
+	}
+	nxftw_debugfs = ent;
+
+	return 0;
+}
+
+void nxftw_debugfs_exit(void)
+{
+	debugfs_remove_recursive(nxftw_debugfs);
+}
+
+int __init nxftw_init(void)
+{
+	int rc;
+
+	rc = nxftw_file_init();
+	if (rc)
+		return rc;
+
+	rc = nxftw_debugfs_init();
+	if (rc)
+		goto free_file;
+
+	pr_err("NX-FTW Device initialized\n");

That's not an error.

+
+	return 0;
+
+free_file:
+	nxftw_file_exit();
+	return rc;
+}
+
+void __init nxftw_exit(void)
+{
+	pr_devel("NX-FTW Device exiting\n");
+	nxftw_debugfs_exit();
+	nxftw_file_exit();
+}
+
+module_init(nxftw_init);
+module_exit(nxftw_exit);

This can't be a module, so you shouldn't be using these.

Or these:

+MODULE_DESCRIPTION("IBM NX Fast Thread Wakeup Device");
+MODULE_AUTHOR("Sukadev Bhattiprolu [off-list ref]");
+MODULE_LICENSE("GPL");

cheers