From: Haren Myneni <haren@linux.ibm.com> Date: 2020-01-22 07:59:06
On power9, Virtual Accelerator Switchboard (VAS) allows user space or
kernel to communicate with Nest Accelerator (NX) directly using COPY/PASTE
instructions. NX provides verious 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 insted 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)
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 | 325 ++++++++++++++++++++++++++++
arch/powerpc/platforms/powernv/vas-window.c | 184 ++++++++++++++--
arch/powerpc/platforms/powernv/vas.c | 73 ++++++-
arch/powerpc/platforms/powernv/vas.h | 38 +++-
arch/powerpc/sysdev/xive/native.c | 29 ++-
10 files changed, 655 insertions(+), 47 deletions(-)
create mode 100644 arch/powerpc/platforms/powernv/vas-fault.c
--
1.8.3.1
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-01-22 08:08:27
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(-)
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-01-22 08:10:17
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(-)
@@ -487,24 +487,8 @@ int pnv_ocxl_spa_remove_pe_from_cache(void *platform_data, int pe_handle)intpnv_ocxl_alloc_xive_irq(u32*irq,u64*trigger_addr){-__be64flags,trigger_page;-s64rc;-u32hwirq;--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);-return0;-+returnxive_native_alloc_get_irq_info(OPAL_XIVE_ANY_CHIP,irq,+trigger_addr);}EXPORT_SYMBOL_GPL(pnv_ocxl_alloc_xive_irq);
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-01-22 08:12:49
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 <redacted>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/include/asm/icswx.h | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-01-22 08:15:08
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(-)
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-01-22 08:19:17
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 <redacted>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas-fault.c | 85 +++++++++++++++++++++++++++++
arch/powerpc/platforms/powernv/vas-window.c | 60 ++++++++++++++++++++
arch/powerpc/platforms/powernv/vas.c | 21 ++++++-
arch/powerpc/platforms/powernv/vas.h | 4 ++
4 files changed, 169 insertions(+), 1 deletion(-)
@@ -1254,3 +1263,54 @@ int vas_win_close(struct vas_window *window)return0;}EXPORT_SYMBOL_GPL(vas_win_close);++structvas_window*vas_pswid_to_window(structvas_instance*vinst,+uint32_tpswid)+{+intwinid;+structvas_window*window;++if(!pswid){+pr_devel("%s: called for pswid 0!\n",__func__);+returnERR_PTR(-ESRCH);+}++decode_pswid(pswid,NULL,&winid);++if(winid>=VAS_WINDOWS_PER_CHIP)+returnERR_PTR(-ESRCH);++/*+*Ifapplicationclosesthewindowbeforethehardware+*returnsthefaultCRB,weshouldwaitinvas_win_close()+*forthependingrequests.sothewindowmustbeactive+*andtheprocessalive.+*+*Ifitsakernelprocess,weshouldnotgetanyfaultsand+*shouldnotgethere.+*/+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);+returnNULL;+}++/*+*Dosomesanitychecksonthedecodedwindow.Windowshouldbe+*NXGZIPusersendwindow.FTWwindowsshouldnotincurfaults+*sincetheirCRBsareignored(notqueuedonFIFOorprocessed+*byNX).+*/+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);+}++returnwindow;+}
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-01-22 08:21:20
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 <redacted>
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(-)
@@ -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. */
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-01-22 08:23:35
Process close windows after its requests are completed. In multi-thread
applications, child can open a window but release FD will not be called
upon its exit. Parent thread will be closing it later upon its exit.
The parent can also send NX requests with this window and NX can
generate page faults. After kernel handles the page fault, send
signal to process by using PID if CSB address is invalid. Parent
thread will not receive signal since its PID is different from the one
saved in vas_window. So use tgid in case if the task for the pid saved
in window is not running and send signal to its parent.
To prevent reusing the pid until the window closed, take reference to
pid and task mm.
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(-)
@@ -1068,8 +1067,43 @@ struct vas_window *vas_tx_win_open(int vasid, enum vas_cop_type cop,gotofree_window;}-set_vinst_win(vinst,txwin);+if(txwin->user_win){+/*+*Windowopenedbychildthreadmaynotbeclosedwhen+*itexits.Sotakereferencetoitspidandreleaseit+*whenthewindowisfreebyparentthread.+*Acquireareferencetothetask'spidtomakesure+*pidwillnotbere-used-neededonlyformultithread+*applications.+*/+txwin->pid=get_task_pid(current,PIDTYPE_PID);+/*+*Acquireareferencetothetask'smm.+*/+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;+gotofree_window;+}++mmgrab(txwin->mm);+mmput(txwin->mm);+mm_context_add_copro(txwin->mm);+/*+*Processcloseswindowduringexit.Inthecaseof+*multithreadapplication,childcanopenwindowand+*canexitwithoutclosingit.Expectsparentthread+*touseandclosethewindow.Sodonotneedtotake+*pidreferenceforparentthread.+*/+txwin->tgid=find_get_pid(task_tgid_vnr(current));+}++set_vinst_win(vinst,txwin);returntxwin;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){+mmdrop(window->mm);+mm_context_remove_copro(window->mm);+}+}put_rx_win(window->rxwin);+}vas_window_free(window);
@@ -340,7 +340,9 @@ struct vas_window {booluser_win;/* True if user space window */void*hvwc_map;/* HV window context */void*uwc_map;/* OS/User window context */-pid_tpid;/* Linux process id of owner */+structpid*pid;/* Linux process id of owner */+structpid*tgid;/* Thread group ID of owner */+structmm_struct*mm;/* Linux process mm_struct */intwcreds_max;/* Window credits */char*dbgname;
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-01-22 08:25:26
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 <redacted>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas-fault.c | 116 +++++++++++++++++++++++++++++
1 file changed, 116 insertions(+)
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-01-22 08:29:42
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(-)
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-01-22 08:31:40
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 <redacted>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas-fault.c | 10 ++++++++++
arch/powerpc/platforms/powernv/vas-window.c | 17 +++++++++++++++++
arch/powerpc/platforms/powernv/vas.h | 1 +
3 files changed, 28 insertions(+)
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-01-22 08:33:36
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 message in case taking longer to close the
window.
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas-window.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-01-22 08:35:11
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(-)
@@ -1316,14 +1316,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 */
From: Michael Neuling <hidden> Date: 2020-02-07 05:47:37
On Wed, 2020-01-22 at 00:17 -0800, Haren Myneni wrote:
quoted hunk
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 <redacted>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas-fault.c | 116
+++++++++++++++++++++++++++++
1 file changed, 116 insertions(+)
This looks wrong and I don't understand the comment. You need to convert this
back to be64 to write it to csb.address. ie.
csb.address = cpu_to_be64(be64_to_cpu(crb->stamp.nx.fault_storage_addr));
Which I think you can just avoid the endian conversion all together.
+ 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 (!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.
+ */
+ smp_mb();
+ if (!rc) {
+ csb.flags = CSB_V;
+ rc = copy_to_user(csb_addr, &csb, sizeof(u8));
+ }
+ unuse_mm(window->mm);
+ put_task_struct(tsk);
+
+ /* Success */
+ if (!rc)
+ return;
+
+ pr_err("Invalid CSB address 0x%p signalling pid(%d)\n",
+ csb_addr, pid_vnr(pid));
This is a userspace error, not a kernel error. This should not be a pr_err().
Userspace could spam the console with this.
+
+ 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();
why the rcu_read_un/lock() here?
quoted hunk
+
+ pr_devel("%s(): pid %d kill_proc_info() rc %d\n", __func__,
+ pid_vnr(pid), rc);
+}
+
+/*
* Process CRBs that we receive on the fault window.
*/
irqreturn_t vas_fault_handler(int irq, void *data)
From: Michael Neuling <hidden> Date: 2020-02-07 05:58:30
/*
+ * Process CRBs that we receive on the fault window.
+ */
+irqreturn_t vas_fault_handler(int irq, void *data)
+{
+ struct vas_instance *vinst = data;
+ struct coprocessor_request_block buf, *crb;
+ struct vas_window *window;
+ void *fifo;
+
+ /*
+ * 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 non-zero and
+ * use this to check whether CRB is valid.
+ * After reading CRB entry, it is reset with 0's in fault FIFO.
+ *
+ * In case kernel receives another interrupt with different page
+ * fault and CRBs are processed by the previous handling, will be
+ * returned from this function when it sees invalid CRB (means 0's).
+ */
+ do {
+ mutex_lock(&vinst->mutex);
This isn't going to work.
From Documentation/locking/mutex-design.rst
- Mutexes may not be used in hardware or software interrupt
contexts such as tasklets and timers.
Mikey
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-02-10 05:15:33
Mikey, Thanks for your review comments.
On Fri, 2020-02-07 at 16:46 +1100, Michael Neuling wrote:
On Wed, 2020-01-22 at 00:17 -0800, Haren Myneni wrote:
quoted
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 <redacted>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas-fault.c | 116
+++++++++++++++++++++++++++++
1 file changed, 116 insertions(+)
This looks wrong and I don't understand the comment. You need to convert this
back to be64 to write it to csb.address. ie.
csb.address = cpu_to_be64(be64_to_cpu(crb->stamp.nx.fault_storage_addr));
Which I think you can just avoid the endian conversion all together.
NX pastes fault CRB in big-endian, so passing this address in CPU format
to user space, otherwise the library has to convert.
What is the standard way for passing to user space?
quoted
+ 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 (!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.
+ */
+ smp_mb();
+ if (!rc) {
+ csb.flags = CSB_V;
+ rc = copy_to_user(csb_addr, &csb, sizeof(u8));
+ }
+ unuse_mm(window->mm);
+ put_task_struct(tsk);
+
+ /* Success */
+ if (!rc)
+ return;
+
+ pr_err("Invalid CSB address 0x%p signalling pid(%d)\n",
+ csb_addr, pid_vnr(pid));
This is a userspace error, not a kernel error. This should not be a pr_err().
Userspace could spam the console with this.
Will change it to pr_debug/info. Added pr_err() during development and
missed to remove.
quoted
+
+ 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();
why the rcu_read_un/lock() here?
Used same as in kill_proc_info()/kill_something_info()
quoted
+
+ pr_devel("%s(): pid %d kill_proc_info() rc %d\n", __func__,
+ pid_vnr(pid), rc);
+}
+
+/*
* Process CRBs that we receive on the fault window.
*/
irqreturn_t vas_fault_handler(int irq, void *data)
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-02-10 05:19:51
On Fri, 2020-02-07 at 16:57 +1100, Michael Neuling wrote:
quoted
/*
+ * Process CRBs that we receive on the fault window.
+ */
+irqreturn_t vas_fault_handler(int irq, void *data)
+{
+ struct vas_instance *vinst = data;
+ struct coprocessor_request_block buf, *crb;
+ struct vas_window *window;
+ void *fifo;
+
+ /*
+ * 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 non-zero and
+ * use this to check whether CRB is valid.
+ * After reading CRB entry, it is reset with 0's in fault FIFO.
+ *
+ * In case kernel receives another interrupt with different page
+ * fault and CRBs are processed by the previous handling, will be
+ * returned from this function when it sees invalid CRB (means 0's).
+ */
+ do {
+ mutex_lock(&vinst->mutex);
This isn't going to work.
From Documentation/locking/mutex-design.rst
- Mutexes may not be used in hardware or software interrupt
contexts such as tasklets and timers.
Initially used kernel thread per VAS instance and later using IRQ
thread.
vas_fault_handler() is IRQ thread function, not IRQ handler. I thought
we can use mutex_lock() in thread function.
From: Michael Neuling <hidden> Date: 2020-02-10 09:27:31
quoted
quoted
+
+ csb.cc = CSB_CC_TRANSLATION;
+ csb.ce = CSB_CE_TERMINATION;
+ csb.cs = 0;
+ csb.count = 0;
+
+ /*
+ * Returns the fault address in CPU format since it is passed with
+ * signal. But if the user space expects BE format, need changes.
+ * i.e either kernel (here) or user should convert to CPU format.
+ * Not both!
+ */
+ csb.address = be64_to_cpu(crb->stamp.nx.fault_storage_addr);
This looks wrong and I don't understand the comment. You need to convert
this
back to be64 to write it to csb.address. ie.
csb.address = cpu_to_be64(be64_to_cpu(crb->stamp.nx.fault_storage_addr));
Which I think you can just avoid the endian conversion all together.
NX pastes fault CRB in big-endian, so passing this address in CPU format
to user space, otherwise the library has to convert.
OK, then please change the definition in struct coprocessor_status_block to just
__u64.
struct coprocessor_status_block {
u8 flags;
u8 cs;
u8 cc;
u8 ce;
__be32 count;
__be64 address;
} __packed __aligned(CSB_ALIGN);
Big but....
I thought "struct coprocessor_status_block" was also written by hardware. If
that's the case then it needs to be __be64 and you need the kernel to synthesize
exactly what the hardware is doing. Hence the struct definition is correct and
the kernel needs to convert to _be64 on writing.
What is the standard way for passing to user space?
CPU endian.
quoted
quoted
+ * 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();
why the rcu_read_un/lock() here?
Used same as in kill_proc_info()/kill_something_info()
From: Michael Neuling <hidden> Date: 2020-02-11 04:10:25
On Sun, 2020-02-09 at 21:17 -0800, Haren Myneni wrote:
On Fri, 2020-02-07 at 16:57 +1100, Michael Neuling wrote:
quoted
quoted
/*
+ * Process CRBs that we receive on the fault window.
+ */
+irqreturn_t vas_fault_handler(int irq, void *data)
+{
+ struct vas_instance *vinst = data;
+ struct coprocessor_request_block buf, *crb;
+ struct vas_window *window;
+ void *fifo;
+
+ /*
+ * 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 non-zero and
+ * use this to check whether CRB is valid.
+ * After reading CRB entry, it is reset with 0's in fault FIFO.
+ *
+ * In case kernel receives another interrupt with different page
+ * fault and CRBs are processed by the previous handling, will be
+ * returned from this function when it sees invalid CRB (means 0's).
+ */
+ do {
+ mutex_lock(&vinst->mutex);
This isn't going to work.
From Documentation/locking/mutex-design.rst
- Mutexes may not be used in hardware or software interrupt
contexts such as tasklets and timers.
Initially used kernel thread per VAS instance and later using IRQ
thread.
vas_fault_handler() is IRQ thread function, not IRQ handler. I thought
we can use mutex_lock() in thread function.
Sorry, I missed it was a threaded IRQ handler, so I think is ok to use a
mutex_lock() in there.
You should run with CONFIG DEBUG_MUTEXES and CONFIG_LOCKDEP enabled to give you
some more confidence.
It would be good to document how this mutex is used and document the start of
the function so it doesn't get changed later to a non-threaded handler.
Mikey