Re: [PATCH V2] cxl: Export library to support IBM XSL

Salut Christophe,

A few comments below, nothing major...

Le 14/06/2017 à 15:29, Christophe Lombard a écrit :

This patch exports a in-kernel 'library' API which can be called by
other drivers to help interacting with an IBM XSL on a POWER9 system.

The XSL (Translation Service Layer) is a stripped down version of the
PSL (Power Service Layer) used in some cards such as the Mellanox CX5.
Like the PSL, it implements the CAIA architecture, but has a number
of differences, mostly in it's implementation dependent registers.

The XSL also uses a special DMA cxl mode, which uses a slightly
different init sequence for the CAPP and PHB.

Changelog[v2]
  - Rebase to latest upstream.
  - Return -EFAULT in case of NULL pointer in cxllib_handle_fault().
  - Reverse parameters when copro_handle_mm_fault() is called.

The change log shouldn't be part of the commit message, but below the 
next "---"

Year need to be updated

+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/hugetlb.h>
+#include <linux/sched/mm.h>
+#include "cxl.h"
+#include <misc/cxllib.h>
+#include <asm/pnv-pci.h>

Ordering of the #include is messy:
#include <linux/hugetlb.h>
#include <linux/sched/mm.h>
#include <asm/pnv-pci.h>
#include <misc/cxllib.h>
#include "cxl.h"

+int cxllib_set_device_dma(struct pci_dev *dev, unsigned long flags)
+{
+	int rc;
+
+	if (flags)
+		return -EINVAL;
+
+	rc = dma_set_mask(&dev->dev, DMA_BIT_MASK(64));
+	return rc;
+}
+EXPORT_SYMBOL_GPL(cxllib_set_device_dma);

A comment in cxllib_set_device_dma() would help:
	/*
	 * When switching the PHB to capi mode, the TVT#1 entry for
	 * the Partitionable Endpoint is set in bypass mode, like
	 * in PCI mode.
	 * Configure the device dma to use TVT#1, which is done
	 * by calling dma_set_mask() with a mask large enough.
	 */

+
+int cxllib_get_PE_attributes(struct task_struct *task,
+	     unsigned long translation_mode, struct cxllib_pe_attributes *attr)
+{
+	struct mm_struct *mm = NULL;
+
+	if (translation_mode != CXL_TRANSLATED_MODE &&
+		translation_mode != CXL_REAL_MODE)
+		return -EINVAL;
+
+	attr->sr = cxl_calculate_sr(false /* master */,
+				task == NULL /* kernel ctx */,
+				translation_mode == CXL_REAL_MODE,
+				true /* p9 */);
+	attr->lpid = mfspr(SPRN_LPID);
+	if (task) {
+		mm = get_task_mm(task);
+		if (mm == NULL)
+			return -EINVAL;
+		/*
+		 * Caller is keeping a reference on mm_users for as long
+		 * as XSL uses the memory context
+		 */
+		attr->pid = mm->context.id;
+		mmput(mm);
+	} else {
+		attr->pid = 0;
+	}
+	attr->tid = 0;

We'll need to remember to patch that function as welll (attr->tid) when 
we add support for as_notify (even though Mellanox is not expected to 
use as_notify in capi mode, just pci I believe)

+int cxllib_handle_fault(struct mm_struct *mm, u64 addr, u64 size, u64 flags)
+{
+	int rc;
+	u64 dar;
+	struct vm_area_struct *vma = NULL;
+	unsigned long page_size;
+
+	if (mm == NULL)
+		return -EFAULT;
+
+	down_read(&mm->mmap_sem);
+
+	for (dar = addr; dar < addr + size; dar += page_size) {
+		if (!vma || dar < vma->vm_start || dar > vma->vm_end) {
+			vma = find_vma(mm, addr);
+			if (!vma) {
+				pr_err("Can't find vma for addr %016llx\n", addr);
+				rc = -EFAULT;
+				goto out;
+			}
+			/* get the size of the pages allocated */
+			page_size = vma_kernel_pagesize(vma);
+		}
+
+		rc = cxl_handle_page_fault(true, mm, flags, dar);

Why do we pass "true" for kernel parameter?
Actually do we even need a kernel input parameter for 
cxl_handle_page_fault() ? It seems that we can infer it based on mm. If 
NULL, then we are in kernel space.

Could we have that code in a wrapper before calling 
cxl_handle_page_fault()? It would keep the code cleaner and in line with 
what we do for cxl_handle_segment_miss().

Year update.

+/*
+ * Get the Process Element structure for the given thread
+ *
+ * Input:
+ *	pid: points the struct pid for the given thread (i.e. linux pid)
+ *	translation_mode: whether addresses should be translated
+ */
+struct cxllib_pe_attributes {
+	u64 sr;
+	u32 lpid;
+	u32 tid;
+	u32 pid;
+};
+#define CXL_TRANSLATED_MODE 0
+#define CXL_REAL_MODE 1
+
+int cxllib_get_PE_attributes(struct task_struct *task,
+	     unsigned long translation_mode, struct cxllib_pe_attributes *attr);

Description in comment no longer matches reality.
/*
  * Get the Process Element structure for the given thread
  *
  * Input:
  *	task: task_struct for the context of the translation
  *	translation_mode: whether addresses should be translated
  * Output:
  *	attr: attributes to fill up the Process Element structure from CAIA
  */


Thanks,

   Fred