[PATCH v8 00/14] powerpc/vas: Page fault handling for user space NX requests

[PATCH v8 00/14] powerpc/vas: Page fault handling for user space NX requests

[Haren Myneni]

On power9, Virtual Accelerator Switchboard (VAS) allows user space or
kernel to communicate with Nest Accelerator (NX) directly using COPY/PASTE
instructions. NX provides various functionalities such as compression,
encryption and etc. But only compression (842 and GZIP formats) is
supported in Linux kernel on power9.

842 compression driver (drivers/crypto/nx/nx-842-powernv.c)
is already included in Linux. Only GZIP support will be available from
user space.

Applications can issue GZIP compression / decompression requests to NX with
COPY/PASTE instructions. When NX is processing these requests, can hit
fault on the request buffer (not in memory). It issues an interrupt and
pastes fault CRB in fault FIFO. Expects kernel to handle this fault and
return credits for both send and fault windows after processing.

This patch series adds IRQ and fault window setup, and NX fault handling:
- Alloc IRQ and trigger port address, and configure IRQ per VAS instance.
- Set port# for each window to generate an interrupt when noticed fault.
- Set fault window and FIFO on which NX paste fault CRB.
- Setup IRQ thread fault handler per VAS instance.
- When receiving an interrupt, Read CRBs from fault FIFO and update
  coprocessor_status_block (CSB) in the corresponding CRB with translation
  failure (CSB_CC_TRANSLATION). After issuing NX requests, process polls
  on CSB address. When it sees translation error, can touch the request
  buffer to bring the page in to memory and reissue NX request.
- If copy_to_user fails on user space CSB address, OS sends SEGV signal.

Tested these patches with NX-GZIP support and will be posting this series
soon.

Patches 1 & 2: Define alloc IRQ and get port address per chip which are needed
               to alloc IRQ per VAS instance.
Patch 3: Define nx_fault_stamp on which NX writes fault status for the fault
         CRB
Patch 4: Alloc and setup IRQ and trigger port address for each VAS instance
Patch 5: Setup fault window per each VAS instance. This window is used for
         NX to paste fault CRB in FIFO.
Patches 6 & 7: Setup threaded IRQ per VAS and register NX with fault window
         ID and port number for each send window so that NX paste fault CRB
         in this window.
Patch 8: Reference to pid and mm so that pid is not used until window closed.
         Needed for multi thread application where child can open a window
         and can be used by parent later.
Patches 9 and 10: Process CRBs from fault FIFO and notify tasks by
         updating CSB or through signals.
Patches 11 and 12: Return credits for send and fault windows after handling
        faults.
Patch 14:Fix closing send window after all credits are returned. This issue
         happens only for user space requests. No page faults on kernel
         request buffer.

Changelog:
V2:
  - Use threaded IRQ instead of own kernel thread handler
  - Use pswid instead of user space CSB address to find valid CRB
  - Removed unused macros and other changes as suggested by Christoph Hellwig

V3:
  - Rebased to 5.5-rc2
  - Use struct pid * instead of pid_t for vas_window tgid
  - Code cleanup as suggested by Christoph Hellwig

V4:
  - Define xive alloc and get IRQ info based on chip ID and use these
    functions for IRQ setup per VAS instance. It eliminates skiboot
    dependency as suggested by Oliver.

V5:
  - Do not update CSB if the process is exiting (patch9)

V6:
  - Add interrupt handler instead of default one and return IRQ_HANDLED
    if the fault handling thread is already in progress. (Patch6)
  - Use platform send window ID and CCW[0] bit to find valid CRB in
    fault FIFO (Patch6).
  - Return fault address to user space in BE and other changes as
    suggested by Michael Neuling. (patch9)
  - Rebased to 5.6-rc4

V7:
  - Fix sparse warnings (patches 6,9 and 10)

V8:
  - Move mm_context_remove_copro() before mmdrop() (patch8)
  - Move barrier before csb.flags store and add WARN_ON_ONCE() checks (patch9)

Haren Myneni (14):
  powerpc/xive: Define xive_native_alloc_irq_on_chip()
  powerpc/xive: Define xive_native_alloc_get_irq_info()
  powerpc/vas: Define nx_fault_stamp in coprocessor_request_block
  powerpc/vas: Alloc and setup IRQ and trigger port address
  powerpc/vas: Setup fault window per VAS instance
  powerpc/vas: Setup thread IRQ handler per VAS instance
  powerpc/vas: Register NX with fault window ID and IRQ port value
  powerpc/vas: Take reference to PID and mm for user space windows
  powerpc/vas: Update CSB and notify process for fault CRBs
  powerpc/vas: Print CRB and FIFO values
  powerpc/vas: Do not use default credits for receive window
  powerpc/vas: Return credits after handling fault
  powerpc/vas: Display process stuck message
  powerpc/vas: Free send window in VAS instance after credits returned

 arch/powerpc/include/asm/icswx.h            |  18 +-
 arch/powerpc/include/asm/xive.h             |  11 +-
 arch/powerpc/platforms/powernv/Makefile     |   2 +-
 arch/powerpc/platforms/powernv/ocxl.c       |  20 +-
 arch/powerpc/platforms/powernv/vas-debug.c  |   2 +-
 arch/powerpc/platforms/powernv/vas-fault.c  | 332 ++++++++++++++++++++++++++++
 arch/powerpc/platforms/powernv/vas-window.c | 185 ++++++++++++++--
 arch/powerpc/platforms/powernv/vas.c        | 101 ++++++++-
 arch/powerpc/platforms/powernv/vas.h        |  51 ++++-
 arch/powerpc/sysdev/xive/native.c           |  29 ++-
 10 files changed, 704 insertions(+), 47 deletions(-)
 create mode 100644 arch/powerpc/platforms/powernv/vas-fault.c

-- 
1.8.3.1

[PATCH v8 01/14] powerpc/xive: Define xive_native_alloc_irq_on_chip()

[Haren Myneni]

This function allocates IRQ on a specific chip. VAS needs per chip
IRQ allocation and will have IRQ handler per VAS instance.

Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
 arch/powerpc/include/asm/xive.h   | 9 ++++++++-
 arch/powerpc/sysdev/xive/native.c | 6 +++---
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
index 93f982db..d08ea11 100644
--- a/arch/powerpc/include/asm/xive.h
+++ b/arch/powerpc/include/asm/xive.h
@@ -5,6 +5,8 @@
 #ifndef _ASM_POWERPC_XIVE_H
 #define _ASM_POWERPC_XIVE_H
 
+#include <asm/opal-api.h>
+
 #define XIVE_INVALID_VP	0xffffffff
 
 #ifdef CONFIG_PPC_XIVE
@@ -108,7 +110,6 @@ struct xive_q {
 int xive_native_populate_irq_data(u32 hw_irq,
 				  struct xive_irq_data *data);
 void xive_cleanup_irq_data(struct xive_irq_data *xd);
-u32 xive_native_alloc_irq(void);
 void xive_native_free_irq(u32 irq);
 int xive_native_configure_irq(u32 hw_irq, u32 target, u8 prio, u32 sw_irq);
 
@@ -137,6 +138,12 @@ int xive_native_set_queue_state(u32 vp_id, uint32_t prio, u32 qtoggle,
 				u32 qindex);
 int xive_native_get_vp_state(u32 vp_id, u64 *out_state);
 bool xive_native_has_queue_state_support(void);
+extern u32 xive_native_alloc_irq_on_chip(u32 chip_id);
+
+static inline u32 xive_native_alloc_irq(void)
+{
+	return xive_native_alloc_irq_on_chip(OPAL_XIVE_ANY_CHIP);
+}
 
 #else
 
diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
index 0ff6b73..14d4406 100644
--- a/arch/powerpc/sysdev/xive/native.c
+++ b/arch/powerpc/sysdev/xive/native.c
@@ -279,12 +279,12 @@ static int xive_native_get_ipi(unsigned int cpu, struct xive_cpu *xc)
 }
 #endif /* CONFIG_SMP */
 
-u32 xive_native_alloc_irq(void)
+u32 xive_native_alloc_irq_on_chip(u32 chip_id)
 {
 	s64 rc;
 
 	for (;;) {
-		rc = opal_xive_allocate_irq(OPAL_XIVE_ANY_CHIP);
+		rc = opal_xive_allocate_irq(chip_id);
 		if (rc != OPAL_BUSY)
 			break;
 		msleep(OPAL_BUSY_DELAY_MS);
@@ -293,7 +293,7 @@ u32 xive_native_alloc_irq(void)
 		return 0;
 	return rc;
 }
-EXPORT_SYMBOL_GPL(xive_native_alloc_irq);
+EXPORT_SYMBOL_GPL(xive_native_alloc_irq_on_chip);
 
 void xive_native_free_irq(u32 irq)
 {
-- 
1.8.3.1

[PATCH v8 02/14] powerpc/xive: Define xive_native_alloc_get_irq_info()

[Haren Myneni]

pnv_ocxl_alloc_xive_irq() in ocxl.c allocates IRQ and gets trigger port
address. VAS also needs this function, but based on chip ID. So moved
this common function to xive/native.c.

Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
 arch/powerpc/include/asm/xive.h       |  2 ++
 arch/powerpc/platforms/powernv/ocxl.c | 20 ++------------------
 arch/powerpc/sysdev/xive/native.c     | 23 +++++++++++++++++++++++
 3 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
index d08ea11..fd337da 100644
--- a/arch/powerpc/include/asm/xive.h
+++ b/arch/powerpc/include/asm/xive.h
@@ -139,6 +139,8 @@ int xive_native_set_queue_state(u32 vp_id, uint32_t prio, u32 qtoggle,
 int xive_native_get_vp_state(u32 vp_id, u64 *out_state);
 bool xive_native_has_queue_state_support(void);
 extern u32 xive_native_alloc_irq_on_chip(u32 chip_id);
+extern int xive_native_alloc_get_irq_info(u32 chip_id, u32 *irq,
+					u64 *trigger_addr);
 
 static inline u32 xive_native_alloc_irq(void)
 {
diff --git a/arch/powerpc/platforms/powernv/ocxl.c b/arch/powerpc/platforms/powernv/ocxl.c
index 8c65aac..fb8f99a 100644
--- a/arch/powerpc/platforms/powernv/ocxl.c
+++ b/arch/powerpc/platforms/powernv/ocxl.c
@@ -487,24 +487,8 @@ int pnv_ocxl_spa_remove_pe_from_cache(void *platform_data, int pe_handle)
 
 int pnv_ocxl_alloc_xive_irq(u32 *irq, u64 *trigger_addr)
 {
-	__be64 flags, trigger_page;
-	s64 rc;
-	u32 hwirq;
-
-	hwirq = xive_native_alloc_irq();
-	if (!hwirq)
-		return -ENOENT;
-
-	rc = opal_xive_get_irq_info(hwirq, &flags, NULL, &trigger_page, NULL,
-				NULL);
-	if (rc || !trigger_page) {
-		xive_native_free_irq(hwirq);
-		return -ENOENT;
-	}
-	*irq = hwirq;
-	*trigger_addr = be64_to_cpu(trigger_page);
-	return 0;
-
+	return xive_native_alloc_get_irq_info(OPAL_XIVE_ANY_CHIP, irq,
+						trigger_addr);
 }
 EXPORT_SYMBOL_GPL(pnv_ocxl_alloc_xive_irq);
 
diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
index 14d4406..abdd892 100644
--- a/arch/powerpc/sysdev/xive/native.c
+++ b/arch/powerpc/sysdev/xive/native.c
@@ -295,6 +295,29 @@ u32 xive_native_alloc_irq_on_chip(u32 chip_id)
 }
 EXPORT_SYMBOL_GPL(xive_native_alloc_irq_on_chip);
 
+int xive_native_alloc_get_irq_info(u32 chip_id, u32 *irq, u64 *trigger_addr)
+{
+	__be64 flags, trigger_page;
+	u32 hwirq;
+	s64 rc;
+
+	hwirq = xive_native_alloc_irq_on_chip(chip_id);
+	if (!hwirq)
+		return -ENOENT;
+
+	rc = opal_xive_get_irq_info(hwirq, &flags, NULL, &trigger_page, NULL,
+				NULL);
+	if (rc || !trigger_page) {
+		xive_native_free_irq(hwirq);
+		return -ENOENT;
+	}
+	*irq = hwirq;
+	*trigger_addr = be64_to_cpu(trigger_page);
+
+	return 0;
+}
+EXPORT_SYMBOL(xive_native_alloc_get_irq_info);
+
 void xive_native_free_irq(u32 irq)
 {
 	for (;;) {
-- 
1.8.3.1

[PATCH v8 03/14] powerpc/vas: Define nx_fault_stamp in coprocessor_request_block

[Haren Myneni]

Kernel sets fault address and status in CRB for NX page fault on user
space address after processing page fault. User space gets the signal
and handles the fault mentioned in CRB by bringing the page in to
memory and send NX request again.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
 arch/powerpc/include/asm/icswx.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/icswx.h b/arch/powerpc/include/asm/icswx.h
index 9872f85..b233d1e 100644
--- a/arch/powerpc/include/asm/icswx.h
+++ b/arch/powerpc/include/asm/icswx.h
@@ -108,6 +108,17 @@ struct data_descriptor_entry {
 	__be64 address;
 } __packed __aligned(DDE_ALIGN);
 
+/* 4.3.2 NX-stamped Fault CRB */
+
+#define NX_STAMP_ALIGN          (0x10)
+
+struct nx_fault_stamp {
+	__be64 fault_storage_addr;
+	__be16 reserved;
+	__u8   flags;
+	__u8   fault_status;
+	__be32 pswid;
+} __packed __aligned(NX_STAMP_ALIGN);
 
 /* Chapter 6.5.2 Coprocessor-Request Block (CRB) */
 
@@ -135,7 +146,12 @@ struct coprocessor_request_block {
 
 	struct coprocessor_completion_block ccb;
 
-	u8 reserved[48];
+	union {
+		struct nx_fault_stamp nx;
+		u8 reserved[16];
+	} stamp;
+
+	u8 reserved[32];
 
 	struct coprocessor_status_block csb;
 } __packed __aligned(CRB_ALIGN);
-- 
1.8.3.1

[PATCH v8 04/14] powerpc/vas: Alloc and setup IRQ and trigger port address

[Haren Myneni]

Alloc IRQ and get trigger port address for each VAS instance. Kernel
register this IRQ per VAS instance and sets this port for each send
window. NX interrupts the kernel when it sees page fault.

Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
 arch/powerpc/platforms/powernv/vas.c | 34 ++++++++++++++++++++++++++++------
 arch/powerpc/platforms/powernv/vas.h |  2 ++
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/vas.c b/arch/powerpc/platforms/powernv/vas.c
index ed9cc6d..168ab68 100644
--- a/arch/powerpc/platforms/powernv/vas.c
+++ b/arch/powerpc/platforms/powernv/vas.c
@@ -15,6 +15,7 @@
 #include <linux/of_address.h>
 #include <linux/of.h>
 #include <asm/prom.h>
+#include <asm/xive.h>
 
 #include "vas.h"
 
@@ -25,10 +26,12 @@
 
 static int init_vas_instance(struct platform_device *pdev)
 {
-	int rc, cpu, vasid;
-	struct resource *res;
-	struct vas_instance *vinst;
 	struct device_node *dn = pdev->dev.of_node;
+	struct vas_instance *vinst;
+	uint32_t chipid, irq;
+	struct resource *res;
+	int rc, cpu, vasid;
+	uint64_t port;
 
 	rc = of_property_read_u32(dn, "ibm,vas-id", &vasid);
 	if (rc) {
@@ -36,6 +39,12 @@ static int init_vas_instance(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	rc = of_property_read_u32(dn, "ibm,chip-id", &chipid);
+	if (rc) {
+		pr_err("No ibm,chip-id property for %s?\n", pdev->name);
+		return -ENODEV;
+	}
+
 	if (pdev->num_resources != 4) {
 		pr_err("Unexpected DT configuration for [%s, %d]\n",
 				pdev->name, vasid);
@@ -69,9 +78,22 @@ static int init_vas_instance(struct platform_device *pdev)
 
 	vinst->paste_win_id_shift = 63 - res->end;
 
-	pr_devel("Initialized instance [%s, %d], paste_base 0x%llx, "
-			"paste_win_id_shift 0x%llx\n", pdev->name, vasid,
-			vinst->paste_base_addr, vinst->paste_win_id_shift);
+	rc = xive_native_alloc_get_irq_info(chipid, &irq, &port);
+	if (rc)
+		return rc;
+
+	vinst->virq = irq_create_mapping(NULL, irq);
+	if (!vinst->virq) {
+		pr_err("Inst%d: Unable to map global irq %d\n",
+				vinst->vas_id, irq);
+		return -EINVAL;
+	}
+
+	vinst->irq_port = port;
+	pr_devel("Initialized instance [%s, %d] paste_base 0x%llx paste_win_id_shift 0x%llx IRQ %d Port 0x%llx\n",
+			pdev->name, vasid, vinst->paste_base_addr,
+			vinst->paste_win_id_shift, vinst->virq,
+			vinst->irq_port);
 
 	for_each_possible_cpu(cpu) {
 		if (cpu_to_chip_id(cpu) == of_get_ibm_chip_id(dn))
diff --git a/arch/powerpc/platforms/powernv/vas.h b/arch/powerpc/platforms/powernv/vas.h
index 5574aec..598608b 100644
--- a/arch/powerpc/platforms/powernv/vas.h
+++ b/arch/powerpc/platforms/powernv/vas.h
@@ -313,6 +313,8 @@ struct vas_instance {
 	u64 paste_base_addr;
 	u64 paste_win_id_shift;
 
+	u64 irq_port;
+	int virq;
 	struct mutex mutex;
 	struct vas_window *rxwin[VAS_COP_TYPE_MAX];
 	struct vas_window *windows[VAS_WINDOWS_PER_CHIP];
-- 
1.8.3.1

[PATCH v8 05/14] powerpc/vas: Setup fault window per VAS instance

[Haren Myneni]

Setup fault window for each VAS instance. When NX gets a fault on
request buffer, write fault CRBs in the corresponding fault FIFO and
then sends an interrupt to the OS.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
 arch/powerpc/platforms/powernv/Makefile     |  2 +-
 arch/powerpc/platforms/powernv/vas-fault.c  | 77 +++++++++++++++++++++++++++++
 arch/powerpc/platforms/powernv/vas-window.c |  4 +-
 arch/powerpc/platforms/powernv/vas.c        | 20 ++++++++
 arch/powerpc/platforms/powernv/vas.h        | 16 ++++++
 5 files changed, 116 insertions(+), 3 deletions(-)
 create mode 100644 arch/powerpc/platforms/powernv/vas-fault.c

diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
index c0f8120..395789f 100644
--- a/arch/powerpc/platforms/powernv/Makefile
+++ b/arch/powerpc/platforms/powernv/Makefile
@@ -17,7 +17,7 @@ obj-$(CONFIG_MEMORY_FAILURE)	+= opal-memory-errors.o
 obj-$(CONFIG_OPAL_PRD)	+= opal-prd.o
 obj-$(CONFIG_PERF_EVENTS) += opal-imc.o
 obj-$(CONFIG_PPC_MEMTRACE)	+= memtrace.o
-obj-$(CONFIG_PPC_VAS)	+= vas.o vas-window.o vas-debug.o
+obj-$(CONFIG_PPC_VAS)	+= vas.o vas-window.o vas-debug.o vas-fault.o
 obj-$(CONFIG_OCXL_BASE)	+= ocxl.o
 obj-$(CONFIG_SCOM_DEBUGFS) += opal-xscom.o
 obj-$(CONFIG_PPC_SECURE_BOOT) += opal-secvar.o
diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c
new file mode 100644
index 0000000..4044998
--- /dev/null
+++ b/arch/powerpc/platforms/powernv/vas-fault.c
@@ -0,0 +1,77 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * VAS Fault handling.
+ * Copyright 2019, IBM Corporation
+ */
+
+#define pr_fmt(fmt) "vas: " fmt
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/kthread.h>
+#include <asm/icswx.h>
+
+#include "vas.h"
+
+/*
+ * The maximum FIFO size for fault window can be 8MB
+ * (VAS_RX_FIFO_SIZE_MAX). Using 4MB FIFO since each VAS
+ * instance will be having fault window.
+ * 8MB FIFO can be used if expects more faults for each VAS
+ * instance.
+ */
+#define VAS_FAULT_WIN_FIFO_SIZE	(4 << 20)
+
+/*
+ * Fault window is opened per VAS instance. NX pastes fault CRB in fault
+ * FIFO upon page faults.
+ */
+int vas_setup_fault_window(struct vas_instance *vinst)
+{
+	struct vas_rx_win_attr attr;
+
+	vinst->fault_fifo_size = VAS_FAULT_WIN_FIFO_SIZE;
+	vinst->fault_fifo = kzalloc(vinst->fault_fifo_size, GFP_KERNEL);
+	if (!vinst->fault_fifo) {
+		pr_err("Unable to alloc %d bytes for fault_fifo\n",
+				vinst->fault_fifo_size);
+		return -ENOMEM;
+	}
+
+	/*
+	 * Invalidate all CRB entries. NX pastes valid entry for each fault.
+	 */
+	memset(vinst->fault_fifo, FIFO_INVALID_ENTRY, vinst->fault_fifo_size);
+	vas_init_rx_win_attr(&attr, VAS_COP_TYPE_FAULT);
+
+	attr.rx_fifo_size = vinst->fault_fifo_size;
+	attr.rx_fifo = vinst->fault_fifo;
+
+	/*
+	 * Max creds is based on number of CRBs can fit in the FIFO.
+	 * (fault_fifo_size/CRB_SIZE). If 8MB FIFO is used, max creds
+	 * will be 0xffff since the receive creds field is 16bits wide.
+	 */
+	attr.wcreds_max = vinst->fault_fifo_size / CRB_SIZE;
+	attr.lnotify_lpid = 0;
+	attr.lnotify_pid = mfspr(SPRN_PID);
+	attr.lnotify_tid = mfspr(SPRN_PID);
+
+	vinst->fault_win = vas_rx_win_open(vinst->vas_id, VAS_COP_TYPE_FAULT,
+					&attr);
+
+	if (IS_ERR(vinst->fault_win)) {
+		pr_err("VAS: Error %ld opening FaultWin\n",
+			PTR_ERR(vinst->fault_win));
+		kfree(vinst->fault_fifo);
+		return PTR_ERR(vinst->fault_win);
+	}
+
+	pr_devel("VAS: Created FaultWin %d, LPID/PID/TID [%d/%d/%d]\n",
+			vinst->fault_win->winid, attr.lnotify_lpid,
+			attr.lnotify_pid, attr.lnotify_tid);
+
+	return 0;
+}
diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
index 0c0d27d..1783fa9 100644
--- a/arch/powerpc/platforms/powernv/vas-window.c
+++ b/arch/powerpc/platforms/powernv/vas-window.c
@@ -827,9 +827,9 @@ void vas_init_rx_win_attr(struct vas_rx_win_attr *rxattr, enum vas_cop_type cop)
 		rxattr->fault_win = true;
 		rxattr->notify_disable = true;
 		rxattr->rx_wcred_mode = true;
-		rxattr->tx_wcred_mode = true;
 		rxattr->rx_win_ord_mode = true;
-		rxattr->tx_win_ord_mode = true;
+		rxattr->rej_no_credit = true;
+		rxattr->tc_mode = VAS_THRESH_DISABLED;
 	} else if (cop == VAS_COP_TYPE_FTW) {
 		rxattr->user_win = true;
 		rxattr->intr_disable = true;
diff --git a/arch/powerpc/platforms/powernv/vas.c b/arch/powerpc/platforms/powernv/vas.c
index 168ab68..557c8e4 100644
--- a/arch/powerpc/platforms/powernv/vas.c
+++ b/arch/powerpc/platforms/powernv/vas.c
@@ -24,6 +24,11 @@
 
 static DEFINE_PER_CPU(int, cpu_vas_id);
 
+static int vas_irq_fault_window_setup(struct vas_instance *vinst)
+{
+	return vas_setup_fault_window(vinst);
+}
+
 static int init_vas_instance(struct platform_device *pdev)
 {
 	struct device_node *dn = pdev->dev.of_node;
@@ -104,6 +109,21 @@ static int init_vas_instance(struct platform_device *pdev)
 	list_add(&vinst->node, &vas_instances);
 	mutex_unlock(&vas_mutex);
 
+	/*
+	 * IRQ and fault handling setup is needed only for user space
+	 * send windows.
+	 */
+	if (vinst->virq) {
+		rc = vas_irq_fault_window_setup(vinst);
+		/*
+		 * Fault window is used only for user space send windows.
+		 * So if vinst->virq is NULL, tx_win_open returns -ENODEV
+		 * for user space.
+		 */
+		if (rc)
+			vinst->virq = 0;
+	}
+
 	vas_instance_init_dbgdir(vinst);
 
 	dev_set_drvdata(&pdev->dev, vinst);
diff --git a/arch/powerpc/platforms/powernv/vas.h b/arch/powerpc/platforms/powernv/vas.h
index 598608b..6c4baf5 100644
--- a/arch/powerpc/platforms/powernv/vas.h
+++ b/arch/powerpc/platforms/powernv/vas.h
@@ -296,6 +296,17 @@ enum vas_notify_after_count {
 };
 
 /*
+ * NX can generate an interrupt for multiple faults and expects kernel
+ * to process all of them. So read all valid CRB entries until find the
+ * invalid one. So use pswid which is pasted by NX and ccw[0] (reserved
+ * bit in BE) to check valid CRB.
+ * Invalidate FIFO during allocation and process all entries from last
+ * successful read until finds invalid pswid and ccw[0] values.
+ */
+#define FIFO_INVALID_ENTRY	0xffffffff
+#define CCW0_INVALID		1
+
+/*
  * One per instance of VAS. Each instance will have a separate set of
  * receive windows, one per coprocessor type.
  *
@@ -315,6 +326,10 @@ struct vas_instance {
 
 	u64 irq_port;
 	int virq;
+	int fault_fifo_size;
+	void *fault_fifo;
+	struct vas_window *fault_win; /* Fault window */
+
 	struct mutex mutex;
 	struct vas_window *rxwin[VAS_COP_TYPE_MAX];
 	struct vas_window *windows[VAS_WINDOWS_PER_CHIP];
@@ -408,6 +423,7 @@ struct vas_winctx {
 extern void vas_instance_init_dbgdir(struct vas_instance *vinst);
 extern void vas_window_init_dbgdir(struct vas_window *win);
 extern void vas_window_free_dbgdir(struct vas_window *win);
+extern int vas_setup_fault_window(struct vas_instance *vinst);
 
 static inline void vas_log_write(struct vas_window *win, char *name,
 			void *regptr, u64 val)
-- 
1.8.3.1

[PATCH v8 06/14] powerpc/vas: Setup thread IRQ handler per VAS instance

[Haren Myneni]

Setup thread IRQ handler per each VAS instance. When NX sees a fault
on CRB, kernel gets an interrupt and vas_fault_handler will be
executed to process fault CRBs. Read all valid CRBs from fault FIFO,
determine the corresponding send window from CRB and process fault
requests.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
 arch/powerpc/platforms/powernv/vas-fault.c  | 90 +++++++++++++++++++++++++++++
 arch/powerpc/platforms/powernv/vas-window.c | 60 +++++++++++++++++++
 arch/powerpc/platforms/powernv/vas.c        | 49 +++++++++++++++-
 arch/powerpc/platforms/powernv/vas.h        |  6 ++
 4 files changed, 204 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c
index 4044998..1c6d5cc 100644
--- a/arch/powerpc/platforms/powernv/vas-fault.c
+++ b/arch/powerpc/platforms/powernv/vas-fault.c
@@ -11,6 +11,7 @@
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/kthread.h>
+#include <linux/mmu_context.h>
 #include <asm/icswx.h>
 
 #include "vas.h"
@@ -25,6 +26,95 @@
 #define VAS_FAULT_WIN_FIFO_SIZE	(4 << 20)
 
 /*
+ * Process valid CRBs in fault FIFO.
+ */
+irqreturn_t vas_fault_thread_fn(int irq, void *data)
+{
+	struct vas_instance *vinst = data;
+	struct coprocessor_request_block *crb, *entry;
+	struct coprocessor_request_block buf;
+	struct vas_window *window;
+	unsigned long flags;
+	void *fifo;
+
+	crb = &buf;
+
+	/*
+	 * VAS can interrupt with multiple page faults. So process all
+	 * valid CRBs within fault FIFO until reaches invalid CRB.
+	 * NX updates nx_fault_stamp in CRB and pastes in fault FIFO.
+	 * kernel retrives send window from parition send window ID
+	 * (pswid) in nx_fault_stamp. So pswid should be valid and
+	 * ccw[0] (in be) should be zero since this bit is reserved.
+	 * If user space touches this bit, NX returns with "CRB format
+	 * error".
+	 *
+	 * After reading CRB entry, invalidate it with pswid (set
+	 * 0xffffffff) and ccw[0] (set to 1).
+	 *
+	 * In case kernel receives another interrupt with different page
+	 * fault, CRBs are already processed by the previous handling. So
+	 * will be returned from this function when it sees invalid CRB.
+	 */
+	do {
+		mutex_lock(&vinst->mutex);
+
+		spin_lock_irqsave(&vinst->fault_lock, flags);
+		/*
+		 * Advance the fault fifo pointer to next CRB.
+		 * Use CRB_SIZE rather than sizeof(*crb) since the latter is
+		 * aligned to CRB_ALIGN (256) but the CRB written to by VAS is
+		 * only CRB_SIZE in len.
+		 */
+		fifo = vinst->fault_fifo + (vinst->fault_crbs * CRB_SIZE);
+		entry = fifo;
+
+		if ((entry->stamp.nx.pswid == cpu_to_be32(FIFO_INVALID_ENTRY))
+			|| (entry->ccw & cpu_to_be32(CCW0_INVALID))) {
+			atomic_set(&vinst->faults_in_progress, 0);
+			spin_unlock_irqrestore(&vinst->fault_lock, flags);
+			mutex_unlock(&vinst->mutex);
+			return IRQ_HANDLED;
+		}
+
+		spin_unlock_irqrestore(&vinst->fault_lock, flags);
+		vinst->fault_crbs++;
+		if (vinst->fault_crbs == (vinst->fault_fifo_size / CRB_SIZE))
+			vinst->fault_crbs = 0;
+
+		memcpy(crb, fifo, CRB_SIZE);
+		entry->stamp.nx.pswid = cpu_to_be32(FIFO_INVALID_ENTRY);
+		entry->ccw |= cpu_to_be32(CCW0_INVALID);
+		mutex_unlock(&vinst->mutex);
+
+		pr_devel("VAS[%d] fault_fifo %p, fifo %p, fault_crbs %d\n",
+				vinst->vas_id, vinst->fault_fifo, fifo,
+				vinst->fault_crbs);
+
+		window = vas_pswid_to_window(vinst,
+				be32_to_cpu(crb->stamp.nx.pswid));
+
+		if (IS_ERR(window)) {
+			/*
+			 * We got an interrupt about a specific send
+			 * window but we can't find that window and we can't
+			 * even clean it up (return credit).
+			 * But we should not get here.
+			 */
+			pr_err("VAS[%d] fault_fifo %p, fifo %p, pswid 0x%x, fault_crbs %d bad CRB?\n",
+				vinst->vas_id, vinst->fault_fifo, fifo,
+				be32_to_cpu(crb->stamp.nx.pswid),
+				vinst->fault_crbs);
+
+			WARN_ON_ONCE(1);
+			atomic_set(&vinst->faults_in_progress, 0);
+			return IRQ_HANDLED;
+		}
+
+	} while (true);
+}
+
+/*
  * Fault window is opened per VAS instance. NX pastes fault CRB in fault
  * FIFO upon page faults.
  */
diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
index 1783fa9..1f31c18 100644
--- a/arch/powerpc/platforms/powernv/vas-window.c
+++ b/arch/powerpc/platforms/powernv/vas-window.c
@@ -1040,6 +1040,15 @@ struct vas_window *vas_tx_win_open(int vasid, enum vas_cop_type cop,
 		}
 	} else {
 		/*
+		 * Interrupt hanlder or fault window setup failed. Means
+		 * NX can not generate fault for page fault. So not
+		 * opening for user space tx window.
+		 */
+		if (!vinst->virq) {
+			rc = -ENODEV;
+			goto free_window;
+		}
+		/*
 		 * A user mapping must ensure that context switch issues
 		 * CP_ABORT for this thread.
 		 */
@@ -1254,3 +1263,54 @@ int vas_win_close(struct vas_window *window)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(vas_win_close);
+
+struct vas_window *vas_pswid_to_window(struct vas_instance *vinst,
+		uint32_t pswid)
+{
+	struct vas_window *window;
+	int winid;
+
+	if (!pswid) {
+		pr_devel("%s: called for pswid 0!\n", __func__);
+		return ERR_PTR(-ESRCH);
+	}
+
+	decode_pswid(pswid, NULL, &winid);
+
+	if (winid >= VAS_WINDOWS_PER_CHIP)
+		return ERR_PTR(-ESRCH);
+
+	/*
+	 * If application closes the window before the hardware
+	 * returns the fault CRB, we should wait in vas_win_close()
+	 * for the pending requests. so the window must be active
+	 * and the process alive.
+	 *
+	 * If its a kernel process, we should not get any faults and
+	 * should not get here.
+	 */
+	window = vinst->windows[winid];
+
+	if (!window) {
+		pr_err("PSWID decode: Could not find window for winid %d pswid %d vinst 0x%p\n",
+			winid, pswid, vinst);
+		return NULL;
+	}
+
+	/*
+	 * Do some sanity checks on the decoded window.  Window should be
+	 * NX GZIP user send window. FTW windows should not incur faults
+	 * since their CRBs are ignored (not queued on FIFO or processed
+	 * by NX).
+	 */
+	if (!window->tx_win || !window->user_win || !window->nx_win ||
+			window->cop == VAS_COP_TYPE_FAULT ||
+			window->cop == VAS_COP_TYPE_FTW) {
+		pr_err("PSWID decode: id %d, tx %d, user %d, nx %d, cop %d\n",
+			winid, window->tx_win, window->user_win,
+			window->nx_win, window->cop);
+		WARN_ON(1);
+	}
+
+	return window;
+}
diff --git a/arch/powerpc/platforms/powernv/vas.c b/arch/powerpc/platforms/powernv/vas.c
index 557c8e4..3d9ba58 100644
--- a/arch/powerpc/platforms/powernv/vas.c
+++ b/arch/powerpc/platforms/powernv/vas.c
@@ -14,6 +14,8 @@
 #include <linux/of_platform.h>
 #include <linux/of_address.h>
 #include <linux/of.h>
+#include <linux/irqdomain.h>
+#include <linux/interrupt.h>
 #include <asm/prom.h>
 #include <asm/xive.h>
 
@@ -24,9 +26,53 @@
 
 static DEFINE_PER_CPU(int, cpu_vas_id);
 
+static irqreturn_t vas_fault_handler(int irq, void *dev_id)
+{
+	struct vas_instance *vinst = dev_id;
+	irqreturn_t ret = IRQ_WAKE_THREAD;
+	unsigned long flags;
+
+	/*
+	 * NX can generate an interrupt for multiple faults. So the
+	 * fault handler thread process all CRBs until finds invalid
+	 * entry. In case if NX sees continuous faults, it is possible
+	 * that the thread function entered with the first interrupt
+	 * can execute and process all valid CRBs.
+	 * So wake up thread only if the fault thread is not in progress.
+	 */
+	spin_lock_irqsave(&vinst->fault_lock, flags);
+
+	if (atomic_read(&vinst->faults_in_progress))
+		ret = IRQ_HANDLED;
+	else
+		atomic_set(&vinst->faults_in_progress, 1);
+
+	spin_unlock_irqrestore(&vinst->fault_lock, flags);
+
+	return ret;
+}
+
 static int vas_irq_fault_window_setup(struct vas_instance *vinst)
 {
-	return vas_setup_fault_window(vinst);
+	char devname[64];
+	int rc = 0;
+
+	snprintf(devname, sizeof(devname), "vas-%d", vinst->vas_id);
+	rc = request_threaded_irq(vinst->virq, vas_fault_handler,
+				vas_fault_thread_fn, 0, devname, vinst);
+
+	if (rc) {
+		pr_err("VAS[%d]: Request IRQ(%d) failed with %d\n",
+				vinst->vas_id, vinst->virq, rc);
+		goto out;
+	}
+
+	rc = vas_setup_fault_window(vinst);
+	if (rc)
+		free_irq(vinst->virq, vinst);
+
+out:
+	return rc;
 }
 
 static int init_vas_instance(struct platform_device *pdev)
@@ -109,6 +155,7 @@ static int init_vas_instance(struct platform_device *pdev)
 	list_add(&vinst->node, &vas_instances);
 	mutex_unlock(&vas_mutex);
 
+	spin_lock_init(&vinst->fault_lock);
 	/*
 	 * IRQ and fault handling setup is needed only for user space
 	 * send windows.
diff --git a/arch/powerpc/platforms/powernv/vas.h b/arch/powerpc/platforms/powernv/vas.h
index 6c4baf5..ecae7cd 100644
--- a/arch/powerpc/platforms/powernv/vas.h
+++ b/arch/powerpc/platforms/powernv/vas.h
@@ -326,7 +326,10 @@ struct vas_instance {
 
 	u64 irq_port;
 	int virq;
+	int fault_crbs;
 	int fault_fifo_size;
+	atomic_t faults_in_progress;
+	spinlock_t fault_lock;
 	void *fault_fifo;
 	struct vas_window *fault_win; /* Fault window */
 
@@ -424,6 +427,9 @@ struct vas_winctx {
 extern void vas_window_init_dbgdir(struct vas_window *win);
 extern void vas_window_free_dbgdir(struct vas_window *win);
 extern int vas_setup_fault_window(struct vas_instance *vinst);
+extern irqreturn_t vas_fault_thread_fn(int irq, void *data);
+extern struct vas_window *vas_pswid_to_window(struct vas_instance *vinst,
+						uint32_t pswid);
 
 static inline void vas_log_write(struct vas_window *win, char *name,
 			void *regptr, u64 val)
-- 
1.8.3.1

[PATCH v8 07/14] powerpc/vas: Register NX with fault window ID and IRQ port value

[Haren Myneni]

For each user space send window, register NX with fault window ID
and port value so that NX paste CRBs in this fault FIFO when it
sees fault on the request buffer.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
 arch/powerpc/platforms/powernv/vas-window.c | 15 +++++++++++++--
 arch/powerpc/platforms/powernv/vas.h        | 15 +++++++++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
index 1f31c18..acb6a22 100644
--- a/arch/powerpc/platforms/powernv/vas-window.c
+++ b/arch/powerpc/platforms/powernv/vas-window.c
@@ -373,7 +373,7 @@ int init_winctx_regs(struct vas_window *window, struct vas_winctx *winctx)
 	init_xlate_regs(window, winctx->user_win);
 
 	val = 0ULL;
-	val = SET_FIELD(VAS_FAULT_TX_WIN, val, 0);
+	val = SET_FIELD(VAS_FAULT_TX_WIN, val, winctx->fault_win_id);
 	write_hvwc_reg(window, VREG(FAULT_TX_WIN), val);
 
 	/* In PowerNV, interrupts go to HV. */
@@ -748,6 +748,8 @@ static void init_winctx_for_rxwin(struct vas_window *rxwin,
 
 	winctx->min_scope = VAS_SCOPE_LOCAL;
 	winctx->max_scope = VAS_SCOPE_VECTORED_GROUP;
+	if (rxwin->vinst->virq)
+		winctx->irq_port = rxwin->vinst->irq_port;
 }
 
 static bool rx_win_args_valid(enum vas_cop_type cop,
@@ -944,13 +946,22 @@ static void init_winctx_for_txwin(struct vas_window *txwin,
 	winctx->lpid = txattr->lpid;
 	winctx->pidr = txattr->pidr;
 	winctx->rx_win_id = txwin->rxwin->winid;
+	/*
+	 * IRQ and fault window setup is successful. Set fault window
+	 * for the send window so that ready to handle faults.
+	 */
+	if (txwin->vinst->virq)
+		winctx->fault_win_id = txwin->vinst->fault_win->winid;
 
 	winctx->dma_type = VAS_DMA_TYPE_INJECT;
 	winctx->tc_mode = txattr->tc_mode;
 	winctx->min_scope = VAS_SCOPE_LOCAL;
 	winctx->max_scope = VAS_SCOPE_VECTORED_GROUP;
+	if (txwin->vinst->virq)
+		winctx->irq_port = txwin->vinst->irq_port;
 
-	winctx->pswid = 0;
+	winctx->pswid = txattr->pswid ? txattr->pswid :
+			encode_pswid(txwin->vinst->vas_id, txwin->winid);
 }
 
 static bool tx_win_args_valid(enum vas_cop_type cop,
diff --git a/arch/powerpc/platforms/powernv/vas.h b/arch/powerpc/platforms/powernv/vas.h
index ecae7cd..310b8a0 100644
--- a/arch/powerpc/platforms/powernv/vas.h
+++ b/arch/powerpc/platforms/powernv/vas.h
@@ -468,6 +468,21 @@ static inline u64 read_hvwc_reg(struct vas_window *win,
 	return in_be64(win->hvwc_map+reg);
 }
 
+/*
+ * Encode/decode the Partition Send Window ID (PSWID) for a window in
+ * a way that we can uniquely identify any window in the system. i.e.
+ * we should be able to locate the 'struct vas_window' given the PSWID.
+ *
+ *	Bits	Usage
+ *	0:7	VAS id (8 bits)
+ *	8:15	Unused, 0 (3 bits)
+ *	16:31	Window id (16 bits)
+ */
+static inline u32 encode_pswid(int vasid, int winid)
+{
+	return ((u32)winid | (vasid << (31 - 7)));
+}
+
 static inline void decode_pswid(u32 pswid, int *vasid, int *winid)
 {
 	if (vasid)
-- 
1.8.3.1

[PATCH v8 08/14] powerpc/vas: Take reference to PID and mm for user space windows

[Haren Myneni]

When process opens a window, its pid and tgid will be saved in vas_window
struct. This window will be closed when the process exits. Kernel handles
NX faults by updating CSB or send SEGV signal to pid if user space csb_addr
is invalid.

In multi-thread applications, a window can be opened by child thread, but
it will not be closed when this thread exits. Expects parent to clean up
all resources including NX windows. Child thread can send requests using
this window and can be killed before they are completed. But the pid
assigned to this thread can be reused for other task while requests are
pending. If the csb_addr passed in these requests is invalid, kernel will
end up sending signal to the wrong task.

To prevent reusing the pid, take references to pid and mm when the window
is opened and release them during window close.

Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
 arch/powerpc/platforms/powernv/vas-debug.c  |  2 +-
 arch/powerpc/platforms/powernv/vas-window.c | 53 ++++++++++++++++++++++++++---
 arch/powerpc/platforms/powernv/vas.h        |  9 ++++-
 3 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/vas-debug.c b/arch/powerpc/platforms/powernv/vas-debug.c
index 09e63df..ef9a717 100644
--- a/arch/powerpc/platforms/powernv/vas-debug.c
+++ b/arch/powerpc/platforms/powernv/vas-debug.c
@@ -38,7 +38,7 @@ static int info_show(struct seq_file *s, void *private)
 
 	seq_printf(s, "Type: %s, %s\n", cop_to_str(window->cop),
 					window->tx_win ? "Send" : "Receive");
-	seq_printf(s, "Pid : %d\n", window->pid);
+	seq_printf(s, "Pid : %d\n", vas_window_pid(window));
 
 unlock:
 	mutex_unlock(&vas_mutex);
diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
index acb6a22..e7641a5 100644
--- a/arch/powerpc/platforms/powernv/vas-window.c
+++ b/arch/powerpc/platforms/powernv/vas-window.c
@@ -12,6 +12,8 @@
 #include <linux/log2.h>
 #include <linux/rcupdate.h>
 #include <linux/cred.h>
+#include <linux/sched/mm.h>
+#include <linux/mmu_context.h>
 #include <asm/switch_to.h>
 #include <asm/ppc-opcode.h>
 #include "vas.h"
@@ -876,8 +878,6 @@ struct vas_window *vas_rx_win_open(int vasid, enum vas_cop_type cop,
 	rxwin->user_win = rxattr->user_win;
 	rxwin->cop = cop;
 	rxwin->wcreds_max = rxattr->wcreds_max ?: VAS_WCREDS_DEFAULT;
-	if (rxattr->user_win)
-		rxwin->pid = task_pid_vnr(current);
 
 	init_winctx_for_rxwin(rxwin, rxattr, &winctx);
 	init_winctx_regs(rxwin, &winctx);
@@ -1027,7 +1027,6 @@ struct vas_window *vas_tx_win_open(int vasid, enum vas_cop_type cop,
 	txwin->tx_win = 1;
 	txwin->rxwin = rxwin;
 	txwin->nx_win = txwin->rxwin->nx_win;
-	txwin->pid = attr->pid;
 	txwin->user_win = attr->user_win;
 	txwin->wcreds_max = attr->wcreds_max ?: VAS_WCREDS_DEFAULT;
 
@@ -1068,8 +1067,43 @@ struct vas_window *vas_tx_win_open(int vasid, enum vas_cop_type cop,
 			goto free_window;
 	}
 
-	set_vinst_win(vinst, txwin);
+	if (txwin->user_win) {
+		/*
+		 * Window opened by child thread may not be closed when
+		 * it exits. So take reference to its pid and release it
+		 * when the window is free by parent thread.
+		 * Acquire a reference to the task's pid to make sure
+		 * pid will not be re-used - needed only for multithread
+		 * applications.
+		 */
+		txwin->pid = get_task_pid(current, PIDTYPE_PID);
+		/*
+		 * Acquire a reference to the task's mm.
+		 */
+		txwin->mm = get_task_mm(current);
 
+		if (!txwin->mm) {
+			put_pid(txwin->pid);
+			pr_err("VAS: pid(%d): mm_struct is not found\n",
+					current->pid);
+			rc = -EPERM;
+			goto free_window;
+		}
+
+		mmgrab(txwin->mm);
+		mmput(txwin->mm);
+		mm_context_add_copro(txwin->mm);
+		/*
+		 * Process closes window during exit. In the case of
+		 * multithread application, child can open window and
+		 * can exit without closing it. Expects parent thread
+		 * to use and close the window. So do not need to take
+		 * pid reference for parent thread.
+		 */
+		txwin->tgid = find_get_pid(task_tgid_vnr(current));
+	}
+
+	set_vinst_win(vinst, txwin);
 	return txwin;
 
 free_window:
@@ -1266,8 +1300,17 @@ int vas_win_close(struct vas_window *window)
 	poll_window_castout(window);
 
 	/* if send window, drop reference to matching receive window */
-	if (window->tx_win)
+	if (window->tx_win) {
+		if (window->user_win) {
+			/* Drop references to pid and mm */
+			put_pid(window->pid);
+			if (window->mm) {
+				mm_context_remove_copro(window->mm);
+				mmdrop(window->mm);
+			}
+		}
 		put_rx_win(window->rxwin);
+	}
 
 	vas_window_free(window);
 
diff --git a/arch/powerpc/platforms/powernv/vas.h b/arch/powerpc/platforms/powernv/vas.h
index 310b8a0..16aa8ec 100644
--- a/arch/powerpc/platforms/powernv/vas.h
+++ b/arch/powerpc/platforms/powernv/vas.h
@@ -353,7 +353,9 @@ struct vas_window {
 	bool user_win;		/* True if user space window */
 	void *hvwc_map;		/* HV window context */
 	void *uwc_map;		/* OS/User window context */
-	pid_t pid;		/* Linux process id of owner */
+	struct pid *pid;	/* Linux process id of owner */
+	struct pid *tgid;	/* Thread group ID of owner */
+	struct mm_struct *mm;	/* Linux process mm_struct */
 	int wcreds_max;		/* Window credits */
 
 	char *dbgname;
@@ -431,6 +433,11 @@ struct vas_winctx {
 extern struct vas_window *vas_pswid_to_window(struct vas_instance *vinst,
 						uint32_t pswid);
 
+static inline int vas_window_pid(struct vas_window *window)
+{
+	return pid_vnr(window->pid);
+}
+
 static inline void vas_log_write(struct vas_window *win, char *name,
 			void *regptr, u64 val)
 {
-- 
1.8.3.1

[PATCH v8 09/14] powerpc/vas: Update CSB and notify process for fault CRBs

[Haren Myneni]

For each fault CRB, update fault address in CRB (fault_storage_addr)
and translation error status in CSB so that user space can touch the
fault address and resend the request. If the user space passed invalid
CSB address send signal to process with SIGSEGV.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
 arch/powerpc/platforms/powernv/vas-fault.c | 115 +++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c
index 1c6d5cc..6eceac5d 100644
--- a/arch/powerpc/platforms/powernv/vas-fault.c
+++ b/arch/powerpc/platforms/powernv/vas-fault.c
@@ -11,6 +11,7 @@
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/kthread.h>
+#include <linux/sched/signal.h>
 #include <linux/mmu_context.h>
 #include <asm/icswx.h>
 
@@ -26,6 +27,119 @@
 #define VAS_FAULT_WIN_FIFO_SIZE	(4 << 20)
 
 /*
+ * Update the CSB to indicate a translation error.
+ *
+ * If we are unable to update the CSB means copy_to_user failed due to
+ * invalid csb_addr, send a signal to the process.
+ *
+ * Remaining settings in the CSB are based on wait_for_csb() of
+ * NX-GZIP.
+ */
+static void update_csb(struct vas_window *window,
+			struct coprocessor_request_block *crb)
+{
+	struct coprocessor_status_block csb;
+	struct kernel_siginfo info;
+	struct task_struct *tsk;
+	void __user *csb_addr;
+	struct pid *pid;
+	int rc;
+
+	/*
+	 * NX user space windows can not be opened for task->mm=NULL
+	 * and faults will not be generated for kernel requests.
+	 */
+	if (WARN_ON_ONCE(!window->mm || !window->user_win))
+		return;
+
+	csb_addr = (void __user *)be64_to_cpu(crb->csb_addr);
+
+	memset(&csb, 0, sizeof(csb));
+	csb.cc = CSB_CC_TRANSLATION;
+	csb.ce = CSB_CE_TERMINATION;
+	csb.cs = 0;
+	csb.count = 0;
+
+	/*
+	 * NX operates and returns in BE format as defined CRB struct.
+	 * So return fault_storage_addr in BE as NX pastes in FIFO and
+	 * expects user space to convert to CPU format.
+	 */
+	csb.address = crb->stamp.nx.fault_storage_addr;
+	csb.flags = 0;
+
+	pid = window->pid;
+	tsk = get_pid_task(pid, PIDTYPE_PID);
+	/*
+	 * Send window will be closed after processing all NX requests
+	 * and process exits after closing all windows. In multi-thread
+	 * applications, thread may not exists, but does not close FD
+	 * (means send window) upon exit. Parent thread (tgid) can use
+	 * and close the window later.
+	 * pid and mm references are taken when window is opened by
+	 * process (pid). So tgid is used only when child thread opens
+	 * a window and exits without closing it in multithread tasks.
+	 */
+	if (!tsk) {
+		pid = window->tgid;
+		tsk = get_pid_task(pid, PIDTYPE_PID);
+		/*
+		 * Parent thread will be closing window during its exit.
+		 * So should not get here.
+		 */
+		if (WARN_ON_ONCE(!tsk))
+			return;
+	}
+
+	/* Return if the task is exiting. */
+	if (tsk->flags & PF_EXITING) {
+		put_task_struct(tsk);
+		return;
+	}
+
+	use_mm(window->mm);
+	rc = copy_to_user(csb_addr, &csb, sizeof(csb));
+	/*
+	 * User space polls on csb.flags (first byte). So add barrier
+	 * then copy first byte with csb flags update.
+	 */
+	if (!rc) {
+		csb.flags = CSB_V;
+		smp_mb();
+		rc = copy_to_user(csb_addr, &csb, sizeof(u8));
+	}
+	unuse_mm(window->mm);
+	put_task_struct(tsk);
+
+	/* Success */
+	if (!rc)
+		return;
+
+	pr_debug("Invalid CSB address 0x%p signalling pid(%d)\n",
+			csb_addr, pid_vnr(pid));
+
+	clear_siginfo(&info);
+	info.si_signo = SIGSEGV;
+	info.si_errno = EFAULT;
+	info.si_code = SEGV_MAPERR;
+	info.si_addr = csb_addr;
+
+	/*
+	 * process will be polling on csb.flags after request is sent to
+	 * NX. So generally CSB update should not fail except when an
+	 * application does not follow the process properly. So an error
+	 * message will be displayed and leave it to user space whether
+	 * to ignore or handle this signal.
+	 */
+	rcu_read_lock();
+	rc = kill_pid_info(SIGSEGV, &info, pid);
+	rcu_read_unlock();
+
+	pr_devel("%s(): pid %d kill_proc_info() rc %d\n", __func__,
+			pid_vnr(pid), rc);
+}
+
+/*
  * Process valid CRBs in fault FIFO.
  */
 irqreturn_t vas_fault_thread_fn(int irq, void *data)
@@ -111,6 +225,7 @@ irqreturn_t vas_fault_thread_fn(int irq, void *data)
 			return IRQ_HANDLED;
 		}
 
+		update_csb(window, crb);
 	} while (true);
 }
 
-- 
1.8.3.1

[PATCH v8 10/14] powerpc/vas: Print CRB and FIFO values

[Haren Myneni]

Dump FIFO entries if could not find send window and print CRB
for debugging.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
 arch/powerpc/platforms/powernv/vas-fault.c | 41 ++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c
index 6eceac5d..40e1de4 100644
--- a/arch/powerpc/platforms/powernv/vas-fault.c
+++ b/arch/powerpc/platforms/powernv/vas-fault.c
@@ -26,6 +26,28 @@
  */
 #define VAS_FAULT_WIN_FIFO_SIZE	(4 << 20)
 
+static void dump_crb(struct coprocessor_request_block *crb)
+{
+	struct data_descriptor_entry *dde;
+	struct nx_fault_stamp *nx;
+
+	dde = &crb->source;
+	pr_devel("SrcDDE: addr 0x%llx, len %d, count %d, idx %d, flags %d\n",
+		be64_to_cpu(dde->address), be32_to_cpu(dde->length),
+		dde->count, dde->index, dde->flags);
+
+	dde = &crb->target;
+	pr_devel("TgtDDE: addr 0x%llx, len %d, count %d, idx %d, flags %d\n",
+		be64_to_cpu(dde->address), be32_to_cpu(dde->length),
+		dde->count, dde->index, dde->flags);
+
+	nx = &crb->stamp.nx;
+	pr_devel("NX Stamp: PSWID 0x%x, FSA 0x%llx, flags 0x%x, FS 0x%x\n",
+		be32_to_cpu(nx->pswid),
+		be64_to_cpu(crb->stamp.nx.fault_storage_addr),
+		nx->flags, nx->fault_status);
+}
+
 /*
  * Update the CSB to indicate a translation error.
  *
@@ -139,6 +161,23 @@ static void update_csb(struct vas_window *window,
 			pid_vnr(pid), rc);
 }
 
+static void dump_fifo(struct vas_instance *vinst, void *entry)
+{
+	unsigned long *end = vinst->fault_fifo + vinst->fault_fifo_size;
+	unsigned long *fifo = entry;
+	int i;
+
+	pr_err("Fault fifo size %d, Max crbs %d\n", vinst->fault_fifo_size,
+			vinst->fault_fifo_size / CRB_SIZE);
+
+	/* Dump 10 CRB entries or until end of FIFO */
+	pr_err("Fault FIFO Dump:\n");
+	for (i = 0; i < 10*(CRB_SIZE/8) && fifo < end; i += 4, fifo += 4) {
+		pr_err("[%.3d, %p]: 0x%.16lx 0x%.16lx 0x%.16lx 0x%.16lx\n",
+			i, fifo, *fifo, *(fifo+1), *(fifo+2), *(fifo+3));
+	}
+}
+
 /*
  * Process valid CRBs in fault FIFO.
  */
@@ -205,6 +244,7 @@ irqreturn_t vas_fault_thread_fn(int irq, void *data)
 				vinst->vas_id, vinst->fault_fifo, fifo,
 				vinst->fault_crbs);
 
+		dump_crb(crb);
 		window = vas_pswid_to_window(vinst,
 				be32_to_cpu(crb->stamp.nx.pswid));
 
@@ -215,6 +255,7 @@ irqreturn_t vas_fault_thread_fn(int irq, void *data)
 			 * even clean it up (return credit).
 			 * But we should not get here.
 			 */
+			dump_fifo(vinst, (void *)entry);
 			pr_err("VAS[%d] fault_fifo %p, fifo %p, pswid 0x%x, fault_crbs %d bad CRB?\n",
 				vinst->vas_id, vinst->fault_fifo, fifo,
 				be32_to_cpu(crb->stamp.nx.pswid),
-- 
1.8.3.1

[PATCH v8 11/14] powerpc/vas: Do not use default credits for receive window

[Haren Myneni]

System checkstops if RxFIFO overruns with more requests than the
maximum possible number of CRBs allowed in FIFO at any time. So
max credits value (rxattr.wcreds_max) is set and is passed to
vas_rx_win_open() by the the driver.

Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
 arch/powerpc/platforms/powernv/vas-window.c | 4 ++--
 arch/powerpc/platforms/powernv/vas.h        | 2 --
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
index e7641a5..6658ccc 100644
--- a/arch/powerpc/platforms/powernv/vas-window.c
+++ b/arch/powerpc/platforms/powernv/vas-window.c
@@ -772,7 +772,7 @@ static bool rx_win_args_valid(enum vas_cop_type cop,
 	if (attr->rx_fifo_size > VAS_RX_FIFO_SIZE_MAX)
 		return false;
 
-	if (attr->wcreds_max > VAS_RX_WCREDS_MAX)
+	if (!attr->wcreds_max)
 		return false;
 
 	if (attr->nx_win) {
@@ -877,7 +877,7 @@ struct vas_window *vas_rx_win_open(int vasid, enum vas_cop_type cop,
 	rxwin->nx_win = rxattr->nx_win;
 	rxwin->user_win = rxattr->user_win;
 	rxwin->cop = cop;
-	rxwin->wcreds_max = rxattr->wcreds_max ?: VAS_WCREDS_DEFAULT;
+	rxwin->wcreds_max = rxattr->wcreds_max;
 
 	init_winctx_for_rxwin(rxwin, rxattr, &winctx);
 	init_winctx_regs(rxwin, &winctx);
diff --git a/arch/powerpc/platforms/powernv/vas.h b/arch/powerpc/platforms/powernv/vas.h
index 16aa8ec..bc728d7 100644
--- a/arch/powerpc/platforms/powernv/vas.h
+++ b/arch/powerpc/platforms/powernv/vas.h
@@ -101,11 +101,9 @@
 /*
  * Initial per-process credits.
  * Max send window credits:    4K-1 (12-bits in VAS_TX_WCRED)
- * Max receive window credits: 64K-1 (16 bits in VAS_LRX_WCRED)
  *
  * TODO: Needs tuning for per-process credits
  */
-#define VAS_RX_WCREDS_MAX		((64 << 10) - 1)
 #define VAS_TX_WCREDS_MAX		((4 << 10) - 1)
 #define VAS_WCREDS_DEFAULT		(1 << 10)
 
-- 
1.8.3.1

[PATCH v8 12/14] powerpc/vas: Return credits after handling fault

[Haren Myneni]

NX expects OS to return credit for send window after processing each
fault. Also credit has to be returned even for fault window.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
 arch/powerpc/platforms/powernv/vas-fault.c  |  9 +++++++++
 arch/powerpc/platforms/powernv/vas-window.c | 17 +++++++++++++++++
 arch/powerpc/platforms/powernv/vas.h        |  1 +
 3 files changed, 27 insertions(+)

diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c
index 40e1de4..292f7ba 100644
--- a/arch/powerpc/platforms/powernv/vas-fault.c
+++ b/arch/powerpc/platforms/powernv/vas-fault.c
@@ -238,6 +238,10 @@ irqreturn_t vas_fault_thread_fn(int irq, void *data)
 		memcpy(crb, fifo, CRB_SIZE);
 		entry->stamp.nx.pswid = cpu_to_be32(FIFO_INVALID_ENTRY);
 		entry->ccw |= cpu_to_be32(CCW0_INVALID);
+		/*
+		 * Return credit for the fault window.
+		 */
+		vas_return_credit(vinst->fault_win, 0);
 		mutex_unlock(&vinst->mutex);
 
 		pr_devel("VAS[%d] fault_fifo %p, fifo %p, fault_crbs %d\n",
@@ -267,6 +271,11 @@ irqreturn_t vas_fault_thread_fn(int irq, void *data)
 		}
 
 		update_csb(window, crb);
+		/*
+		 * Return credit for send window after processing
+		 * fault CRB.
+		 */
+		vas_return_credit(window, 1);
 	} while (true);
 }
 
diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
index 6658ccc..20a2a8b 100644
--- a/arch/powerpc/platforms/powernv/vas-window.c
+++ b/arch/powerpc/platforms/powernv/vas-window.c
@@ -1318,6 +1318,23 @@ int vas_win_close(struct vas_window *window)
 }
 EXPORT_SYMBOL_GPL(vas_win_close);
 
+/*
+ * Return credit for the given window.
+ */
+void vas_return_credit(struct vas_window *window, bool tx)
+{
+	uint64_t val;
+
+	val = 0ULL;
+	if (tx) { /* send window */
+		val = SET_FIELD(VAS_TX_WCRED, val, 1);
+		write_hvwc_reg(window, VREG(TX_WCRED_ADDER), val);
+	} else {
+		val = SET_FIELD(VAS_LRX_WCRED, val, 1);
+		write_hvwc_reg(window, VREG(LRX_WCRED_ADDER), val);
+	}
+}
+
 struct vas_window *vas_pswid_to_window(struct vas_instance *vinst,
 		uint32_t pswid)
 {
diff --git a/arch/powerpc/platforms/powernv/vas.h b/arch/powerpc/platforms/powernv/vas.h
index bc728d7..8c39a7d 100644
--- a/arch/powerpc/platforms/powernv/vas.h
+++ b/arch/powerpc/platforms/powernv/vas.h
@@ -428,6 +428,7 @@ struct vas_winctx {
 extern void vas_window_free_dbgdir(struct vas_window *win);
 extern int vas_setup_fault_window(struct vas_instance *vinst);
 extern irqreturn_t vas_fault_thread_fn(int irq, void *data);
+extern void vas_return_credit(struct vas_window *window, bool tx);
 extern struct vas_window *vas_pswid_to_window(struct vas_instance *vinst,
 						uint32_t pswid);
 
-- 
1.8.3.1

[PATCH v8 13/14] powerpc/vas: Display process stuck message

[Haren Myneni]

Process can not close send window until all requests are processed.
Means wait until window state is not busy and send credits are
returned. Display debug messages in case taking longer to close the
window.

Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
 arch/powerpc/platforms/powernv/vas-window.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
index 20a2a8b..d5754a8 100644
--- a/arch/powerpc/platforms/powernv/vas-window.c
+++ b/arch/powerpc/platforms/powernv/vas-window.c
@@ -1182,6 +1182,7 @@ static void poll_window_credits(struct vas_window *window)
 {
 	u64 val;
 	int creds, mode;
+	int count = 0;
 
 	val = read_hvwc_reg(window, VREG(WINCTL));
 	if (window->tx_win)
@@ -1200,10 +1201,27 @@ static void poll_window_credits(struct vas_window *window)
 		creds = GET_FIELD(VAS_LRX_WCRED, val);
 	}
 
+	/*
+	 * Takes around few milliseconds to complete all pending requests
+	 * and return credits.
+	 * TODO: Scan fault FIFO and invalidate CRBs points to this window
+	 *       and issue CRB Kill to stop all pending requests. Need only
+	 *       if there is a bug in NX or fault handling in kernel.
+	 */
 	if (creds < window->wcreds_max) {
 		val = 0;
 		set_current_state(TASK_UNINTERRUPTIBLE);
 		schedule_timeout(msecs_to_jiffies(10));
+		count++;
+		/*
+		 * Process can not close send window until all credits are
+		 * returned.
+		 */
+		if (!(count % 10000))
+			pr_debug("VAS: pid %d stuck. Waiting for credits returned for Window(%d). creds %d, Retries %d\n",
+				vas_window_pid(window), window->winid,
+				creds, count);
+
 		goto retry;
 	}
 }
@@ -1217,6 +1235,7 @@ static void poll_window_busy_state(struct vas_window *window)
 {
 	int busy;
 	u64 val;
+	int count = 0;
 
 retry:
 	val = read_hvwc_reg(window, VREG(WIN_STATUS));
@@ -1225,6 +1244,15 @@ static void poll_window_busy_state(struct vas_window *window)
 		val = 0;
 		set_current_state(TASK_UNINTERRUPTIBLE);
 		schedule_timeout(msecs_to_jiffies(5));
+		count++;
+		/*
+		 * Takes around few milliseconds to process all pending
+		 * requests.
+		 */
+		if (!(count % 10000))
+			pr_debug("VAS: pid %d stuck. Window (ID=%d) is in busy state. Retries %d\n",
+				vas_window_pid(window), window->winid, count);
+
 		goto retry;
 	}
 }
-- 
1.8.3.1

[PATCH v8 14/14] powerpc/vas: Free send window in VAS instance after credits returned

[Haren Myneni]

NX may be processing requests while trying to close window. Wait until
all credits are returned and then free send window from VAS instance.

Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
 arch/powerpc/platforms/powernv/vas-window.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
index d5754a8..a1d3fb1 100644
--- a/arch/powerpc/platforms/powernv/vas-window.c
+++ b/arch/powerpc/platforms/powernv/vas-window.c
@@ -1317,14 +1317,14 @@ int vas_win_close(struct vas_window *window)
 
 	unmap_paste_region(window);
 
-	clear_vinst_win(window);
-
 	poll_window_busy_state(window);
 
 	unpin_close_window(window);
 
 	poll_window_credits(window);
 
+	clear_vinst_win(window);
+
 	poll_window_castout(window);
 
 	/* if send window, drop reference to matching receive window */
-- 
1.8.3.1

Re: [PATCH v8 01/14] powerpc/xive: Define xive_native_alloc_irq_on_chip()

[Nicholas Piggin]

Haren Myneni's on March 19, 2020 4:12 pm:
> 
> This function allocates IRQ on a specific chip. VAS needs per chip
> IRQ allocation and will have IRQ handler per VAS instance.

Can't see a problem, but don't really know the XIVE code. Cédric seems 
like an obvious omission from CC here.

Thanks,
Nick

> 
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/xive.h   | 9 ++++++++-
>  arch/powerpc/sysdev/xive/native.c | 6 +++---
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
> index 93f982db..d08ea11 100644
> --- a/arch/powerpc/include/asm/xive.h
> +++ b/arch/powerpc/include/asm/xive.h
> @@ -5,6 +5,8 @@
>  #ifndef _ASM_POWERPC_XIVE_H
>  #define _ASM_POWERPC_XIVE_H
>  
> +#include <asm/opal-api.h>
> +
>  #define XIVE_INVALID_VP	0xffffffff
>  
>  #ifdef CONFIG_PPC_XIVE
> @@ -108,7 +110,6 @@ struct xive_q {
>  int xive_native_populate_irq_data(u32 hw_irq,
>  				  struct xive_irq_data *data);
>  void xive_cleanup_irq_data(struct xive_irq_data *xd);
> -u32 xive_native_alloc_irq(void);
>  void xive_native_free_irq(u32 irq);
>  int xive_native_configure_irq(u32 hw_irq, u32 target, u8 prio, u32 sw_irq);
>  
> @@ -137,6 +138,12 @@ int xive_native_set_queue_state(u32 vp_id, uint32_t prio, u32 qtoggle,
>  				u32 qindex);
>  int xive_native_get_vp_state(u32 vp_id, u64 *out_state);
>  bool xive_native_has_queue_state_support(void);
> +extern u32 xive_native_alloc_irq_on_chip(u32 chip_id);
> +
> +static inline u32 xive_native_alloc_irq(void)
> +{
> +	return xive_native_alloc_irq_on_chip(OPAL_XIVE_ANY_CHIP);
> +}
>  
>  #else
>  
> diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
> index 0ff6b73..14d4406 100644
> --- a/arch/powerpc/sysdev/xive/native.c
> +++ b/arch/powerpc/sysdev/xive/native.c
> @@ -279,12 +279,12 @@ static int xive_native_get_ipi(unsigned int cpu, struct xive_cpu *xc)
>  }
>  #endif /* CONFIG_SMP */
>  
> -u32 xive_native_alloc_irq(void)
> +u32 xive_native_alloc_irq_on_chip(u32 chip_id)
>  {
>  	s64 rc;
>  
>  	for (;;) {
> -		rc = opal_xive_allocate_irq(OPAL_XIVE_ANY_CHIP);
> +		rc = opal_xive_allocate_irq(chip_id);
>  		if (rc != OPAL_BUSY)
>  			break;
>  		msleep(OPAL_BUSY_DELAY_MS);
> @@ -293,7 +293,7 @@ u32 xive_native_alloc_irq(void)
>  		return 0;
>  	return rc;
>  }
> -EXPORT_SYMBOL_GPL(xive_native_alloc_irq);
> +EXPORT_SYMBOL_GPL(xive_native_alloc_irq_on_chip);
>  
>  void xive_native_free_irq(u32 irq)
>  {
> -- 
> 1.8.3.1
> 
> 
> 
> 

Re: [PATCH v8 03/14] powerpc/vas: Define nx_fault_stamp in coprocessor_request_block

[Nicholas Piggin]

Haren Myneni's on March 19, 2020 4:13 pm:
> 
> Kernel sets fault address and status in CRB for NX page fault on user
> space address after processing page fault. User space gets the signal
> and handles the fault mentioned in CRB by bringing the page in to
> memory and send NX request again.
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/icswx.h | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/icswx.h b/arch/powerpc/include/asm/icswx.h
> index 9872f85..b233d1e 100644
> --- a/arch/powerpc/include/asm/icswx.h
> +++ b/arch/powerpc/include/asm/icswx.h

"icswx" is not a thing anymore, after 6ff4d3e96652 ("powerpc: Remove old 
unused icswx based coprocessor support"). I guess NX is reusing some 
things from it, but it would be good to get rid of the cruft and re-name
this file and and relevant names.

NX already uses this file, so I guesss that can happen after this series.

Thanks,
Nick

Re: [PATCH v8 03/14] powerpc/vas: Define nx_fault_stamp in coprocessor_request_block

[Haren Myneni]

On Mon, 2020-03-23 at 10:30 +1000, Nicholas Piggin wrote:
> Haren Myneni's on March 19, 2020 4:13 pm:
> > 
> > Kernel sets fault address and status in CRB for NX page fault on user
> > space address after processing page fault. User space gets the signal
> > and handles the fault mentioned in CRB by bringing the page in to
> > memory and send NX request again.
> > 
> > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> > ---
> >  arch/powerpc/include/asm/icswx.h | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/include/asm/icswx.h b/arch/powerpc/include/asm/icswx.h
> > index 9872f85..b233d1e 100644
> > --- a/arch/powerpc/include/asm/icswx.h
> > +++ b/arch/powerpc/include/asm/icswx.h
> 
> "icswx" is not a thing anymore, after 6ff4d3e96652 ("powerpc: Remove old 
> unused icswx based coprocessor support"). I guess NX is reusing some 
> things from it, but it would be good to get rid of the cruft and re-name
> this file and and relevant names.
> 
> NX already uses this file, so I guesss that can happen after this series.

But NX uses icswx on P8 and icswx.h has only NX specific macros right
now. 

> 
> Thanks,
> Nick
> 

Re: [PATCH v8 04/14] powerpc/vas: Alloc and setup IRQ and trigger port address

[Nicholas Piggin]

Haren Myneni's on March 19, 2020 4:14 pm:
> 
> Alloc IRQ and get trigger port address for each VAS instance. Kernel
> register this IRQ per VAS instance and sets this port for each send
> window. NX interrupts the kernel when it sees page fault.

Again, should cc Cedric and Greg for XIVE / interrupt stuff. And
for patch 2/14.

The changelogs could use a bit of work. They're hard to read, and it can 
be a bit hard to decipher "why".

  Allocate a xive irq on each chip with a vas instance. The NX 
  coprocessor raises a host CPU interrupt via vas if it encounters a
  page fault on an effective address. Subsequent patches register the
  trigger port with the NX coprocessor, and create a vas fault handler
  for this interrupt mapping.

Don't know if the technical details are correct, but something like that
in structure.

Thanks,
Nick

Re: [PATCH v8 03/14] powerpc/vas: Define nx_fault_stamp in coprocessor_request_block

[Nicholas Piggin]

Haren Myneni's on March 23, 2020 10:57 am:
> On Mon, 2020-03-23 at 10:30 +1000, Nicholas Piggin wrote:
>> Haren Myneni's on March 19, 2020 4:13 pm:
>> > 
>> > Kernel sets fault address and status in CRB for NX page fault on user
>> > space address after processing page fault. User space gets the signal
>> > and handles the fault mentioned in CRB by bringing the page in to
>> > memory and send NX request again.
>> > 
>> > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
>> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
>> > ---
>> >  arch/powerpc/include/asm/icswx.h | 18 +++++++++++++++++-
>> >  1 file changed, 17 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/arch/powerpc/include/asm/icswx.h b/arch/powerpc/include/asm/icswx.h
>> > index 9872f85..b233d1e 100644
>> > --- a/arch/powerpc/include/asm/icswx.h
>> > +++ b/arch/powerpc/include/asm/icswx.h
>> 
>> "icswx" is not a thing anymore, after 6ff4d3e96652 ("powerpc: Remove old 
>> unused icswx based coprocessor support"). I guess NX is reusing some 
>> things from it, but it would be good to get rid of the cruft and re-name
>> this file and and relevant names.
>> 
>> NX already uses this file, so I guesss that can happen after this series.
> 
> But NX uses icswx on P8 and icswx.h has only NX specific macros right
> now. 

Oh P8 in kernel is still using it? Ignore me then.

Thanks,
Nick

Re: [PATCH v8 06/14] powerpc/vas: Setup thread IRQ handler per VAS instance

[Nicholas Piggin]

Haren Myneni's on March 19, 2020 4:15 pm:
> 
> Setup thread IRQ handler per each VAS instance. When NX sees a fault
> on CRB, kernel gets an interrupt and vas_fault_handler will be
> executed to process fault CRBs. Read all valid CRBs from fault FIFO,
> determine the corresponding send window from CRB and process fault
> requests.

Perhaps some more overview/why.

"If NX encounters a translation error when accessing the CRB or one
of addresses in the request, it raises an interrupt on the CPU to
handle the fault.


> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
>  arch/powerpc/platforms/powernv/vas-fault.c  | 90 +++++++++++++++++++++++++++++
>  arch/powerpc/platforms/powernv/vas-window.c | 60 +++++++++++++++++++
>  arch/powerpc/platforms/powernv/vas.c        | 49 +++++++++++++++-
>  arch/powerpc/platforms/powernv/vas.h        |  6 ++
>  4 files changed, 204 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c
> index 4044998..1c6d5cc 100644
> --- a/arch/powerpc/platforms/powernv/vas-fault.c
> +++ b/arch/powerpc/platforms/powernv/vas-fault.c
> @@ -11,6 +11,7 @@
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
>  #include <linux/kthread.h>
> +#include <linux/mmu_context.h>
>  #include <asm/icswx.h>
>  
>  #include "vas.h"
> @@ -25,6 +26,95 @@
>  #define VAS_FAULT_WIN_FIFO_SIZE	(4 << 20)
>  
>  /*
> + * Process valid CRBs in fault FIFO.
> + */
> +irqreturn_t vas_fault_thread_fn(int irq, void *data)

Are page faults the only reason why VAS would raise this interrupt? Is 
NX really the only possible user of this, so you can have NX specifics
in here?

> +{
> +	struct vas_instance *vinst = data;
> +	struct coprocessor_request_block *crb, *entry;
> +	struct coprocessor_request_block buf;
> +	struct vas_window *window;
> +	unsigned long flags;
> +	void *fifo;
> +
> +	crb = &buf;

The below comment could just be moved to replace the one at the top of the
function. Can you explain slightly more about how the faults work, and 
be more clear about what the coprocessor does versus what the host does? The
use of VAS and NX is a bit confusing too. VAS doesn't interrupt with
page faults, does it? NX has the page fault(s), and it requests VAS to
interrupt the host?

> +
> +	/*
> +	 * VAS can interrupt with multiple page faults. So process all
> +	 * valid CRBs within fault FIFO until reaches invalid CRB.

 When NX encounters a fault accessing a memory address for a particular
 CRB, it updates the nx_fault_stamp field in the CRB (to what?), and 
 copies the CRB to the fault FIFO memory, then raises an interrupt on the
 CPU (memory ordering on the store and load sides are provided how?). NX
 can store multiple faults into the FIFO per interrupt (does it proceed
 asynchronously after the interrupt? what's the stopping condition?).

 When the CPU takes this interrupt, it reads the faulting CRBs from the
 FIFO and processes them in order until it reaches an invalid entry, FIFO
 empty (memory ordering how?). After each FIFO entry is processed, store
 to mark them as invalid. (How does NX resume after this?)

How is the fault actually even "handled" here? Nothing seems to be
actually done for them.

> +	 * NX updates nx_fault_stamp in CRB and pastes in fault FIFO.
> +	 * kernel retrives send window from parition send window ID
> +	 * (pswid) in nx_fault_stamp. So pswid should be valid and
> +	 * ccw[0] (in be) should be zero since this bit is reserved.
> +	 * If user space touches this bit, NX returns with "CRB format
> +	 * error".
> +	 *
> +	 * After reading CRB entry, invalidate it with pswid (set
> +	 * 0xffffffff) and ccw[0] (set to 1).

Al this is very busy and hard to decipher unambiguously. It should read
more like a spec, a precise sequence of things happening.

> +	 *
> +	 * In case kernel receives another interrupt with different page
> +	 * fault, CRBs are already processed by the previous handling. So
> +	 * will be returned from this function when it sees invalid CRB.
> +	 */

Ambiguous at best. Assuming the NX continues running asynchronously and
it's a usual kind of FIFO, I assume this means if the kernel gets 
another interrupt for a page fault corresponding to a FIFO entry that
has already been processed by this fault.


> +	do {

Can you make this 'while (true)' or 'for (;;)' so you don't need to go
to the bottom to see it's an infinite loop.

> +		mutex_lock(&vinst->mutex);

What does this protect? Threaded handlers don't run concurrently for the
same request_threaded_irq?

> +
> +		spin_lock_irqsave(&vinst->fault_lock, flags);
> +		/*
> +		 * Advance the fault fifo pointer to next CRB.

The code below the comment isn't advancing the fault fifo pointer, it's
grabbing the current one. The pointer (fault_crbs) is advanced later. 
You presumabl don't want to advance over an invalid entry.

> +		 * Use CRB_SIZE rather than sizeof(*crb) since the latter is
> +		 * aligned to CRB_ALIGN (256) but the CRB written to by VAS is
> +		 * only CRB_SIZE in len.
> +		 */
> +		fifo = vinst->fault_fifo + (vinst->fault_crbs * CRB_SIZE);
> +		entry = fifo;

Don't think you should really do this. It may be harmless in this case,
but the compiler expects the type to be aligned. Make it another type,
like coprocessor_fault_block or something?

> +
> +		if ((entry->stamp.nx.pswid == cpu_to_be32(FIFO_INVALID_ENTRY))
> +			|| (entry->ccw & cpu_to_be32(CCW0_INVALID))) {
> +			atomic_set(&vinst->faults_in_progress, 0);
> +			spin_unlock_irqrestore(&vinst->fault_lock, flags);

So what does the fault_lock protect? The only data it protects is 
faults_in_progress (vs the hard interrupt handler), which doesn't 
achieve anything by itself, so I guess it also prevents the hard irq
handler from returning until the handler here has checked that the
fault FIFO is empty then returns IRQ_HANDLED? That seems fine (so long
as memory ordering details are okay), but it should be documented
that way.

Also why is the hard handler in a different file? Makes it harder to
see how this works at a glance.

faults_in_progress does not have to be atomic because it's always
accessed under the lock. And IMO it should have a better name. If the
NX can be causing more faults as we go, it really doesn't indicate
anything about faults. It's whether or not the threaded handler is
currently woken and processing faults.

> +			mutex_unlock(&vinst->mutex);
> +			return IRQ_HANDLED;
> +		}
> +
> +		spin_unlock_irqrestore(&vinst->fault_lock, flags);
> +		vinst->fault_crbs++;
> +		if (vinst->fault_crbs == (vinst->fault_fifo_size / CRB_SIZE))
> +			vinst->fault_crbs = 0;
> +
> +		memcpy(crb, fifo, CRB_SIZE);
> +		entry->stamp.nx.pswid = cpu_to_be32(FIFO_INVALID_ENTRY);
> +		entry->ccw |= cpu_to_be32(CCW0_INVALID);
> +		mutex_unlock(&vinst->mutex);
> +
> +		pr_devel("VAS[%d] fault_fifo %p, fifo %p, fault_crbs %d\n",
> +				vinst->vas_id, vinst->fault_fifo, fifo,
> +				vinst->fault_crbs);
> +
> +		window = vas_pswid_to_window(vinst,
> +				be32_to_cpu(crb->stamp.nx.pswid));
> +
> +		if (IS_ERR(window)) {
> +			/*
> +			 * We got an interrupt about a specific send
> +			 * window but we can't find that window and we can't
> +			 * even clean it up (return credit).
> +			 * But we should not get here.
> +			 */
> +			pr_err("VAS[%d] fault_fifo %p, fifo %p, pswid 0x%x, fault_crbs %d bad CRB?\n",
> +				vinst->vas_id, vinst->fault_fifo, fifo,
> +				be32_to_cpu(crb->stamp.nx.pswid),
> +				vinst->fault_crbs);
> +
> +			WARN_ON_ONCE(1);
> +			atomic_set(&vinst->faults_in_progress, 0);
> +			return IRQ_HANDLED;

Shouldn't get here but you have a handler for it, so it should try to
be graceful. Keep processing the rest of the FIFO until it's empty
otherwise you have a missed wakeup here? Probably less code too, just
delete the last 2 lines.

Thanks,
Nick

> +		}
> +
> +	} while (true);
> +}
> +
> +/*
>   * Fault window is opened per VAS instance. NX pastes fault CRB in fault
>   * FIFO upon page faults.
>   */
> diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
> index 1783fa9..1f31c18 100644
> --- a/arch/powerpc/platforms/powernv/vas-window.c
> +++ b/arch/powerpc/platforms/powernv/vas-window.c
> @@ -1040,6 +1040,15 @@ struct vas_window *vas_tx_win_open(int vasid, enum vas_cop_type cop,
>  		}
>  	} else {
>  		/*
> +		 * Interrupt hanlder or fault window setup failed. Means
> +		 * NX can not generate fault for page fault. So not
> +		 * opening for user space tx window.
> +		 */
> +		if (!vinst->virq) {
> +			rc = -ENODEV;
> +			goto free_window;
> +		}
> +		/*
>  		 * A user mapping must ensure that context switch issues
>  		 * CP_ABORT for this thread.
>  		 */
> @@ -1254,3 +1263,54 @@ int vas_win_close(struct vas_window *window)
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(vas_win_close);
> +
> +struct vas_window *vas_pswid_to_window(struct vas_instance *vinst,
> +		uint32_t pswid)
> +{
> +	struct vas_window *window;
> +	int winid;
> +
> +	if (!pswid) {
> +		pr_devel("%s: called for pswid 0!\n", __func__);
> +		return ERR_PTR(-ESRCH);
> +	}
> +
> +	decode_pswid(pswid, NULL, &winid);
> +
> +	if (winid >= VAS_WINDOWS_PER_CHIP)
> +		return ERR_PTR(-ESRCH);
> +
> +	/*
> +	 * If application closes the window before the hardware
> +	 * returns the fault CRB, we should wait in vas_win_close()
> +	 * for the pending requests. so the window must be active
> +	 * and the process alive.
> +	 *
> +	 * If its a kernel process, we should not get any faults and
> +	 * should not get here.
> +	 */
> +	window = vinst->windows[winid];
> +
> +	if (!window) {
> +		pr_err("PSWID decode: Could not find window for winid %d pswid %d vinst 0x%p\n",
> +			winid, pswid, vinst);
> +		return NULL;
> +	}
> +
> +	/*
> +	 * Do some sanity checks on the decoded window.  Window should be
> +	 * NX GZIP user send window. FTW windows should not incur faults
> +	 * since their CRBs are ignored (not queued on FIFO or processed
> +	 * by NX).
> +	 */
> +	if (!window->tx_win || !window->user_win || !window->nx_win ||
> +			window->cop == VAS_COP_TYPE_FAULT ||
> +			window->cop == VAS_COP_TYPE_FTW) {
> +		pr_err("PSWID decode: id %d, tx %d, user %d, nx %d, cop %d\n",
> +			winid, window->tx_win, window->user_win,
> +			window->nx_win, window->cop);
> +		WARN_ON(1);
> +	}
> +
> +	return window;
> +}
> diff --git a/arch/powerpc/platforms/powernv/vas.c b/arch/powerpc/platforms/powernv/vas.c
> index 557c8e4..3d9ba58 100644
> --- a/arch/powerpc/platforms/powernv/vas.c
> +++ b/arch/powerpc/platforms/powernv/vas.c
> @@ -14,6 +14,8 @@
>  #include <linux/of_platform.h>
>  #include <linux/of_address.h>
>  #include <linux/of.h>
> +#include <linux/irqdomain.h>
> +#include <linux/interrupt.h>
>  #include <asm/prom.h>
>  #include <asm/xive.h>
>  
> @@ -24,9 +26,53 @@
>  
>  static DEFINE_PER_CPU(int, cpu_vas_id);
>  
> +static irqreturn_t vas_fault_handler(int irq, void *dev_id)
> +{
> +	struct vas_instance *vinst = dev_id;
> +	irqreturn_t ret = IRQ_WAKE_THREAD;
> +	unsigned long flags;
> +
> +	/*
> +	 * NX can generate an interrupt for multiple faults. So the
> +	 * fault handler thread process all CRBs until finds invalid
> +	 * entry. In case if NX sees continuous faults, it is possible
> +	 * that the thread function entered with the first interrupt
> +	 * can execute and process all valid CRBs.
> +	 * So wake up thread only if the fault thread is not in progress.
> +	 */
> +	spin_lock_irqsave(&vinst->fault_lock, flags);
> +
> +	if (atomic_read(&vinst->faults_in_progress))
> +		ret = IRQ_HANDLED;
> +	else
> +		atomic_set(&vinst->faults_in_progress, 1);
> +
> +	spin_unlock_irqrestore(&vinst->fault_lock, flags);
> +
> +	return ret;
> +}
> +
>  static int vas_irq_fault_window_setup(struct vas_instance *vinst)
>  {
> -	return vas_setup_fault_window(vinst);
> +	char devname[64];
> +	int rc = 0;
> +
> +	snprintf(devname, sizeof(devname), "vas-%d", vinst->vas_id);
> +	rc = request_threaded_irq(vinst->virq, vas_fault_handler,
> +				vas_fault_thread_fn, 0, devname, vinst);
> +
> +	if (rc) {
> +		pr_err("VAS[%d]: Request IRQ(%d) failed with %d\n",
> +				vinst->vas_id, vinst->virq, rc);
> +		goto out;
> +	}
> +
> +	rc = vas_setup_fault_window(vinst);
> +	if (rc)
> +		free_irq(vinst->virq, vinst);
> +
> +out:
> +	return rc;
>  }
>  
>  static int init_vas_instance(struct platform_device *pdev)
> @@ -109,6 +155,7 @@ static int init_vas_instance(struct platform_device *pdev)
>  	list_add(&vinst->node, &vas_instances);
>  	mutex_unlock(&vas_mutex);
>  
> +	spin_lock_init(&vinst->fault_lock);
>  	/*
>  	 * IRQ and fault handling setup is needed only for user space
>  	 * send windows.
> diff --git a/arch/powerpc/platforms/powernv/vas.h b/arch/powerpc/platforms/powernv/vas.h
> index 6c4baf5..ecae7cd 100644
> --- a/arch/powerpc/platforms/powernv/vas.h
> +++ b/arch/powerpc/platforms/powernv/vas.h
> @@ -326,7 +326,10 @@ struct vas_instance {
>  
>  	u64 irq_port;
>  	int virq;
> +	int fault_crbs;
>  	int fault_fifo_size;
> +	atomic_t faults_in_progress;
> +	spinlock_t fault_lock;
>  	void *fault_fifo;
>  	struct vas_window *fault_win; /* Fault window */
>  
> @@ -424,6 +427,9 @@ struct vas_winctx {
>  extern void vas_window_init_dbgdir(struct vas_window *win);
>  extern void vas_window_free_dbgdir(struct vas_window *win);
>  extern int vas_setup_fault_window(struct vas_instance *vinst);
> +extern irqreturn_t vas_fault_thread_fn(int irq, void *data);
> +extern struct vas_window *vas_pswid_to_window(struct vas_instance *vinst,
> +						uint32_t pswid);
>  
>  static inline void vas_log_write(struct vas_window *win, char *name,
>  			void *regptr, u64 val)
> -- 
> 1.8.3.1
> 
> 
> 
> 

Re: [PATCH v8 08/14] powerpc/vas: Take reference to PID and mm for user space windows

[Nicholas Piggin]

Haren Myneni's on March 19, 2020 4:16 pm:
> 
> When process opens a window, its pid and tgid will be saved in vas_window
> struct. This window will be closed when the process exits. Kernel handles
> NX faults by updating CSB or send SEGV signal to pid if user space csb_addr
> is invalid.

Bit of a nitpick, but can you use articles consistently ("the", "a")? I
won't keep nitpicking changelogs but I think they could be made easier 
to read. I'm happy to help proof read and suggest things offline when 
you're happy with the technical content of them, let me know.

> 
> In multi-thread applications, a window can be opened by child thread, but
> it will not be closed when this thread exits. Expects parent to clean up
> all resources including NX windows. Child thread can send requests using
> this window and can be killed before they are completed. But the pid
> assigned to this thread can be reused for other task while requests are
> pending. If the csb_addr passed in these requests is invalid, kernel will
> end up sending signal to the wrong task.
> 
> To prevent reusing the pid, take references to pid and mm when the window
> is opened and release them during window close.

We went over this together a while back, but task management isn't 
something I look at every day and it's complicated and easy to introduce
bugs. I suggest if we can get the changelog and comments written well
and understandable for someone who does not know or care about vas, 
then cc linux-kernel and the maintainers, and hopefully someone will 
take a look. It's not a large patch so if assumptions and concurrency
etc is documented, then it shouldn't be too much work.

Thanks,
Nick

> 
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
>  arch/powerpc/platforms/powernv/vas-debug.c  |  2 +-
>  arch/powerpc/platforms/powernv/vas-window.c | 53 ++++++++++++++++++++++++++---
>  arch/powerpc/platforms/powernv/vas.h        |  9 ++++-
>  3 files changed, 57 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/vas-debug.c b/arch/powerpc/platforms/powernv/vas-debug.c
> index 09e63df..ef9a717 100644
> --- a/arch/powerpc/platforms/powernv/vas-debug.c
> +++ b/arch/powerpc/platforms/powernv/vas-debug.c
> @@ -38,7 +38,7 @@ static int info_show(struct seq_file *s, void *private)
>  
>  	seq_printf(s, "Type: %s, %s\n", cop_to_str(window->cop),
>  					window->tx_win ? "Send" : "Receive");
> -	seq_printf(s, "Pid : %d\n", window->pid);
> +	seq_printf(s, "Pid : %d\n", vas_window_pid(window));
>  
>  unlock:
>  	mutex_unlock(&vas_mutex);
> diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
> index acb6a22..e7641a5 100644
> --- a/arch/powerpc/platforms/powernv/vas-window.c
> +++ b/arch/powerpc/platforms/powernv/vas-window.c
> @@ -12,6 +12,8 @@
>  #include <linux/log2.h>
>  #include <linux/rcupdate.h>
>  #include <linux/cred.h>
> +#include <linux/sched/mm.h>
> +#include <linux/mmu_context.h>
>  #include <asm/switch_to.h>
>  #include <asm/ppc-opcode.h>
>  #include "vas.h"
> @@ -876,8 +878,6 @@ struct vas_window *vas_rx_win_open(int vasid, enum vas_cop_type cop,
>  	rxwin->user_win = rxattr->user_win;
>  	rxwin->cop = cop;
>  	rxwin->wcreds_max = rxattr->wcreds_max ?: VAS_WCREDS_DEFAULT;
> -	if (rxattr->user_win)
> -		rxwin->pid = task_pid_vnr(current);
>  
>  	init_winctx_for_rxwin(rxwin, rxattr, &winctx);
>  	init_winctx_regs(rxwin, &winctx);
> @@ -1027,7 +1027,6 @@ struct vas_window *vas_tx_win_open(int vasid, enum vas_cop_type cop,
>  	txwin->tx_win = 1;
>  	txwin->rxwin = rxwin;
>  	txwin->nx_win = txwin->rxwin->nx_win;
> -	txwin->pid = attr->pid;
>  	txwin->user_win = attr->user_win;
>  	txwin->wcreds_max = attr->wcreds_max ?: VAS_WCREDS_DEFAULT;
>  
> @@ -1068,8 +1067,43 @@ struct vas_window *vas_tx_win_open(int vasid, enum vas_cop_type cop,
>  			goto free_window;
>  	}
>  
> -	set_vinst_win(vinst, txwin);
> +	if (txwin->user_win) {
> +		/*
> +		 * Window opened by child thread may not be closed when
> +		 * it exits. So take reference to its pid and release it
> +		 * when the window is free by parent thread.
> +		 * Acquire a reference to the task's pid to make sure
> +		 * pid will not be re-used - needed only for multithread
> +		 * applications.
> +		 */
> +		txwin->pid = get_task_pid(current, PIDTYPE_PID);
> +		/*
> +		 * Acquire a reference to the task's mm.
> +		 */
> +		txwin->mm = get_task_mm(current);
>  
> +		if (!txwin->mm) {
> +			put_pid(txwin->pid);
> +			pr_err("VAS: pid(%d): mm_struct is not found\n",
> +					current->pid);
> +			rc = -EPERM;
> +			goto free_window;
> +		}
> +
> +		mmgrab(txwin->mm);
> +		mmput(txwin->mm);
> +		mm_context_add_copro(txwin->mm);
> +		/*
> +		 * Process closes window during exit. In the case of
> +		 * multithread application, child can open window and
> +		 * can exit without closing it. Expects parent thread
> +		 * to use and close the window. So do not need to take
> +		 * pid reference for parent thread.
> +		 */
> +		txwin->tgid = find_get_pid(task_tgid_vnr(current));
> +	}
> +
> +	set_vinst_win(vinst, txwin);
>  	return txwin;
>  
>  free_window:
> @@ -1266,8 +1300,17 @@ int vas_win_close(struct vas_window *window)
>  	poll_window_castout(window);
>  
>  	/* if send window, drop reference to matching receive window */
> -	if (window->tx_win)
> +	if (window->tx_win) {
> +		if (window->user_win) {
> +			/* Drop references to pid and mm */
> +			put_pid(window->pid);
> +			if (window->mm) {
> +				mm_context_remove_copro(window->mm);
> +				mmdrop(window->mm);
> +			}
> +		}
>  		put_rx_win(window->rxwin);
> +	}
>  
>  	vas_window_free(window);
>  
> diff --git a/arch/powerpc/platforms/powernv/vas.h b/arch/powerpc/platforms/powernv/vas.h
> index 310b8a0..16aa8ec 100644
> --- a/arch/powerpc/platforms/powernv/vas.h
> +++ b/arch/powerpc/platforms/powernv/vas.h
> @@ -353,7 +353,9 @@ struct vas_window {
>  	bool user_win;		/* True if user space window */
>  	void *hvwc_map;		/* HV window context */
>  	void *uwc_map;		/* OS/User window context */
> -	pid_t pid;		/* Linux process id of owner */
> +	struct pid *pid;	/* Linux process id of owner */
> +	struct pid *tgid;	/* Thread group ID of owner */
> +	struct mm_struct *mm;	/* Linux process mm_struct */
>  	int wcreds_max;		/* Window credits */
>  
>  	char *dbgname;
> @@ -431,6 +433,11 @@ struct vas_winctx {
>  extern struct vas_window *vas_pswid_to_window(struct vas_instance *vinst,
>  						uint32_t pswid);
>  
> +static inline int vas_window_pid(struct vas_window *window)
> +{
> +	return pid_vnr(window->pid);
> +}
> +
>  static inline void vas_log_write(struct vas_window *win, char *name,
>  			void *regptr, u64 val)
>  {
> -- 
> 1.8.3.1
> 
> 
> 
> 

Re: [PATCH v8 09/14] powerpc/vas: Update CSB and notify process for fault CRBs

[Nicholas Piggin]

Haren Myneni's on March 19, 2020 4:17 pm:
> 
> For each fault CRB, update fault address in CRB (fault_storage_addr)
> and translation error status in CSB so that user space can touch the
> fault address and resend the request. If the user space passed invalid
> CSB address send signal to process with SIGSEGV.

This is where the actual fault handling is done? Does this need to be 
split from the other patch? Why not merge them and put it after the
reference counting one?

I'll wait until comments and questions on the first fault handling patch 
are resolved before I look at this one.

Thanks,
Nick

> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
>  arch/powerpc/platforms/powernv/vas-fault.c | 115 +++++++++++++++++++++++++++++
>  1 file changed, 115 insertions(+)
> 
> diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c
> index 1c6d5cc..6eceac5d 100644
> --- a/arch/powerpc/platforms/powernv/vas-fault.c
> +++ b/arch/powerpc/platforms/powernv/vas-fault.c
> @@ -11,6 +11,7 @@
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
>  #include <linux/kthread.h>
> +#include <linux/sched/signal.h>
>  #include <linux/mmu_context.h>
>  #include <asm/icswx.h>
>  
> @@ -26,6 +27,119 @@
>  #define VAS_FAULT_WIN_FIFO_SIZE	(4 << 20)
>  
>  /*
> + * Update the CSB to indicate a translation error.
> + *
> + * If we are unable to update the CSB means copy_to_user failed due to
> + * invalid csb_addr, send a signal to the process.
> + *
> + * Remaining settings in the CSB are based on wait_for_csb() of
> + * NX-GZIP.
> + */
> +static void update_csb(struct vas_window *window,
> +			struct coprocessor_request_block *crb)
> +{
> +	struct coprocessor_status_block csb;
> +	struct kernel_siginfo info;
> +	struct task_struct *tsk;
> +	void __user *csb_addr;
> +	struct pid *pid;
> +	int rc;
> +
> +	/*
> +	 * NX user space windows can not be opened for task->mm=NULL
> +	 * and faults will not be generated for kernel requests.
> +	 */
> +	if (WARN_ON_ONCE(!window->mm || !window->user_win))
> +		return;
> +
> +	csb_addr = (void __user *)be64_to_cpu(crb->csb_addr);
> +
> +	memset(&csb, 0, sizeof(csb));
> +	csb.cc = CSB_CC_TRANSLATION;
> +	csb.ce = CSB_CE_TERMINATION;
> +	csb.cs = 0;
> +	csb.count = 0;
> +
> +	/*
> +	 * NX operates and returns in BE format as defined CRB struct.
> +	 * So return fault_storage_addr in BE as NX pastes in FIFO and
> +	 * expects user space to convert to CPU format.
> +	 */
> +	csb.address = crb->stamp.nx.fault_storage_addr;
> +	csb.flags = 0;
> +
> +	pid = window->pid;
> +	tsk = get_pid_task(pid, PIDTYPE_PID);
> +	/*
> +	 * Send window will be closed after processing all NX requests
> +	 * and process exits after closing all windows. In multi-thread
> +	 * applications, thread may not exists, but does not close FD
> +	 * (means send window) upon exit. Parent thread (tgid) can use
> +	 * and close the window later.
> +	 * pid and mm references are taken when window is opened by
> +	 * process (pid). So tgid is used only when child thread opens
> +	 * a window and exits without closing it in multithread tasks.
> +	 */
> +	if (!tsk) {
> +		pid = window->tgid;
> +		tsk = get_pid_task(pid, PIDTYPE_PID);
> +		/*
> +		 * Parent thread will be closing window during its exit.
> +		 * So should not get here.
> +		 */
> +		if (WARN_ON_ONCE(!tsk))
> +			return;
> +	}
> +
> +	/* Return if the task is exiting. */
> +	if (tsk->flags & PF_EXITING) {
> +		put_task_struct(tsk);
> +		return;
> +	}
> +
> +	use_mm(window->mm);
> +	rc = copy_to_user(csb_addr, &csb, sizeof(csb));
> +	/*
> +	 * User space polls on csb.flags (first byte). So add barrier
> +	 * then copy first byte with csb flags update.
> +	 */
> +	if (!rc) {
> +		csb.flags = CSB_V;
> +		smp_mb();
> +		rc = copy_to_user(csb_addr, &csb, sizeof(u8));
> +	}
> +	unuse_mm(window->mm);
> +	put_task_struct(tsk);
> +
> +	/* Success */
> +	if (!rc)
> +		return;
> +
> +	pr_debug("Invalid CSB address 0x%p signalling pid(%d)\n",
> +			csb_addr, pid_vnr(pid));
> +
> +	clear_siginfo(&info);
> +	info.si_signo = SIGSEGV;
> +	info.si_errno = EFAULT;
> +	info.si_code = SEGV_MAPERR;
> +	info.si_addr = csb_addr;
> +
> +	/*
> +	 * process will be polling on csb.flags after request is sent to
> +	 * NX. So generally CSB update should not fail except when an
> +	 * application does not follow the process properly. So an error
> +	 * message will be displayed and leave it to user space whether
> +	 * to ignore or handle this signal.
> +	 */
> +	rcu_read_lock();
> +	rc = kill_pid_info(SIGSEGV, &info, pid);
> +	rcu_read_unlock();
> +
> +	pr_devel("%s(): pid %d kill_proc_info() rc %d\n", __func__,
> +			pid_vnr(pid), rc);
> +}
> +
> +/*
>   * Process valid CRBs in fault FIFO.
>   */
>  irqreturn_t vas_fault_thread_fn(int irq, void *data)
> @@ -111,6 +225,7 @@ irqreturn_t vas_fault_thread_fn(int irq, void *data)
>  			return IRQ_HANDLED;
>  		}
>  
> +		update_csb(window, crb);
>  	} while (true);
>  }
>  
> -- 
> 1.8.3.1
> 
> 
> 
> 

Re: [PATCH v8 11/14] powerpc/vas: Do not use default credits for receive window

[Nicholas Piggin]

Haren Myneni's on March 19, 2020 4:18 pm:
> 
> System checkstops if RxFIFO overruns with more requests than the
> maximum possible number of CRBs allowed in FIFO at any time. So
> max credits value (rxattr.wcreds_max) is set and is passed to
> vas_rx_win_open() by the the driver.

This seems like it should be a bug fix or merged in the NX fault
window register patch or something.

Thanks,
Nick

Re: [PATCH v8 12/14] powerpc/vas: Return credits after handling fault

[Nicholas Piggin]

Haren Myneni's on March 19, 2020 4:19 pm:
> 
> NX expects OS to return credit for send window after processing each
> fault. Also credit has to be returned even for fault window.

And this should be merged in the fault handler function.

> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
>  arch/powerpc/platforms/powernv/vas-fault.c  |  9 +++++++++
>  arch/powerpc/platforms/powernv/vas-window.c | 17 +++++++++++++++++
>  arch/powerpc/platforms/powernv/vas.h        |  1 +
>  3 files changed, 27 insertions(+)
> 
> diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c
> index 40e1de4..292f7ba 100644
> --- a/arch/powerpc/platforms/powernv/vas-fault.c
> +++ b/arch/powerpc/platforms/powernv/vas-fault.c
> @@ -238,6 +238,10 @@ irqreturn_t vas_fault_thread_fn(int irq, void *data)
>  		memcpy(crb, fifo, CRB_SIZE);
>  		entry->stamp.nx.pswid = cpu_to_be32(FIFO_INVALID_ENTRY);
>  		entry->ccw |= cpu_to_be32(CCW0_INVALID);
> +		/*
> +		 * Return credit for the fault window.
> +		 */

None of the comments in this patch are useful.

> +		vas_return_credit(vinst->fault_win, 0);

Can you use true/false for bools?

>  		mutex_unlock(&vinst->mutex);
>  
>  		pr_devel("VAS[%d] fault_fifo %p, fifo %p, fault_crbs %d\n",
> @@ -267,6 +271,11 @@ irqreturn_t vas_fault_thread_fn(int irq, void *data)
>  		}
>  
>  		update_csb(window, crb);
> +		/*
> +		 * Return credit for send window after processing
> +		 * fault CRB.
> +		 */

Any chance of a little bit of explanation how the credit system works?
Or is it in the code somewhere already?

I don't suppose there is a chance to batch credit updates with multiple
faults? (maybe the MMIO is insignificant)

> +		vas_return_credit(window, 1);
>  	} while (true);
>  }
>  

Thanks,
Nick

Re: [PATCH v8 01/14] powerpc/xive: Define xive_native_alloc_irq_on_chip()

[Cédric Le Goater]

On 3/19/20 7:12 AM, Haren Myneni wrote:
> 
> This function allocates IRQ on a specific chip. VAS needs per chip
> IRQ allocation and will have IRQ handler per VAS instance.

The pool of generic interrupt source (IPI) numbers is generally used 
by user space application which generally do not care on which chip 
the interrupt is allocated. It's used by the CXL driver and KVM for 
the guest interrupts. The CPU IPI are the exceptions.

The underlying FW call will try to allocate on the chip of the CPU 
first and then on the others. If you specify a chip id, there is no 
fallback. Is it what you want ? 

Why do you need to allocate a generic interrupt source (IPI) from
a specific chip ? Is it a VAS requirement ? 

Could you explain a bit more how it is used because there might be 
similar request. 

The code is fine.

Thanks,

C.

  
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/xive.h   | 9 ++++++++-
>  arch/powerpc/sysdev/xive/native.c | 6 +++---
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
> index 93f982db..d08ea11 100644
> --- a/arch/powerpc/include/asm/xive.h
> +++ b/arch/powerpc/include/asm/xive.h
> @@ -5,6 +5,8 @@
>  #ifndef _ASM_POWERPC_XIVE_H
>  #define _ASM_POWERPC_XIVE_H
>  
> +#include <asm/opal-api.h>
> +
>  #define XIVE_INVALID_VP	0xffffffff
>  
>  #ifdef CONFIG_PPC_XIVE
> @@ -108,7 +110,6 @@ struct xive_q {
>  int xive_native_populate_irq_data(u32 hw_irq,
>  				  struct xive_irq_data *data);
>  void xive_cleanup_irq_data(struct xive_irq_data *xd);
> -u32 xive_native_alloc_irq(void);
>  void xive_native_free_irq(u32 irq);
>  int xive_native_configure_irq(u32 hw_irq, u32 target, u8 prio, u32 sw_irq);
>  
> @@ -137,6 +138,12 @@ int xive_native_set_queue_state(u32 vp_id, uint32_t prio, u32 qtoggle,
>  				u32 qindex);
>  int xive_native_get_vp_state(u32 vp_id, u64 *out_state);
>  bool xive_native_has_queue_state_support(void);
> +extern u32 xive_native_alloc_irq_on_chip(u32 chip_id);
> +
> +static inline u32 xive_native_alloc_irq(void)
> +{
> +	return xive_native_alloc_irq_on_chip(OPAL_XIVE_ANY_CHIP);
> +}
>  
>  #else
>  
> diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
> index 0ff6b73..14d4406 100644
> --- a/arch/powerpc/sysdev/xive/native.c
> +++ b/arch/powerpc/sysdev/xive/native.c
> @@ -279,12 +279,12 @@ static int xive_native_get_ipi(unsigned int cpu, struct xive_cpu *xc)
>  }
>  #endif /* CONFIG_SMP */
>  
> -u32 xive_native_alloc_irq(void)
> +u32 xive_native_alloc_irq_on_chip(u32 chip_id)
>  {
>  	s64 rc;
>  
>  	for (;;) {
> -		rc = opal_xive_allocate_irq(OPAL_XIVE_ANY_CHIP);
> +		rc = opal_xive_allocate_irq(chip_id);
>  		if (rc != OPAL_BUSY)
>  			break;
>  		msleep(OPAL_BUSY_DELAY_MS);
> @@ -293,7 +293,7 @@ u32 xive_native_alloc_irq(void)
>  		return 0;
>  	return rc;
>  }
> -EXPORT_SYMBOL_GPL(xive_native_alloc_irq);
> +EXPORT_SYMBOL_GPL(xive_native_alloc_irq_on_chip);
>  
>  void xive_native_free_irq(u32 irq)
>  {
> 

Re: [PATCH v8 02/14] powerpc/xive: Define xive_native_alloc_get_irq_info()

[Cédric Le Goater]

On 3/19/20 7:13 AM, Haren Myneni wrote:
> 
> pnv_ocxl_alloc_xive_irq() in ocxl.c allocates IRQ and gets trigger port
> address. VAS also needs this function, but based on chip ID. So moved
> this common function to xive/native.c.

We now have two drivers using the lowlevel routines of the machine 
irqchip driver. I am not sure OCXL is doing the right thing by calling
opal_xive_get_irq_info() and not xive_native_populate_irq_data().


C.

> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/xive.h       |  2 ++
>  arch/powerpc/platforms/powernv/ocxl.c | 20 ++------------------
>  arch/powerpc/sysdev/xive/native.c     | 23 +++++++++++++++++++++++
>  3 files changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
> index d08ea11..fd337da 100644
> --- a/arch/powerpc/include/asm/xive.h
> +++ b/arch/powerpc/include/asm/xive.h
> @@ -139,6 +139,8 @@ int xive_native_set_queue_state(u32 vp_id, uint32_t prio, u32 qtoggle,
>  int xive_native_get_vp_state(u32 vp_id, u64 *out_state);
>  bool xive_native_has_queue_state_support(void);
>  extern u32 xive_native_alloc_irq_on_chip(u32 chip_id);
> +extern int xive_native_alloc_get_irq_info(u32 chip_id, u32 *irq,
> +					u64 *trigger_addr);
>  
>  static inline u32 xive_native_alloc_irq(void)
>  {
> diff --git a/arch/powerpc/platforms/powernv/ocxl.c b/arch/powerpc/platforms/powernv/ocxl.c
> index 8c65aac..fb8f99a 100644
> --- a/arch/powerpc/platforms/powernv/ocxl.c
> +++ b/arch/powerpc/platforms/powernv/ocxl.c
> @@ -487,24 +487,8 @@ int pnv_ocxl_spa_remove_pe_from_cache(void *platform_data, int pe_handle)
>  
>  int pnv_ocxl_alloc_xive_irq(u32 *irq, u64 *trigger_addr)
>  {
> -	__be64 flags, trigger_page;
> -	s64 rc;
> -	u32 hwirq;
> -
> -	hwirq = xive_native_alloc_irq();
> -	if (!hwirq)
> -		return -ENOENT;
> -
> -	rc = opal_xive_get_irq_info(hwirq, &flags, NULL, &trigger_page, NULL,
> -				NULL);
> -	if (rc || !trigger_page) {
> -		xive_native_free_irq(hwirq);
> -		return -ENOENT;
> -	}
> -	*irq = hwirq;
> -	*trigger_addr = be64_to_cpu(trigger_page);
> -	return 0;
> -
> +	return xive_native_alloc_get_irq_info(OPAL_XIVE_ANY_CHIP, irq,
> +						trigger_addr);
>  }
>  EXPORT_SYMBOL_GPL(pnv_ocxl_alloc_xive_irq);
>  
> diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
> index 14d4406..abdd892 100644
> --- a/arch/powerpc/sysdev/xive/native.c
> +++ b/arch/powerpc/sysdev/xive/native.c
> @@ -295,6 +295,29 @@ u32 xive_native_alloc_irq_on_chip(u32 chip_id)
>  }
>  EXPORT_SYMBOL_GPL(xive_native_alloc_irq_on_chip);
>  
> +int xive_native_alloc_get_irq_info(u32 chip_id, u32 *irq, u64 *trigger_addr)
> +{
> +	__be64 flags, trigger_page;
> +	u32 hwirq;
> +	s64 rc;
> +
> +	hwirq = xive_native_alloc_irq_on_chip(chip_id);
> +	if (!hwirq)
> +		return -ENOENT;
> +
> +	rc = opal_xive_get_irq_info(hwirq, &flags, NULL, &trigger_page, NULL,
> +				NULL);
> +	if (rc || !trigger_page) {
> +		xive_native_free_irq(hwirq);
> +		return -ENOENT;
> +	}
> +	*irq = hwirq;
> +	*trigger_addr = be64_to_cpu(trigger_page);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(xive_native_alloc_get_irq_info);
> +
>  void xive_native_free_irq(u32 irq)
>  {
>  	for (;;) {
> 

Re: [PATCH v8 00/14] powerpc/vas: Page fault handling for user space NX requests

[Cédric Le Goater]

On 3/19/20 7:08 AM, Haren Myneni wrote:
> 
> On power9, Virtual Accelerator Switchboard (VAS) allows user space or
> kernel to communicate with Nest Accelerator (NX) directly using COPY/PASTE
> instructions. NX provides various functionalities such as compression,
> encryption and etc. But only compression (842 and GZIP formats) is
> supported in Linux kernel on power9.
> 
> 842 compression driver (drivers/crypto/nx/nx-842-powernv.c)
> is already included in Linux. Only GZIP support will be available from
> user space.
> 
> Applications can issue GZIP compression / decompression requests to NX with
> COPY/PASTE instructions. When NX is processing these requests, can hit
> fault on the request buffer (not in memory). It issues an interrupt and
> pastes fault CRB in fault FIFO. Expects kernel to handle this fault and
> return credits for both send and fault windows after processing.
> 
> This patch series adds IRQ and fault window setup, and NX fault handling:
> - Alloc IRQ and trigger port address, and configure IRQ per VAS instance.

Is the model similar to OCXL ? 

If so, I suppose that the IRQ for fault handling is allocated by skiboot,
exposed in the DT and automatically mapped in Linux when the driver is
loaded. 

Are there other interrupts ? Such as for job completion ?

Thanks,

C. 

> - Set port# for each window to generate an interrupt when noticed fault.
> - Set fault window and FIFO on which NX paste fault CRB.
> - Setup IRQ thread fault handler per VAS instance.
> - When receiving an interrupt, Read CRBs from fault FIFO and update
>   coprocessor_status_block (CSB) in the corresponding CRB with translation
>   failure (CSB_CC_TRANSLATION). After issuing NX requests, process polls
>   on CSB address. When it sees translation error, can touch the request
>   buffer to bring the page in to memory and reissue NX request.
> - If copy_to_user fails on user space CSB address, OS sends SEGV signal.
> 
> Tested these patches with NX-GZIP support and will be posting this series
> soon.
> 
> Patches 1 & 2: Define alloc IRQ and get port address per chip which are needed
>                to alloc IRQ per VAS instance.
> Patch 3: Define nx_fault_stamp on which NX writes fault status for the fault
>          CRB
> Patch 4: Alloc and setup IRQ and trigger port address for each VAS instance
> Patch 5: Setup fault window per each VAS instance. This window is used for
>          NX to paste fault CRB in FIFO.
> Patches 6 & 7: Setup threaded IRQ per VAS and register NX with fault window
>          ID and port number for each send window so that NX paste fault CRB
>          in this window.
> Patch 8: Reference to pid and mm so that pid is not used until window closed.
>          Needed for multi thread application where child can open a window
>          and can be used by parent later.
> Patches 9 and 10: Process CRBs from fault FIFO and notify tasks by
>          updating CSB or through signals.
> Patches 11 and 12: Return credits for send and fault windows after handling
>         faults.
> Patch 14:Fix closing send window after all credits are returned. This issue
>          happens only for user space requests. No page faults on kernel
>          request buffer.
> 
> Changelog:
> V2:
>   - Use threaded IRQ instead of own kernel thread handler
>   - Use pswid instead of user space CSB address to find valid CRB
>   - Removed unused macros and other changes as suggested by Christoph Hellwig
> 
> V3:
>   - Rebased to 5.5-rc2
>   - Use struct pid * instead of pid_t for vas_window tgid
>   - Code cleanup as suggested by Christoph Hellwig
> 
> V4:
>   - Define xive alloc and get IRQ info based on chip ID and use these
>     functions for IRQ setup per VAS instance. It eliminates skiboot
>     dependency as suggested by Oliver.
> 
> V5:
>   - Do not update CSB if the process is exiting (patch9)
> 
> V6:
>   - Add interrupt handler instead of default one and return IRQ_HANDLED
>     if the fault handling thread is already in progress. (Patch6)
>   - Use platform send window ID and CCW[0] bit to find valid CRB in
>     fault FIFO (Patch6).
>   - Return fault address to user space in BE and other changes as
>     suggested by Michael Neuling. (patch9)
>   - Rebased to 5.6-rc4
> 
> V7:
>   - Fix sparse warnings (patches 6,9 and 10)
> 
> V8:
>   - Move mm_context_remove_copro() before mmdrop() (patch8)
>   - Move barrier before csb.flags store and add WARN_ON_ONCE() checks (patch9)
> 
> Haren Myneni (14):
>   powerpc/xive: Define xive_native_alloc_irq_on_chip()
>   powerpc/xive: Define xive_native_alloc_get_irq_info()
>   powerpc/vas: Define nx_fault_stamp in coprocessor_request_block
>   powerpc/vas: Alloc and setup IRQ and trigger port address
>   powerpc/vas: Setup fault window per VAS instance
>   powerpc/vas: Setup thread IRQ handler per VAS instance
>   powerpc/vas: Register NX with fault window ID and IRQ port value
>   powerpc/vas: Take reference to PID and mm for user space windows
>   powerpc/vas: Update CSB and notify process for fault CRBs
>   powerpc/vas: Print CRB and FIFO values
>   powerpc/vas: Do not use default credits for receive window
>   powerpc/vas: Return credits after handling fault
>   powerpc/vas: Display process stuck message
>   powerpc/vas: Free send window in VAS instance after credits returned
> 
>  arch/powerpc/include/asm/icswx.h            |  18 +-
>  arch/powerpc/include/asm/xive.h             |  11 +-
>  arch/powerpc/platforms/powernv/Makefile     |   2 +-
>  arch/powerpc/platforms/powernv/ocxl.c       |  20 +-
>  arch/powerpc/platforms/powernv/vas-debug.c  |   2 +-
>  arch/powerpc/platforms/powernv/vas-fault.c  | 332 ++++++++++++++++++++++++++++
>  arch/powerpc/platforms/powernv/vas-window.c | 185 ++++++++++++++--
>  arch/powerpc/platforms/powernv/vas.c        | 101 ++++++++-
>  arch/powerpc/platforms/powernv/vas.h        |  51 ++++-
>  arch/powerpc/sysdev/xive/native.c           |  29 ++-
>  10 files changed, 704 insertions(+), 47 deletions(-)
>  create mode 100644 arch/powerpc/platforms/powernv/vas-fault.c
> 

Re: [PATCH v8 04/14] powerpc/vas: Alloc and setup IRQ and trigger port address

[Cédric Le Goater]

On 3/19/20 7:14 AM, Haren Myneni wrote:
> 
> Alloc IRQ and get trigger port address for each VAS instance. Kernel
> register this IRQ per VAS instance and sets this port for each send
> window. NX interrupts the kernel when it sees page fault.

I don't understand why this is not done by the OPAL driver for each VAS 
of the system. Is the VAS unit very different from OpenCAPI regarding
the fault ? 


C.

> 
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
>  arch/powerpc/platforms/powernv/vas.c | 34 ++++++++++++++++++++++++++++------
>  arch/powerpc/platforms/powernv/vas.h |  2 ++
>  2 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/vas.c b/arch/powerpc/platforms/powernv/vas.c
> index ed9cc6d..168ab68 100644
> --- a/arch/powerpc/platforms/powernv/vas.c
> +++ b/arch/powerpc/platforms/powernv/vas.c
> @@ -15,6 +15,7 @@
>  #include <linux/of_address.h>
>  #include <linux/of.h>
>  #include <asm/prom.h>
> +#include <asm/xive.h>
>  
>  #include "vas.h"
>  
> @@ -25,10 +26,12 @@
>  
>  static int init_vas_instance(struct platform_device *pdev)
>  {
> -	int rc, cpu, vasid;
> -	struct resource *res;
> -	struct vas_instance *vinst;
>  	struct device_node *dn = pdev->dev.of_node;
> +	struct vas_instance *vinst;
> +	uint32_t chipid, irq;
> +	struct resource *res;
> +	int rc, cpu, vasid;
> +	uint64_t port;
>  
>  	rc = of_property_read_u32(dn, "ibm,vas-id", &vasid);
>  	if (rc) {
> @@ -36,6 +39,12 @@ static int init_vas_instance(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> +	rc = of_property_read_u32(dn, "ibm,chip-id", &chipid);
> +	if (rc) {
> +		pr_err("No ibm,chip-id property for %s?\n", pdev->name);
> +		return -ENODEV;
> +	}
> +
>  	if (pdev->num_resources != 4) {
>  		pr_err("Unexpected DT configuration for [%s, %d]\n",
>  				pdev->name, vasid);
> @@ -69,9 +78,22 @@ static int init_vas_instance(struct platform_device *pdev)
>  
>  	vinst->paste_win_id_shift = 63 - res->end;
>  
> -	pr_devel("Initialized instance [%s, %d], paste_base 0x%llx, "
> -			"paste_win_id_shift 0x%llx\n", pdev->name, vasid,
> -			vinst->paste_base_addr, vinst->paste_win_id_shift);
> +	rc = xive_native_alloc_get_irq_info(chipid, &irq, &port);
> +	if (rc)
> +		return rc;
> +
> +	vinst->virq = irq_create_mapping(NULL, irq);
> +	if (!vinst->virq) {
> +		pr_err("Inst%d: Unable to map global irq %d\n",
> +				vinst->vas_id, irq);
> +		return -EINVAL;
> +	}
> +
> +	vinst->irq_port = port;
> +	pr_devel("Initialized instance [%s, %d] paste_base 0x%llx paste_win_id_shift 0x%llx IRQ %d Port 0x%llx\n",
> +			pdev->name, vasid, vinst->paste_base_addr,
> +			vinst->paste_win_id_shift, vinst->virq,
> +			vinst->irq_port);
>  
>  	for_each_possible_cpu(cpu) {
>  		if (cpu_to_chip_id(cpu) == of_get_ibm_chip_id(dn))
> diff --git a/arch/powerpc/platforms/powernv/vas.h b/arch/powerpc/platforms/powernv/vas.h
> index 5574aec..598608b 100644
> --- a/arch/powerpc/platforms/powernv/vas.h
> +++ b/arch/powerpc/platforms/powernv/vas.h
> @@ -313,6 +313,8 @@ struct vas_instance {
>  	u64 paste_base_addr;
>  	u64 paste_win_id_shift;
>  
> +	u64 irq_port;
> +	int virq;
>  	struct mutex mutex;
>  	struct vas_window *rxwin[VAS_COP_TYPE_MAX];
>  	struct vas_window *windows[VAS_WINDOWS_PER_CHIP];
> 

Re: [PATCH v8 04/14] powerpc/vas: Alloc and setup IRQ and trigger port address

[Cédric Le Goater]

On 3/23/20 10:06 AM, Cédric Le Goater wrote:
> On 3/19/20 7:14 AM, Haren Myneni wrote:
>>
>> Alloc IRQ and get trigger port address for each VAS instance. Kernel
>> register this IRQ per VAS instance and sets this port for each send
>> window. NX interrupts the kernel when it sees page fault.
> 
> I don't understand why this is not done by the OPAL driver for each VAS 
> of the system. Is the VAS unit very different from OpenCAPI regarding
> the fault ? 

I checked the previous patchsets and I see that v3 was more like I expected
it: one interrupt for faults allocated by the skiboot driver and exposed  
in the DT.

What made you change your mind ? 

This version is hijacking the lowlevel routines of the XIVE irqchip which
is not the best approach. OCXL is doing that because it needs to allocate
interrupts for the user space processes using the AFU and we should rework 
that part. 

However, the translation fault interrupt is allocated by skiboot.

Sorry for the noise, I would like to understand more how this works. I also
have passthrough in mind.

C.

Re: [PATCH v8 03/14] powerpc/vas: Define nx_fault_stamp in coprocessor_request_block

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:
> Haren Myneni's on March 19, 2020 4:13 pm:
>> 
>> Kernel sets fault address and status in CRB for NX page fault on user
>> space address after processing page fault. User space gets the signal
>> and handles the fault mentioned in CRB by bringing the page in to
>> memory and send NX request again.
>> 
>> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
>> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
>> ---
>>  arch/powerpc/include/asm/icswx.h | 18 +++++++++++++++++-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/powerpc/include/asm/icswx.h b/arch/powerpc/include/asm/icswx.h
>> index 9872f85..b233d1e 100644
>> --- a/arch/powerpc/include/asm/icswx.h
>> +++ b/arch/powerpc/include/asm/icswx.h
>
> "icswx" is not a thing anymore, after 6ff4d3e96652 ("powerpc: Remove old 
> unused icswx based coprocessor support").

Yeah that commit ripped out some parts of the previous attempt at a user
visible API for this sort of "coprocessor" stuff. VAS is yet another
attempt to do something useful with most of the same pieces but some
slightly different details.

> I guess NX is reusing some 
> things from it, but it would be good to get rid of the cruft and re-name
> this file and and relevant names.

> NX already uses this file, so I guesss that can happen after this series.

A lot of the CRB/CSB stuff is still the same, and P8 still uses icswx.
But I'd be happy if the header was renamed eventually, as icswx is now a
legacy name.

cheers

Re: [PATCH v8 03/14] powerpc/vas: Define nx_fault_stamp in coprocessor_request_block

[Haren Myneni]

On Mon, 2020-03-23 at 22:32 +1100, Michael Ellerman wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
> > Haren Myneni's on March 19, 2020 4:13 pm:
> >> 
> >> Kernel sets fault address and status in CRB for NX page fault on user
> >> space address after processing page fault. User space gets the signal
> >> and handles the fault mentioned in CRB by bringing the page in to
> >> memory and send NX request again.
> >> 
> >> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> >> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> >> ---
> >>  arch/powerpc/include/asm/icswx.h | 18 +++++++++++++++++-
> >>  1 file changed, 17 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/arch/powerpc/include/asm/icswx.h b/arch/powerpc/include/asm/icswx.h
> >> index 9872f85..b233d1e 100644
> >> --- a/arch/powerpc/include/asm/icswx.h
> >> +++ b/arch/powerpc/include/asm/icswx.h
> >
> > "icswx" is not a thing anymore, after 6ff4d3e96652 ("powerpc: Remove old 
> > unused icswx based coprocessor support").
> 
> Yeah that commit ripped out some parts of the previous attempt at a user
> visible API for this sort of "coprocessor" stuff. VAS is yet another
> attempt to do something useful with most of the same pieces but some
> slightly different details.
> 
> > I guess NX is reusing some 
> > things from it, but it would be good to get rid of the cruft and re-name
> > this file and and relevant names.
> 
> > NX already uses this file, so I guesss that can happen after this series.
> 
> A lot of the CRB/CSB stuff is still the same, and P8 still uses icswx.
> But I'd be happy if the header was renamed eventually, as icswx is now a
> legacy name.

We can move all macros and struct definitions to vas.h and remove
icswx.h. Can I do this after this series? 

Thanks
Haren

> 
> cheers

Re: [PATCH v8 04/14] powerpc/vas: Alloc and setup IRQ and trigger port address

[Haren Myneni]

On Mon, 2020-03-23 at 10:27 +0100, Cédric Le Goater wrote:
> On 3/23/20 10:06 AM, Cédric Le Goater wrote:
> > On 3/19/20 7:14 AM, Haren Myneni wrote:
> >>
> >> Alloc IRQ and get trigger port address for each VAS instance. Kernel
> >> register this IRQ per VAS instance and sets this port for each send
> >> window. NX interrupts the kernel when it sees page fault.
> > 
> > I don't understand why this is not done by the OPAL driver for each VAS 
> > of the system. Is the VAS unit very different from OpenCAPI regarding
> > the fault ? 
> 
> I checked the previous patchsets and I see that v3 was more like I expected
> it: one interrupt for faults allocated by the skiboot driver and exposed  
> in the DT.
> 
> What made you change your mind ? 
> 
> This version is hijacking the lowlevel routines of the XIVE irqchip which
> is not the best approach. OCXL is doing that because it needs to allocate
> interrupts for the user space processes using the AFU and we should rework 
> that part. 
> 
> However, the translation fault interrupt is allocated by skiboot.

Sorry my mistake. I should have CC you earlier. 

Each VAS instance will generate fault interrupt which is per chip. There
won't be other job completion interrupts. 

Correct, V3 used allocating interrupts per chip in skiboot and exposed
in DT. Since XIVE code has similar feature, exploited this approach so
that we do not need skiboot changes. 

Thanks
Haren


> 
> Sorry for the noise, I would like to understand more how this works. I also
> have passthrough in mind.
> 
> C.
> 

Re: [PATCH v8 04/14] powerpc/vas: Alloc and setup IRQ and trigger port address

[Oliver O'Halloran]

On Mon, Mar 23, 2020 at 8:28 PM Cédric Le Goater <clg@kaod.org> wrote:
>
> On 3/23/20 10:06 AM, Cédric Le Goater wrote:
> > On 3/19/20 7:14 AM, Haren Myneni wrote:
> >>
> >> Alloc IRQ and get trigger port address for each VAS instance. Kernel
> >> register this IRQ per VAS instance and sets this port for each send
> >> window. NX interrupts the kernel when it sees page fault.
> >
> > I don't understand why this is not done by the OPAL driver for each VAS
> > of the system. Is the VAS unit very different from OpenCAPI regarding
> > the fault ?
>
> I checked the previous patchsets and I see that v3 was more like I expected
> it: one interrupt for faults allocated by the skiboot driver and exposed
> in the DT.
>
> What made you change your mind ?

From init_vas_inst() in arch/powerpc/platforms/powernv/vas.c:

        if (pdev->num_resources != 4) {
                pr_err("Unexpected DT configuration for [%s, %d]\n",
                                pdev->name, vasid);
                return -ENODEV;
        }

This code should never have been written, but here we are. Due to the
above adding an interrupt in the DT makes the driver unable to bind on
older kernels. In an older version of the patches (don't think it was
posted) Haren was using a non-standard interrupt property and we could
work around the problem by going back to that.

However, we already have the OPAL calls for allocating / freeing
hardware interrupt numbers so why not do that? If we ever want to take
advantage of the job completion interrupts we'd want to have the
ability to allocate them since the completion interrupts are
per-window rather than per-VAS.

> This version is hijacking the lowlevel routines of the XIVE irqchip which
> is not the best approach. OCXL is doing that because it needs to allocate
> interrupts for the user space processes using the AFU and we should rework
> that part.

What'd you have in mind for the reworking the oxcl interrupt
allocation? I didn't find it that objectionable since it's more or
less the same as what happens when allocating IPIs.

> However, the translation fault interrupt is allocated by skiboot.
>
> Sorry for the noise, I would like to understand more how this works. I also
> have passthrough in mind.
>
> C.

Re: [PATCH v8 04/14] powerpc/vas: Alloc and setup IRQ and trigger port address

[Cédric Le Goater]

On 3/23/20 8:02 PM, Haren Myneni wrote:
> On Mon, 2020-03-23 at 10:27 +0100, Cédric Le Goater wrote:
>> On 3/23/20 10:06 AM, Cédric Le Goater wrote:
>>> On 3/19/20 7:14 AM, Haren Myneni wrote:
>>>>
>>>> Alloc IRQ and get trigger port address for each VAS instance. Kernel
>>>> register this IRQ per VAS instance and sets this port for each send
>>>> window. NX interrupts the kernel when it sees page fault.
>>>
>>> I don't understand why this is not done by the OPAL driver for each VAS 
>>> of the system. Is the VAS unit very different from OpenCAPI regarding
>>> the fault ? 
>>
>> I checked the previous patchsets and I see that v3 was more like I expected
>> it: one interrupt for faults allocated by the skiboot driver and exposed  
>> in the DT.
>>
>> What made you change your mind ? 
>>
>> This version is hijacking the lowlevel routines of the XIVE irqchip which
>> is not the best approach. OCXL is doing that because it needs to allocate
>> interrupts for the user space processes using the AFU and we should rework 
>> that part. 
>>
>> However, the translation fault interrupt is allocated by skiboot.
> 
> Sorry my mistake. I should have CC you earlier. 
> 
> Each VAS instance will generate fault interrupt which is per chip. There
> won't be other job completion interrupts. 

That's a very good reason to set everything in the skiboot driver and 
advertise the interrupt number in the DT. The interrupt will be mapped
automatically by OF routines and the driver will only have to install
an interrupt handler. 

> Correct, V3 used allocating interrupts per chip in skiboot and exposed
> in DT. Since XIVE code has similar feature, exploited this approach so
> that we do not need skiboot changes. 

It's not the same. These are the low level (OPAL) interface used by the 
XIVE driver. The exception is the KVM XIVE device which needs a finer
grain. 

C.

Re: [PATCH v8 04/14] powerpc/vas: Alloc and setup IRQ and trigger port address

[Cédric Le Goater]

On 3/24/20 3:26 AM, Oliver O'Halloran wrote:
> On Mon, Mar 23, 2020 at 8:28 PM Cédric Le Goater <clg@kaod.org> wrote:
>>
>> On 3/23/20 10:06 AM, Cédric Le Goater wrote:
>>> On 3/19/20 7:14 AM, Haren Myneni wrote:
>>>>
>>>> Alloc IRQ and get trigger port address for each VAS instance. Kernel
>>>> register this IRQ per VAS instance and sets this port for each send
>>>> window. NX interrupts the kernel when it sees page fault.
>>>
>>> I don't understand why this is not done by the OPAL driver for each VAS
>>> of the system. Is the VAS unit very different from OpenCAPI regarding
>>> the fault ?
>>
>> I checked the previous patchsets and I see that v3 was more like I expected
>> it: one interrupt for faults allocated by the skiboot driver and exposed
>> in the DT.
>>
>> What made you change your mind ?
> 
> From init_vas_inst() in arch/powerpc/platforms/powernv/vas.c:
> 
>         if (pdev->num_resources != 4) {
>                 pr_err("Unexpected DT configuration for [%s, %d]\n",
>                                 pdev->name, vasid);
>                 return -ENODEV;
>         }
> 
> This code should never have been written, but here we are. Due to the
> above adding an interrupt in the DT makes the driver unable to bind on
> older kernels. In an older version of the patches (don't think it was
> posted) Haren was using a non-standard interrupt property and we could
> work around the problem by going back to that.

ok ... :/ I didn't know. Don't we have a rule on LinuxPPC for such 
things ? Such as, the culprit should send a croissant to everyone 
involved. 

> However, we already have the OPAL calls for allocating / freeing
> hardware interrupt numbers so why not do that? 

It's a good way to work around the problem but we are bypassing the
irqchip which does other things for the driver.

> If we ever want to take
> advantage of the job completion interrupts we'd want to have the
> ability to allocate them since the completion interrupts are
> per-window rather than per-VAS.

Yes. That's what I thought it was about to begin with. OCXL has a  
first implementation of such interrupts. 

>> This version is hijacking the lowlevel routines of the XIVE irqchip which
>> is not the best approach. OCXL is doing that because it needs to allocate
>> interrupts for the user space processes using the AFU and we should rework
>> that part.
> 
> What'd you have in mind for the reworking the oxcl interrupt allocation? 
> I didn't find it that objectionable since it's more or less the same as 
> what happens when allocating IPIs.

I think we need to work a bit more on the concepts, on the interfaces,
internal at the platform kernel level and at the user space level, and 
on the configuration, with chip affinity in mind. There are bunch of 
information on the sources that are retrieved from the firmware or 
hypervisor that we care about. An irqchip might be the best option 
for the moment. 

At the same time, it would be good to keep in mind user interrupts. 

C.

> 
>> However, the translation fault interrupt is allocated by skiboot.
>>
>> Sorry for the noise, I would like to understand more how this works. I also
>> have passthrough in mind.
>>
>> C.

Re: [PATCH v8 01/14] powerpc/xive: Define xive_native_alloc_irq_on_chip()

[Cédric Le Goater]

On 3/19/20 7:12 AM, Haren Myneni wrote:
> 
> This function allocates IRQ on a specific chip. VAS needs per chip
> IRQ allocation and will have IRQ handler per VAS instance.
> 
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  arch/powerpc/include/asm/xive.h   | 9 ++++++++-
>  arch/powerpc/sysdev/xive/native.c | 6 +++---
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
> index 93f982db..d08ea11 100644
> --- a/arch/powerpc/include/asm/xive.h
> +++ b/arch/powerpc/include/asm/xive.h
> @@ -5,6 +5,8 @@
>  #ifndef _ASM_POWERPC_XIVE_H
>  #define _ASM_POWERPC_XIVE_H
>  
> +#include <asm/opal-api.h>
> +
>  #define XIVE_INVALID_VP	0xffffffff
>  
>  #ifdef CONFIG_PPC_XIVE
> @@ -108,7 +110,6 @@ struct xive_q {
>  int xive_native_populate_irq_data(u32 hw_irq,
>  				  struct xive_irq_data *data);
>  void xive_cleanup_irq_data(struct xive_irq_data *xd);
> -u32 xive_native_alloc_irq(void);
>  void xive_native_free_irq(u32 irq);
>  int xive_native_configure_irq(u32 hw_irq, u32 target, u8 prio, u32 sw_irq);
>  
> @@ -137,6 +138,12 @@ int xive_native_set_queue_state(u32 vp_id, uint32_t prio, u32 qtoggle,
>  				u32 qindex);
>  int xive_native_get_vp_state(u32 vp_id, u64 *out_state);
>  bool xive_native_has_queue_state_support(void);
> +extern u32 xive_native_alloc_irq_on_chip(u32 chip_id);
> +
> +static inline u32 xive_native_alloc_irq(void)
> +{
> +	return xive_native_alloc_irq_on_chip(OPAL_XIVE_ANY_CHIP);
> +}
>  
>  #else
>  
> diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
> index 0ff6b73..14d4406 100644
> --- a/arch/powerpc/sysdev/xive/native.c
> +++ b/arch/powerpc/sysdev/xive/native.c
> @@ -279,12 +279,12 @@ static int xive_native_get_ipi(unsigned int cpu, struct xive_cpu *xc)
>  }
>  #endif /* CONFIG_SMP */
>  
> -u32 xive_native_alloc_irq(void)
> +u32 xive_native_alloc_irq_on_chip(u32 chip_id)
>  {
>  	s64 rc;
>  
>  	for (;;) {
> -		rc = opal_xive_allocate_irq(OPAL_XIVE_ANY_CHIP);
> +		rc = opal_xive_allocate_irq(chip_id);
>  		if (rc != OPAL_BUSY)
>  			break;
>  		msleep(OPAL_BUSY_DELAY_MS);
> @@ -293,7 +293,7 @@ u32 xive_native_alloc_irq(void)
>  		return 0;
>  	return rc;
>  }
> -EXPORT_SYMBOL_GPL(xive_native_alloc_irq);
> +EXPORT_SYMBOL_GPL(xive_native_alloc_irq_on_chip);
>  
>  void xive_native_free_irq(u32 irq)
>  {
> 

Re: [PATCH v8 02/14] powerpc/xive: Define xive_native_alloc_get_irq_info()

[Cédric Le Goater]

On 3/19/20 7:13 AM, Haren Myneni wrote:
> 
> pnv_ocxl_alloc_xive_irq() in ocxl.c allocates IRQ and gets trigger port
> address. VAS also needs this function, but based on chip ID. So moved
> this common function to xive/native.c.
> 
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>

I think we should work on a new interface for generic IPI use. 
This is a beginning.  

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  arch/powerpc/include/asm/xive.h       |  2 ++
>  arch/powerpc/platforms/powernv/ocxl.c | 20 ++------------------
>  arch/powerpc/sysdev/xive/native.c     | 23 +++++++++++++++++++++++
>  3 files changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
> index d08ea11..fd337da 100644
> --- a/arch/powerpc/include/asm/xive.h
> +++ b/arch/powerpc/include/asm/xive.h
> @@ -139,6 +139,8 @@ int xive_native_set_queue_state(u32 vp_id, uint32_t prio, u32 qtoggle,
>  int xive_native_get_vp_state(u32 vp_id, u64 *out_state);
>  bool xive_native_has_queue_state_support(void);
>  extern u32 xive_native_alloc_irq_on_chip(u32 chip_id);
> +extern int xive_native_alloc_get_irq_info(u32 chip_id, u32 *irq,
> +					u64 *trigger_addr);
>  
>  static inline u32 xive_native_alloc_irq(void)
>  {
> diff --git a/arch/powerpc/platforms/powernv/ocxl.c b/arch/powerpc/platforms/powernv/ocxl.c
> index 8c65aac..fb8f99a 100644
> --- a/arch/powerpc/platforms/powernv/ocxl.c
> +++ b/arch/powerpc/platforms/powernv/ocxl.c
> @@ -487,24 +487,8 @@ int pnv_ocxl_spa_remove_pe_from_cache(void *platform_data, int pe_handle)
>  
>  int pnv_ocxl_alloc_xive_irq(u32 *irq, u64 *trigger_addr)
>  {
> -	__be64 flags, trigger_page;
> -	s64 rc;
> -	u32 hwirq;
> -
> -	hwirq = xive_native_alloc_irq();
> -	if (!hwirq)
> -		return -ENOENT;
> -
> -	rc = opal_xive_get_irq_info(hwirq, &flags, NULL, &trigger_page, NULL,
> -				NULL);
> -	if (rc || !trigger_page) {
> -		xive_native_free_irq(hwirq);
> -		return -ENOENT;
> -	}
> -	*irq = hwirq;
> -	*trigger_addr = be64_to_cpu(trigger_page);
> -	return 0;
> -
> +	return xive_native_alloc_get_irq_info(OPAL_XIVE_ANY_CHIP, irq,
> +						trigger_addr);

alignment ^

>  }
>  EXPORT_SYMBOL_GPL(pnv_ocxl_alloc_xive_irq);
>  
> diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
> index 14d4406..abdd892 100644
> --- a/arch/powerpc/sysdev/xive/native.c
> +++ b/arch/powerpc/sysdev/xive/native.c
> @@ -295,6 +295,29 @@ u32 xive_native_alloc_irq_on_chip(u32 chip_id)
>  }
>  EXPORT_SYMBOL_GPL(xive_native_alloc_irq_on_chip);
>  
> +int xive_native_alloc_get_irq_info(u32 chip_id, u32 *irq, u64 *trigger_addr)
> +{
> +	__be64 flags, trigger_page;
> +	u32 hwirq;
> +	s64 rc;
> +
> +	hwirq = xive_native_alloc_irq_on_chip(chip_id);
> +	if (!hwirq)
> +		return -ENOENT;
> +
> +	rc = opal_xive_get_irq_info(hwirq, &flags, NULL, &trigger_page, NULL,
> +				NULL);
> +	if (rc || !trigger_page) {
> +		xive_native_free_irq(hwirq);
> +		return -ENOENT;
> +	}
> +	*irq = hwirq;
> +	*trigger_addr = be64_to_cpu(trigger_page);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(xive_native_alloc_get_irq_info);
> +
>  void xive_native_free_irq(u32 irq)
>  {
>  	for (;;) {
> 

Re: [PATCH v8 04/14] powerpc/vas: Alloc and setup IRQ and trigger port address

[Cédric Le Goater]

On 3/19/20 7:14 AM, Haren Myneni wrote:
> 
> Alloc IRQ and get trigger port address for each VAS instance. Kernel
> register this IRQ per VAS instance and sets this port for each send
> window. NX interrupts the kernel when it sees page fault.
> 
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
>  arch/powerpc/platforms/powernv/vas.c | 34 ++++++++++++++++++++++++++++------
>  arch/powerpc/platforms/powernv/vas.h |  2 ++
>  2 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/vas.c b/arch/powerpc/platforms/powernv/vas.c
> index ed9cc6d..168ab68 100644
> --- a/arch/powerpc/platforms/powernv/vas.c
> +++ b/arch/powerpc/platforms/powernv/vas.c
> @@ -15,6 +15,7 @@
>  #include <linux/of_address.h>
>  #include <linux/of.h>
>  #include <asm/prom.h>
> +#include <asm/xive.h>
>  
>  #include "vas.h"
>  
> @@ -25,10 +26,12 @@
>  
>  static int init_vas_instance(struct platform_device *pdev)
>  {
> -	int rc, cpu, vasid;
> -	struct resource *res;
> -	struct vas_instance *vinst;
>  	struct device_node *dn = pdev->dev.of_node;
> +	struct vas_instance *vinst;
> +	uint32_t chipid, irq;
> +	struct resource *res;
> +	int rc, cpu, vasid;
> +	uint64_t port;
>  
>  	rc = of_property_read_u32(dn, "ibm,vas-id", &vasid);
>  	if (rc) {
> @@ -36,6 +39,12 @@ static int init_vas_instance(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> +	rc = of_property_read_u32(dn, "ibm,chip-id", &chipid);
> +	if (rc) {
> +		pr_err("No ibm,chip-id property for %s?\n", pdev->name);
> +		return -ENODEV;
> +	}
> +
>  	if (pdev->num_resources != 4) {
>  		pr_err("Unexpected DT configuration for [%s, %d]\n",
>  				pdev->name, vasid);
> @@ -69,9 +78,22 @@ static int init_vas_instance(struct platform_device *pdev)
>  
>  	vinst->paste_win_id_shift = 63 - res->end;
>  
> -	pr_devel("Initialized instance [%s, %d], paste_base 0x%llx, "
> -			"paste_win_id_shift 0x%llx\n", pdev->name, vasid,
> -			vinst->paste_base_addr, vinst->paste_win_id_shift);
> +	rc = xive_native_alloc_get_irq_info(chipid, &irq, &port);
> +	if (rc)
> +		return rc;
> +
> +	vinst->virq = irq_create_mapping(NULL, irq);
> +	if (!vinst->virq) {
> +		pr_err("Inst%d: Unable to map global irq %d\n",
> +				vinst->vas_id, irq);
> +		return -EINVAL;
> +	}
> +
> +	vinst->irq_port = port;


I would prefer something like this : 

	hwirq = xive_native_alloc_irq_on_chip(chip_id);

	vinst->virq = irq_create_mapping(NULL, hwirq);
 	if (!vinst->virq) {
		...
	}

	{
		struct irq_data *d = irq_get_irq_data(vinst->virq);
		struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);

		vinst->irq_port = xd->trig_page;
	}


and dump xive_native_alloc_get_irq_info().

C.


> +	pr_devel("Initialized instance [%s, %d] paste_base 0x%llx paste_win_id_shift 0x%llx IRQ %d Port 0x%llx\n",
> +			pdev->name, vasid, vinst->paste_base_addr,
> +			vinst->paste_win_id_shift, vinst->virq,
> +			vinst->irq_port);
>  
>  	for_each_possible_cpu(cpu) {
>  		if (cpu_to_chip_id(cpu) == of_get_ibm_chip_id(dn))
> diff --git a/arch/powerpc/platforms/powernv/vas.h b/arch/powerpc/platforms/powernv/vas.h
> index 5574aec..598608b 100644
> --- a/arch/powerpc/platforms/powernv/vas.h
> +++ b/arch/powerpc/platforms/powernv/vas.h
> @@ -313,6 +313,8 @@ struct vas_instance {
>  	u64 paste_base_addr;
>  	u64 paste_win_id_shift;
>  
> +	u64 irq_port;
> +	int virq;
>  	struct mutex mutex;
>  	struct vas_window *rxwin[VAS_COP_TYPE_MAX];
>  	struct vas_window *windows[VAS_WINDOWS_PER_CHIP];
> 

Re: [PATCH v8 04/14] powerpc/vas: Alloc and setup IRQ and trigger port address

[Haren Myneni]

On Tue, 2020-03-24 at 15:48 +0100, Cédric Le Goater wrote:
> On 3/19/20 7:14 AM, Haren Myneni wrote:
> > 
> > Alloc IRQ and get trigger port address for each VAS instance. Kernel
> > register this IRQ per VAS instance and sets this port for each send
> > window. NX interrupts the kernel when it sees page fault.
> > 
> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> > ---
> >  arch/powerpc/platforms/powernv/vas.c | 34 ++++++++++++++++++++++++++++------
> >  arch/powerpc/platforms/powernv/vas.h |  2 ++
> >  2 files changed, 30 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/powerpc/platforms/powernv/vas.c b/arch/powerpc/platforms/powernv/vas.c
> > index ed9cc6d..168ab68 100644
> > --- a/arch/powerpc/platforms/powernv/vas.c
> > +++ b/arch/powerpc/platforms/powernv/vas.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/of_address.h>
> >  #include <linux/of.h>
> >  #include <asm/prom.h>
> > +#include <asm/xive.h>
> >  
> >  #include "vas.h"
> >  
> > @@ -25,10 +26,12 @@
> >  
> >  static int init_vas_instance(struct platform_device *pdev)
> >  {
> > -	int rc, cpu, vasid;
> > -	struct resource *res;
> > -	struct vas_instance *vinst;
> >  	struct device_node *dn = pdev->dev.of_node;
> > +	struct vas_instance *vinst;
> > +	uint32_t chipid, irq;
> > +	struct resource *res;
> > +	int rc, cpu, vasid;
> > +	uint64_t port;
> >  
> >  	rc = of_property_read_u32(dn, "ibm,vas-id", &vasid);
> >  	if (rc) {
> > @@ -36,6 +39,12 @@ static int init_vas_instance(struct platform_device *pdev)
> >  		return -ENODEV;
> >  	}
> >  
> > +	rc = of_property_read_u32(dn, "ibm,chip-id", &chipid);
> > +	if (rc) {
> > +		pr_err("No ibm,chip-id property for %s?\n", pdev->name);
> > +		return -ENODEV;
> > +	}
> > +
> >  	if (pdev->num_resources != 4) {
> >  		pr_err("Unexpected DT configuration for [%s, %d]\n",
> >  				pdev->name, vasid);
> > @@ -69,9 +78,22 @@ static int init_vas_instance(struct platform_device *pdev)
> >  
> >  	vinst->paste_win_id_shift = 63 - res->end;
> >  
> > -	pr_devel("Initialized instance [%s, %d], paste_base 0x%llx, "
> > -			"paste_win_id_shift 0x%llx\n", pdev->name, vasid,
> > -			vinst->paste_base_addr, vinst->paste_win_id_shift);
> > +	rc = xive_native_alloc_get_irq_info(chipid, &irq, &port);
> > +	if (rc)
> > +		return rc;
> > +
> > +	vinst->virq = irq_create_mapping(NULL, irq);
> > +	if (!vinst->virq) {
> > +		pr_err("Inst%d: Unable to map global irq %d\n",
> > +				vinst->vas_id, irq);
> > +		return -EINVAL;
> > +	}
> > +
> > +	vinst->irq_port = port;
> 
> 
> I would prefer something like this : 
> 
> 	hwirq = xive_native_alloc_irq_on_chip(chip_id);
> 
> 	vinst->virq = irq_create_mapping(NULL, hwirq);
>  	if (!vinst->virq) {
> 		...
> 	}
> 
> 	{
> 		struct irq_data *d = irq_get_irq_data(vinst->virq);
> 		struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
> 
> 		vinst->irq_port = xd->trig_page;
> 	}
> 
> 
> and dump xive_native_alloc_get_irq_info().

Thanks Cedric, I will remove patch which adds this function. 

> 
> C.
> 

Re: [PATCH v8 06/14] powerpc/vas: Setup thread IRQ handler per VAS instance

[Haren Myneni]

On Mon, 2020-03-23 at 12:23 +1000, Nicholas Piggin wrote:
> Haren Myneni's on March 19, 2020 4:15 pm:
> > 
> > Setup thread IRQ handler per each VAS instance. When NX sees a fault
> > on CRB, kernel gets an interrupt and vas_fault_handler will be
> > executed to process fault CRBs. Read all valid CRBs from fault FIFO,
> > determine the corresponding send window from CRB and process fault
> > requests.
> 
> Perhaps some more overview/why.
> 
> "If NX encounters a translation error when accessing the CRB or one
> of addresses in the request, it raises an interrupt on the CPU to
> handle the fault.
> 
> 
> > 
> > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> > ---
> >  arch/powerpc/platforms/powernv/vas-fault.c  | 90 +++++++++++++++++++++++++++++
> >  arch/powerpc/platforms/powernv/vas-window.c | 60 +++++++++++++++++++
> >  arch/powerpc/platforms/powernv/vas.c        | 49 +++++++++++++++-
> >  arch/powerpc/platforms/powernv/vas.h        |  6 ++
> >  4 files changed, 204 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c
> > index 4044998..1c6d5cc 100644
> > --- a/arch/powerpc/platforms/powernv/vas-fault.c
> > +++ b/arch/powerpc/platforms/powernv/vas-fault.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/uaccess.h>
> >  #include <linux/kthread.h>
> > +#include <linux/mmu_context.h>
> >  #include <asm/icswx.h>
> >  
> >  #include "vas.h"
> > @@ -25,6 +26,95 @@
> >  #define VAS_FAULT_WIN_FIFO_SIZE	(4 << 20)
> >  
> >  /*
> > + * Process valid CRBs in fault FIFO.
> > + */
> > +irqreturn_t vas_fault_thread_fn(int irq, void *data)
> 
> Are page faults the only reason why VAS would raise this interrupt? Is 
> NX really the only possible user of this, so you can have NX specifics
> in here?

Yes, When NX sees page faults, it generates interrupts on specific VAS
instance. Right now NX is the only user. So trying to make as
generalized as possible. 

> 
> > +{
> > +	struct vas_instance *vinst = data;
> > +	struct coprocessor_request_block *crb, *entry;
> > +	struct coprocessor_request_block buf;
> > +	struct vas_window *window;
> > +	unsigned long flags;
> > +	void *fifo;
> > +
> > +	crb = &buf;
> 
> The below comment could just be moved to replace the one at the top of the
> function. Can you explain slightly more about how the faults work, and 
> be more clear about what the coprocessor does versus what the host does? The
> use of VAS and NX is a bit confusing too. VAS doesn't interrupt with
> page faults, does it? NX has the page fault(s), and it requests VAS to
> interrupt the host?
NX is the one who raises interrupt on specific VAS port that is
registered with fault window. I will make it clear in the comment.  
> 
> > +
> > +	/*
> > +	 * VAS can interrupt with multiple page faults. So process all
> > +	 * valid CRBs within fault FIFO until reaches invalid CRB.
> 
>  When NX encounters a fault accessing a memory address for a particular
>  CRB, it updates the nx_fault_stamp field in the CRB (to what?), and 
>  copies the CRB to the fault FIFO memory, then raises an interrupt on the
>  CPU (memory ordering on the store and load sides are provided how?). NX
>  can store multiple faults into the FIFO per interrupt (does it proceed
>  asynchronously after the interrupt? what's the stopping condition?).

User space fills CRB and sends request (CRB). NX processes the request
and update CSB (in CRB struct). If NX sees any page fault either on
request buffers or csb address, updates nx_fault_stamp struct (in user
space CRB) and pastes CRB in fault_fifo. Then raises interrupt on port
defined in fault_window (which is per VAS instance). 

NX can raise single interrupt for multiple faults and can paste in fault
FIFO. But OS and NX use credits to control fault FIFO. 

Initially FIFO_SIZE/CRB_SIZE credits are available for fault window. For
example, When NX pastes CRB in fault fifo, credits will be reduced by 1.
It can continue paste CRBs in FIFO until credits reached to 0. 
When OS handles the fault CRB, increments index in FIFO and returns the
credit so that NX knows one more CRB slot is available. 

struct coprocessor_request_block {
        __be32 ccw;
        __be32 flags;
        __be64 csb_addr;

        struct data_descriptor_entry source;
        struct data_descriptor_entry target;

        struct coprocessor_completion_block ccb;

        union {
                struct nx_fault_stamp nx;
                u8 reserved[16];
        } stamp;

        u8 reserved[32];

        struct coprocessor_status_block csb;
} __packed __aligned(CRB_ALIGN);
 


> 
>  When the CPU takes this interrupt, it reads the faulting CRBs from the
>  FIFO and processes them in order until it reaches an invalid entry, FIFO
>  empty (memory ordering how?). After each FIFO entry is processed, store
>  to mark them as invalid. (How does NX resume after this?)

NX should do atomic copy of CRB in fault FIFO. But we had barrier (as
part of spin_unlock()) for the safe side which is suggested by HW team.

NX stops pasting CRBs if credits are not available and start when credit
is returned by OS after handling fault. 

> 
> How is the fault actually even "handled" here? Nothing seems to be
> actually done for them.
> 
> > +	 * NX updates nx_fault_stamp in CRB and pastes in fault FIFO.
> > +	 * kernel retrives send window from parition send window ID
> > +	 * (pswid) in nx_fault_stamp. So pswid should be valid and
> > +	 * ccw[0] (in be) should be zero since this bit is reserved.
> > +	 * If user space touches this bit, NX returns with "CRB format
> > +	 * error".
> > +	 *
> > +	 * After reading CRB entry, invalidate it with pswid (set
> > +	 * 0xffffffff) and ccw[0] (set to 1).
> 
> Al this is very busy and hard to decipher unambiguously. It should read
> more like a spec, a precise sequence of things happening.
Sure, will make it clear
> 
> > +	 *
> > +	 * In case kernel receives another interrupt with different page
> > +	 * fault, CRBs are already processed by the previous handling. So
> > +	 * will be returned from this function when it sees invalid CRB.
> > +	 */
> 
> Ambiguous at best. Assuming the NX continues running asynchronously and
> it's a usual kind of FIFO, I assume this means if the kernel gets 
> another interrupt for a page fault corresponding to a FIFO entry that
> has already been processed by this fault.
> 
> 
> > +	do {
> 
> Can you make this 'while (true)' or 'for (;;)' so you don't need to go
> to the bottom to see it's an infinite loop.
> 
> > +		mutex_lock(&vinst->mutex);
> 
> What does this protect? Threaded handlers don't run concurrently for the
> same request_threaded_irq?

We can remove this mutex_lock. 
> 
> > +
> > +		spin_lock_irqsave(&vinst->fault_lock, flags);
> > +		/*
> > +		 * Advance the fault fifo pointer to next CRB.
> 
> The code below the comment isn't advancing the fault fifo pointer, it's
> grabbing the current one. The pointer (fault_crbs) is advanced later. 
> You presumabl don't want to advance over an invalid entry.

Advancing to next entry which is invalid means start processing from
this entry in the next fault. 

> 
> > +		 * Use CRB_SIZE rather than sizeof(*crb) since the latter is
> > +		 * aligned to CRB_ALIGN (256) but the CRB written to by VAS is
> > +		 * only CRB_SIZE in len.
> > +		 */
> > +		fifo = vinst->fault_fifo + (vinst->fault_crbs * CRB_SIZE);
> > +		entry = fifo;
> 
> Don't think you should really do this. It may be harmless in this case,
> but the compiler expects the type to be aligned. Make it another type,
> like coprocessor_fault_block or something?

Can add like "entry = (struct coprocessor_request_block *)fifo;

> 
> > +
> > +		if ((entry->stamp.nx.pswid == cpu_to_be32(FIFO_INVALID_ENTRY))
> > +			|| (entry->ccw & cpu_to_be32(CCW0_INVALID))) {
> > +			atomic_set(&vinst->faults_in_progress, 0);
> > +			spin_unlock_irqrestore(&vinst->fault_lock, flags);
> 
> So what does the fault_lock protect? The only data it protects is 
> faults_in_progress (vs the hard interrupt handler), which doesn't 
> achieve anything by itself, so I guess it also prevents the hard irq
> handler from returning until the handler here has checked that the
> fault FIFO is empty then returns IRQ_HANDLED? That seems fine (so long
> as memory ordering details are okay), but it should be documented
> that way.

In the case of using default_handler, wakes up thread if it is not in
progress and checks whether interrupts are handled with in some
duration. If un_handled interrupts reached 99000, display bad_irq trace
and disables IRQ. In our case we do not have one fault per interrupt. 
We ran some test case which continuously generates NX faults, the
handler thread is busy processing fault CRBs in FIFO and the later
interrupts are not handled. 

So added own handler which checks whether fault_thread is in progress.
If so returned IRQ_HANDLED. fault_lock is used to check valid entry
section and check faults_in_progress in handler. 

Added comment in vas_fault_handler(). 

> 
> Also why is the hard handler in a different file? Makes it harder to
> see how this works at a glance.

OK, Will change. I thought IRQ handler per VAS is added in vas.c since
it has VAS initialization and vas_fault.c is only for fault handling. 

> 
> faults_in_progress does not have to be atomic because it's always
> accessed under the lock. And IMO it should have a better name. If the
> NX can be causing more faults as we go, it really doesn't indicate
> anything about faults. It's whether or not the threaded handler is
> currently woken and processing faults.

Used atomic since not using spin_lock/unlock for failing case when
window (from CRB) is not valid. 

Sure, How about fault_thread_in_progress? As it is long name, used
faults_in_progress. 

> 
> > +			mutex_unlock(&vinst->mutex);
> > +			return IRQ_HANDLED;
> > +		}
> > +
> > +		spin_unlock_irqrestore(&vinst->fault_lock, flags);
> > +		vinst->fault_crbs++;
> > +		if (vinst->fault_crbs == (vinst->fault_fifo_size / CRB_SIZE))
> > +			vinst->fault_crbs = 0;
> > +
> > +		memcpy(crb, fifo, CRB_SIZE);
> > +		entry->stamp.nx.pswid = cpu_to_be32(FIFO_INVALID_ENTRY);
> > +		entry->ccw |= cpu_to_be32(CCW0_INVALID);
> > +		mutex_unlock(&vinst->mutex);
> > +
> > +		pr_devel("VAS[%d] fault_fifo %p, fifo %p, fault_crbs %d\n",
> > +				vinst->vas_id, vinst->fault_fifo, fifo,
> > +				vinst->fault_crbs);
> > +
> > +		window = vas_pswid_to_window(vinst,
> > +				be32_to_cpu(crb->stamp.nx.pswid));
> > +
> > +		if (IS_ERR(window)) {
> > +			/*
> > +			 * We got an interrupt about a specific send
> > +			 * window but we can't find that window and we can't
> > +			 * even clean it up (return credit).
> > +			 * But we should not get here.
> > +			 */
> > +			pr_err("VAS[%d] fault_fifo %p, fifo %p, pswid 0x%x, fault_crbs %d bad CRB?\n",
> > +				vinst->vas_id, vinst->fault_fifo, fifo,
> > +				be32_to_cpu(crb->stamp.nx.pswid),
> > +				vinst->fault_crbs);
> > +
> > +			WARN_ON_ONCE(1);
> > +			atomic_set(&vinst->faults_in_progress, 0);
> > +			return IRQ_HANDLED;
> 
> Shouldn't get here but you have a handler for it, so it should try to
> be graceful. Keep processing the rest of the FIFO until it's empty
> otherwise you have a missed wakeup here? Probably less code too, just
> delete the last 2 lines.

Derive window address from pswid which is pasted by NX. So if window
address is not valid means a bug, we should not be reached this. If
getting this failure, printed data in FIFO for around 10 CRBs. Not sure
whether proceeding with this failure. We may end up receiving lots of
messages on the console if we see similar failures in later CRBs.
Thinking may be disable IRQ so that kernel will not receive any
interrupts. I have not tested this case. I can change it to continue now
and Can I add disable IRQ as TODO? 

Thanks for your detailed review. 

> 
> Thanks,
> Nick
> 
> > +		}
> > +
> > +	} while (true);
> > +}
> > +
> > +/*
> >   * Fault window is opened per VAS instance. NX pastes fault CRB in fault
> >   * FIFO upon page faults.
> >   */
> > diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
> > index 1783fa9..1f31c18 100644
> > --- a/arch/powerpc/platforms/powernv/vas-window.c
> > +++ b/arch/powerpc/platforms/powernv/vas-window.c
> > @@ -1040,6 +1040,15 @@ struct vas_window *vas_tx_win_open(int vasid, enum vas_cop_type cop,
> >  		}
> >  	} else {
> >  		/*
> > +		 * Interrupt hanlder or fault window setup failed. Means
> > +		 * NX can not generate fault for page fault. So not
> > +		 * opening for user space tx window.
> > +		 */
> > +		if (!vinst->virq) {
> > +			rc = -ENODEV;
> > +			goto free_window;
> > +		}
> > +		/*
> >  		 * A user mapping must ensure that context switch issues
> >  		 * CP_ABORT for this thread.
> >  		 */
> > @@ -1254,3 +1263,54 @@ int vas_win_close(struct vas_window *window)
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(vas_win_close);
> > +
> > +struct vas_window *vas_pswid_to_window(struct vas_instance *vinst,
> > +		uint32_t pswid)
> > +{
> > +	struct vas_window *window;
> > +	int winid;
> > +
> > +	if (!pswid) {
> > +		pr_devel("%s: called for pswid 0!\n", __func__);
> > +		return ERR_PTR(-ESRCH);
> > +	}
> > +
> > +	decode_pswid(pswid, NULL, &winid);
> > +
> > +	if (winid >= VAS_WINDOWS_PER_CHIP)
> > +		return ERR_PTR(-ESRCH);
> > +
> > +	/*
> > +	 * If application closes the window before the hardware
> > +	 * returns the fault CRB, we should wait in vas_win_close()
> > +	 * for the pending requests. so the window must be active
> > +	 * and the process alive.
> > +	 *
> > +	 * If its a kernel process, we should not get any faults and
> > +	 * should not get here.
> > +	 */
> > +	window = vinst->windows[winid];
> > +
> > +	if (!window) {
> > +		pr_err("PSWID decode: Could not find window for winid %d pswid %d vinst 0x%p\n",
> > +			winid, pswid, vinst);
> > +		return NULL;
> > +	}
> > +
> > +	/*
> > +	 * Do some sanity checks on the decoded window.  Window should be
> > +	 * NX GZIP user send window. FTW windows should not incur faults
> > +	 * since their CRBs are ignored (not queued on FIFO or processed
> > +	 * by NX).
> > +	 */
> > +	if (!window->tx_win || !window->user_win || !window->nx_win ||
> > +			window->cop == VAS_COP_TYPE_FAULT ||
> > +			window->cop == VAS_COP_TYPE_FTW) {
> > +		pr_err("PSWID decode: id %d, tx %d, user %d, nx %d, cop %d\n",
> > +			winid, window->tx_win, window->user_win,
> > +			window->nx_win, window->cop);
> > +		WARN_ON(1);
> > +	}
> > +
> > +	return window;
> > +}
> > diff --git a/arch/powerpc/platforms/powernv/vas.c b/arch/powerpc/platforms/powernv/vas.c
> > index 557c8e4..3d9ba58 100644
> > --- a/arch/powerpc/platforms/powernv/vas.c
> > +++ b/arch/powerpc/platforms/powernv/vas.c
> > @@ -14,6 +14,8 @@
> >  #include <linux/of_platform.h>
> >  #include <linux/of_address.h>
> >  #include <linux/of.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/interrupt.h>
> >  #include <asm/prom.h>
> >  #include <asm/xive.h>
> >  
> > @@ -24,9 +26,53 @@
> >  
> >  static DEFINE_PER_CPU(int, cpu_vas_id);
> >  
> > +static irqreturn_t vas_fault_handler(int irq, void *dev_id)
> > +{
> > +	struct vas_instance *vinst = dev_id;
> > +	irqreturn_t ret = IRQ_WAKE_THREAD;
> > +	unsigned long flags;
> > +
> > +	/*
> > +	 * NX can generate an interrupt for multiple faults. So the
> > +	 * fault handler thread process all CRBs until finds invalid
> > +	 * entry. In case if NX sees continuous faults, it is possible
> > +	 * that the thread function entered with the first interrupt
> > +	 * can execute and process all valid CRBs.
> > +	 * So wake up thread only if the fault thread is not in progress.
> > +	 */
> > +	spin_lock_irqsave(&vinst->fault_lock, flags);
> > +
> > +	if (atomic_read(&vinst->faults_in_progress))
> > +		ret = IRQ_HANDLED;
> > +	else
> > +		atomic_set(&vinst->faults_in_progress, 1);
> > +
> > +	spin_unlock_irqrestore(&vinst->fault_lock, flags);
> > +
> > +	return ret;
> > +}
> > +
> >  static int vas_irq_fault_window_setup(struct vas_instance *vinst)
> >  {
> > -	return vas_setup_fault_window(vinst);
> > +	char devname[64];
> > +	int rc = 0;
> > +
> > +	snprintf(devname, sizeof(devname), "vas-%d", vinst->vas_id);
> > +	rc = request_threaded_irq(vinst->virq, vas_fault_handler,
> > +				vas_fault_thread_fn, 0, devname, vinst);
> > +
> > +	if (rc) {
> > +		pr_err("VAS[%d]: Request IRQ(%d) failed with %d\n",
> > +				vinst->vas_id, vinst->virq, rc);
> > +		goto out;
> > +	}
> > +
> > +	rc = vas_setup_fault_window(vinst);
> > +	if (rc)
> > +		free_irq(vinst->virq, vinst);
> > +
> > +out:
> > +	return rc;
> >  }
> >  
> >  static int init_vas_instance(struct platform_device *pdev)
> > @@ -109,6 +155,7 @@ static int init_vas_instance(struct platform_device *pdev)
> >  	list_add(&vinst->node, &vas_instances);
> >  	mutex_unlock(&vas_mutex);
> >  
> > +	spin_lock_init(&vinst->fault_lock);
> >  	/*
> >  	 * IRQ and fault handling setup is needed only for user space
> >  	 * send windows.
> > diff --git a/arch/powerpc/platforms/powernv/vas.h b/arch/powerpc/platforms/powernv/vas.h
> > index 6c4baf5..ecae7cd 100644
> > --- a/arch/powerpc/platforms/powernv/vas.h
> > +++ b/arch/powerpc/platforms/powernv/vas.h
> > @@ -326,7 +326,10 @@ struct vas_instance {
> >  
> >  	u64 irq_port;
> >  	int virq;
> > +	int fault_crbs;
> >  	int fault_fifo_size;
> > +	atomic_t faults_in_progress;
> > +	spinlock_t fault_lock;
> >  	void *fault_fifo;
> >  	struct vas_window *fault_win; /* Fault window */
> >  
> > @@ -424,6 +427,9 @@ struct vas_winctx {
> >  extern void vas_window_init_dbgdir(struct vas_window *win);
> >  extern void vas_window_free_dbgdir(struct vas_window *win);
> >  extern int vas_setup_fault_window(struct vas_instance *vinst);
> > +extern irqreturn_t vas_fault_thread_fn(int irq, void *data);
> > +extern struct vas_window *vas_pswid_to_window(struct vas_instance *vinst,
> > +						uint32_t pswid);
> >  
> >  static inline void vas_log_write(struct vas_window *win, char *name,
> >  			void *regptr, u64 val)
> > -- 
> > 1.8.3.1
> > 
> > 
> > 
> > 

Re: [PATCH v8 11/14] powerpc/vas: Do not use default credits for receive window

[Haren Myneni]

On Mon, 2020-03-23 at 12:40 +1000, Nicholas Piggin wrote:
> Haren Myneni's on March 19, 2020 4:18 pm:
> > 
> > System checkstops if RxFIFO overruns with more requests than the
> > maximum possible number of CRBs allowed in FIFO at any time. So
> > max credits value (rxattr.wcreds_max) is set and is passed to
> > vas_rx_win_open() by the the driver.
> 
> This seems like it should be a bug fix or merged in the NX fault
> window register patch or something.

Yes, it is a bug fix and can affect with any VAS windows, Not related to
NX fault window. Hence added as separate patch. 
 
> 
> Thanks,
> Nick
> 

Re: [PATCH v8 12/14] powerpc/vas: Return credits after handling fault

[Haren Myneni]

On Mon, 2020-03-23 at 12:44 +1000, Nicholas Piggin wrote:
> Haren Myneni's on March 19, 2020 4:19 pm:
> > 
> > NX expects OS to return credit for send window after processing each
> > fault. Also credit has to be returned even for fault window.
> 
> And this should be merged in the fault handler function.

credits are assigned and used per VAS window - default value is 1024 for
user space windows, and fault_fifo_size/CRb_SIZE for fault window.
 
When user space submits request, credit is taken on specific window (by
VAS). After successful processing of this request, NX return credit. In
case if NX sees fault, expects OS return credit for the corresponding
user space window after handling fault CRB. 

Similarly NX takes credit on fault window after pasting fault CRB and
expects return credit after handling fault CRB. NX workbook has on
credits usage and How this credit system works.

Thought vas_return_credit() is unique function and added as separate
patch so that easy to review.

> 
> > 
> > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> > ---
> >  arch/powerpc/platforms/powernv/vas-fault.c  |  9 +++++++++
> >  arch/powerpc/platforms/powernv/vas-window.c | 17 +++++++++++++++++
> >  arch/powerpc/platforms/powernv/vas.h        |  1 +
> >  3 files changed, 27 insertions(+)
> > 
> > diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c
> > index 40e1de4..292f7ba 100644
> > --- a/arch/powerpc/platforms/powernv/vas-fault.c
> > +++ b/arch/powerpc/platforms/powernv/vas-fault.c
> > @@ -238,6 +238,10 @@ irqreturn_t vas_fault_thread_fn(int irq, void *data)
> >  		memcpy(crb, fifo, CRB_SIZE);
> >  		entry->stamp.nx.pswid = cpu_to_be32(FIFO_INVALID_ENTRY);
> >  		entry->ccw |= cpu_to_be32(CCW0_INVALID);
> > +		/*
> > +		 * Return credit for the fault window.
> > +		 */
> 
> None of the comments in this patch are useful.
> 
> > +		vas_return_credit(vinst->fault_win, 0);
> 
> Can you use true/false for bools?
> 
> >  		mutex_unlock(&vinst->mutex);
> >  
> >  		pr_devel("VAS[%d] fault_fifo %p, fifo %p, fault_crbs %d\n",
> > @@ -267,6 +271,11 @@ irqreturn_t vas_fault_thread_fn(int irq, void *data)
> >  		}
> >  
> >  		update_csb(window, crb);
> > +		/*
> > +		 * Return credit for send window after processing
> > +		 * fault CRB.
> > +		 */
> 
> Any chance of a little bit of explanation how the credit system works?
> Or is it in the code somewhere already?
Sure will add few comments on credit usage.
> 
> I don't suppose there is a chance to batch credit updates with multiple
> faults? (maybe the MMIO is insignificant)

Yes, we return credit after processing each CRB. In the case of fault
window, NX can continue pasting fault CRB whenever the credit is
available. 

Thanks
Haren

> 
> > +		vas_return_credit(window, 1);
> >  	} while (true);
> >  }
> >  
> 
> Thanks,
> Nick

Re: [PATCH v8 03/14] powerpc/vas: Define nx_fault_stamp in coprocessor_request_block

[Michael Ellerman]

Haren Myneni <haren@linux.ibm.com> writes:
> On Mon, 2020-03-23 at 22:32 +1100, Michael Ellerman wrote:
>> Nicholas Piggin <npiggin@gmail.com> writes:
>> > Haren Myneni's on March 19, 2020 4:13 pm:
>> >> 
>> >> Kernel sets fault address and status in CRB for NX page fault on user
>> >> space address after processing page fault. User space gets the signal
>> >> and handles the fault mentioned in CRB by bringing the page in to
>> >> memory and send NX request again.
>> >> 
>> >> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
>> >> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
>> >> ---
>> >>  arch/powerpc/include/asm/icswx.h | 18 +++++++++++++++++-
>> >>  1 file changed, 17 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/arch/powerpc/include/asm/icswx.h b/arch/powerpc/include/asm/icswx.h
>> >> index 9872f85..b233d1e 100644
>> >> --- a/arch/powerpc/include/asm/icswx.h
>> >> +++ b/arch/powerpc/include/asm/icswx.h
>> >
>> > "icswx" is not a thing anymore, after 6ff4d3e96652 ("powerpc: Remove old 
>> > unused icswx based coprocessor support").
>> 
>> Yeah that commit ripped out some parts of the previous attempt at a user
>> visible API for this sort of "coprocessor" stuff. VAS is yet another
>> attempt to do something useful with most of the same pieces but some
>> slightly different details.
>> 
>> > I guess NX is reusing some 
>> > things from it, but it would be good to get rid of the cruft and re-name
>> > this file and and relevant names.
>> 
>> > NX already uses this file, so I guesss that can happen after this series.
>> 
>> A lot of the CRB/CSB stuff is still the same, and P8 still uses icswx.
>> But I'd be happy if the header was renamed eventually, as icswx is now a
>> legacy name.
>
> We can move all macros and struct definitions to vas.h and remove
> icswx.h. Can I do this after this series? 

Well they're still needed by the non-vas Power8 code, so that wouldn't
be quite right either :)

But yeah we can do whatever movement later as a cleanup.

cheers