* Re: [PATCH v8 08/14] powerpc/vas: Take reference to PID and mm for user space windows
From: Nicholas Piggin @ 2020-03-23 2:34 UTC (permalink / raw)
To: Haren Myneni, mpe; +Cc: mikey, ajd, hch, oohall, sukadev, linuxppc-dev, herbert
In-Reply-To: <1584598613.9256.15257.camel@hbabu-laptop>
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
>
>
>
>
^ permalink raw reply
* Argh, can't find dcache properties !
From: Chris Packham @ 2020-03-23 2:25 UTC (permalink / raw)
To: christophe.leroy@c-s.fr, paulus@samba.org, mpe@ellerman.id.au,
benh@kernel.crashing.org, tglx@linutronix.de, cai@lca.pw
Cc: Hamish Martin, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
Hi All,
Just booting up v5.5.11 on a Freescale T2080RDB and I'm seeing the
following mesage.
kern.warning linuxbox kernel: Argh, can't find dcache properties !
kern.warning linuxbox kernel: Argh, can't find icache properties !
This was changed from DBG() to pr_warn() in commit 3b9176e9a874
("powerpc/setup_64: fix -Wempty-body warnings") but the message seems
to be much older than that. So it's probably been an issue on the T2080
(and other QorIQ SoCs) for a while.
Looking at the code the t208x doesn't specifiy any of the d-cache-
size/i-cache-size properties. Should I add them to silence the warning
or switch it to pr_debug()/pr_info()?
Thanks,
Chris
^ permalink raw reply
* Re: [PATCH v8 06/14] powerpc/vas: Setup thread IRQ handler per VAS instance
From: Nicholas Piggin @ 2020-03-23 2:23 UTC (permalink / raw)
To: Haren Myneni, mpe; +Cc: mikey, ajd, hch, oohall, sukadev, linuxppc-dev, herbert
In-Reply-To: <1584598540.9256.15252.camel@hbabu-laptop>
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
>
>
>
>
^ permalink raw reply
* Re: [PATCH v8 03/14] powerpc/vas: Define nx_fault_stamp in coprocessor_request_block
From: Nicholas Piggin @ 2020-03-23 1:30 UTC (permalink / raw)
To: Haren Myneni; +Cc: mikey, ajd, hch, oohall, sukadev, linuxppc-dev, herbert
In-Reply-To: <1584925071.9256.15311.camel@hbabu-laptop>
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
^ permalink raw reply
* Re: [PATCH 1/2] dma-mapping: add a dma_ops_bypass flag to struct device
From: Alexey Kardashevskiy @ 2020-03-23 1:28 UTC (permalink / raw)
To: Christoph Hellwig, iommu
Cc: Greg Kroah-Hartman, Joerg Roedel, Robin Murphy, linux-kernel,
Aneesh Kumar K.V, linuxppc-dev, Lu Baolu
In-Reply-To: <20200320141640.366360-2-hch@lst.de>
On 21/03/2020 01:16, Christoph Hellwig wrote:
> Several IOMMU drivers have a bypass mode where they can use a direct
> mapping if the devices DMA mask is large enough. Add generic support
> to the core dma-mapping code to do that to switch those drivers to
> a common solution.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> include/linux/device.h | 6 ++++++
> include/linux/dma-mapping.h | 30 ++++++++++++++++++------------
> kernel/dma/mapping.c | 36 +++++++++++++++++++++++++++---------
> 3 files changed, 51 insertions(+), 21 deletions(-)
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 0cd7c647c16c..09be8bb2c4a6 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -525,6 +525,11 @@ struct dev_links_info {
> * sync_state() callback.
> * @dma_coherent: this particular device is dma coherent, even if the
> * architecture supports non-coherent devices.
> + * @dma_ops_bypass: If set to %true then the dma_ops are bypassed for the
> + * streaming DMA operations (->map_* / ->unmap_* / ->sync_*),
> + * and optionall (if the coherent mask is large enough) also
> + * for dma allocations. This flag is managed by the dma ops
> + * instance from ->dma_supported.
> *
> * At the lowest level, every device in a Linux system is represented by an
> * instance of struct device. The device structure contains the information
> @@ -625,6 +630,7 @@ struct device {
> defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
> bool dma_coherent:1;
> #endif
> + bool dma_ops_bypass : 1;
> };
>
> static inline struct device *kobj_to_dev(struct kobject *kobj)
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 330ad58fbf4d..c3af0cf5e435 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -188,9 +188,15 @@ static inline int dma_mmap_from_global_coherent(struct vm_area_struct *vma,
> }
> #endif /* CONFIG_DMA_DECLARE_COHERENT */
>
> -static inline bool dma_is_direct(const struct dma_map_ops *ops)
> +/*
> + * Check if the devices uses a direct mapping for streaming DMA operations.
> + * This allows IOMMU drivers to set a bypass mode if the DMA mask is large
> + * enough.
> + */
> +static inline bool dma_map_direct(struct device *dev,
> + const struct dma_map_ops *ops)
> {
> - return likely(!ops);
> + return likely(!ops) || dev->dma_ops_bypass;
> }
>
> /*
> @@ -279,7 +285,7 @@ static inline dma_addr_t dma_map_page_attrs(struct device *dev,
> dma_addr_t addr;
>
> BUG_ON(!valid_dma_direction(dir));
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> addr = dma_direct_map_page(dev, page, offset, size, dir, attrs);
> else
> addr = ops->map_page(dev, page, offset, size, dir, attrs);
> @@ -294,7 +300,7 @@ static inline void dma_unmap_page_attrs(struct device *dev, dma_addr_t addr,
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> BUG_ON(!valid_dma_direction(dir));
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> dma_direct_unmap_page(dev, addr, size, dir, attrs);
> else if (ops->unmap_page)
> ops->unmap_page(dev, addr, size, dir, attrs);
> @@ -313,7 +319,7 @@ static inline int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
> int ents;
>
> BUG_ON(!valid_dma_direction(dir));
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> ents = dma_direct_map_sg(dev, sg, nents, dir, attrs);
> else
> ents = ops->map_sg(dev, sg, nents, dir, attrs);
> @@ -331,7 +337,7 @@ static inline void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg
>
> BUG_ON(!valid_dma_direction(dir));
> debug_dma_unmap_sg(dev, sg, nents, dir);
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> dma_direct_unmap_sg(dev, sg, nents, dir, attrs);
> else if (ops->unmap_sg)
> ops->unmap_sg(dev, sg, nents, dir, attrs);
> @@ -352,7 +358,7 @@ static inline dma_addr_t dma_map_resource(struct device *dev,
> if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr))))
> return DMA_MAPPING_ERROR;
>
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> addr = dma_direct_map_resource(dev, phys_addr, size, dir, attrs);
> else if (ops->map_resource)
> addr = ops->map_resource(dev, phys_addr, size, dir, attrs);
> @@ -368,7 +374,7 @@ static inline void dma_unmap_resource(struct device *dev, dma_addr_t addr,
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> BUG_ON(!valid_dma_direction(dir));
> - if (!dma_is_direct(ops) && ops->unmap_resource)
> + if (!dma_map_direct(dev, ops) && ops->unmap_resource)
> ops->unmap_resource(dev, addr, size, dir, attrs);
> debug_dma_unmap_resource(dev, addr, size, dir);
> }
> @@ -380,7 +386,7 @@ static inline void dma_sync_single_for_cpu(struct device *dev, dma_addr_t addr,
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> BUG_ON(!valid_dma_direction(dir));
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> dma_direct_sync_single_for_cpu(dev, addr, size, dir);
> else if (ops->sync_single_for_cpu)
> ops->sync_single_for_cpu(dev, addr, size, dir);
> @@ -394,7 +400,7 @@ static inline void dma_sync_single_for_device(struct device *dev,
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> BUG_ON(!valid_dma_direction(dir));
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> dma_direct_sync_single_for_device(dev, addr, size, dir);
> else if (ops->sync_single_for_device)
> ops->sync_single_for_device(dev, addr, size, dir);
> @@ -408,7 +414,7 @@ dma_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg,
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> BUG_ON(!valid_dma_direction(dir));
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> dma_direct_sync_sg_for_cpu(dev, sg, nelems, dir);
> else if (ops->sync_sg_for_cpu)
> ops->sync_sg_for_cpu(dev, sg, nelems, dir);
> @@ -422,7 +428,7 @@ dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> BUG_ON(!valid_dma_direction(dir));
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> dma_direct_sync_sg_for_device(dev, sg, nelems, dir);
> else if (ops->sync_sg_for_device)
> ops->sync_sg_for_device(dev, sg, nelems, dir);
> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> index 12ff766ec1fa..fdea45574345 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -105,6 +105,24 @@ void *dmam_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle,
> }
> EXPORT_SYMBOL(dmam_alloc_attrs);
>
> +static bool dma_alloc_direct(struct device *dev, const struct dma_map_ops *ops)
> +{
> + if (!ops)
> + return true;
> +
> + /*
> + * Allows IOMMU drivers to bypass dynamic translations if the DMA mask
> + * is large enough.
> + */
> + if (dev->dma_ops_bypass) {
> + if (min_not_zero(dev->coherent_dma_mask, dev->bus_dma_limit) >=
> + dma_direct_get_required_mask(dev))
> + return true;
> + }
Why not do this in dma_map_direct() as well?
Or simply have just one dma_map_direct()?
And one more general question - we need a way to use non-direct IOMMU
for RAM above certain limit.
Let's say we have a system with:
0 .. 0x1.0000.0000
0x100.0000.0000 .. 0x101.0000.0000
2x4G, each is 1TB aligned. And we can map directly only the first 4GB
(because of the maximum IOMMU table size) but not the other. And 1:1 on
that "pseries" is done with offset=0x0800.0000.0000.0000.
So we want to check every bus address against dev->bus_dma_limit, not
dev->coherent_dma_mask. In the example above I'd set bus_dma_limit to
0x0800.0001.0000.0000 and 1:1 mapping for the second 4GB would not be
tried. Does this sound reasonable? Thanks,
> +
> + return false;
> +}
> +
> /*
> * Create scatter-list for the already allocated DMA buffer.
> */
> @@ -138,7 +156,7 @@ int dma_get_sgtable_attrs(struct device *dev, struct sg_table *sgt,
> {
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> - if (dma_is_direct(ops))
> + if (dma_alloc_direct(dev, ops))
> return dma_direct_get_sgtable(dev, sgt, cpu_addr, dma_addr,
> size, attrs);
> if (!ops->get_sgtable)
> @@ -206,7 +224,7 @@ bool dma_can_mmap(struct device *dev)
> {
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> - if (dma_is_direct(ops))
> + if (dma_alloc_direct(dev, ops))
> return dma_direct_can_mmap(dev);
> return ops->mmap != NULL;
> }
> @@ -231,7 +249,7 @@ int dma_mmap_attrs(struct device *dev, struct vm_area_struct *vma,
> {
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> - if (dma_is_direct(ops))
> + if (dma_alloc_direct(dev, ops))
> return dma_direct_mmap(dev, vma, cpu_addr, dma_addr, size,
> attrs);
> if (!ops->mmap)
> @@ -244,7 +262,7 @@ u64 dma_get_required_mask(struct device *dev)
> {
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> return dma_direct_get_required_mask(dev);
> if (ops->get_required_mask)
> return ops->get_required_mask(dev);
> @@ -275,7 +293,7 @@ void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle,
> /* let the implementation decide on the zone to allocate from: */
> flag &= ~(__GFP_DMA | __GFP_DMA32 | __GFP_HIGHMEM);
>
> - if (dma_is_direct(ops))
> + if (dma_alloc_direct(dev, ops))
> cpu_addr = dma_direct_alloc(dev, size, dma_handle, flag, attrs);
> else if (ops->alloc)
> cpu_addr = ops->alloc(dev, size, dma_handle, flag, attrs);
> @@ -307,7 +325,7 @@ void dma_free_attrs(struct device *dev, size_t size, void *cpu_addr,
> return;
>
> debug_dma_free_coherent(dev, size, cpu_addr, dma_handle);
> - if (dma_is_direct(ops))
> + if (dma_alloc_direct(dev, ops))
> dma_direct_free(dev, size, cpu_addr, dma_handle, attrs);
> else if (ops->free)
> ops->free(dev, size, cpu_addr, dma_handle, attrs);
> @@ -318,7 +336,7 @@ int dma_supported(struct device *dev, u64 mask)
> {
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> - if (dma_is_direct(ops))
> + if (!ops)
> return dma_direct_supported(dev, mask);
> if (!ops->dma_supported)
> return 1;
> @@ -374,7 +392,7 @@ void dma_cache_sync(struct device *dev, void *vaddr, size_t size,
>
> BUG_ON(!valid_dma_direction(dir));
>
> - if (dma_is_direct(ops))
> + if (dma_alloc_direct(dev, ops))
> arch_dma_cache_sync(dev, vaddr, size, dir);
> else if (ops->cache_sync)
> ops->cache_sync(dev, vaddr, size, dir);
> @@ -386,7 +404,7 @@ size_t dma_max_mapping_size(struct device *dev)
> const struct dma_map_ops *ops = get_dma_ops(dev);
> size_t size = SIZE_MAX;
>
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> size = dma_direct_max_mapping_size(dev);
> else if (ops && ops->max_mapping_size)
> size = ops->max_mapping_size(dev);
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v8 04/14] powerpc/vas: Alloc and setup IRQ and trigger port address
From: Nicholas Piggin @ 2020-03-23 1:06 UTC (permalink / raw)
To: Haren Myneni, mpe; +Cc: mikey, ajd, hch, oohall, sukadev, linuxppc-dev, herbert
In-Reply-To: <1584598473.9256.15248.camel@hbabu-laptop>
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
^ permalink raw reply
* Re: [PATCH V7 09/14] powerpc/vas: Update CSB and notify process for fault CRBs
From: Haren Myneni @ 2020-03-23 1:06 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: mikey, ajd, hch, oohall, sukadev, linuxppc-dev, herbert
In-Reply-To: <1584920780.45onmviqrl.astroid@bobo.none>
On Mon, 2020-03-23 at 10:06 +1000, Nicholas Piggin wrote:
> Haren Myneni's on March 18, 2020 5:27 am:
> > On Tue, 2020-03-17 at 16:28 +1100, Michael Ellerman wrote:
> >> Haren Myneni <haren@linux.ibm.com> writes:
> >> > 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 | 114 +++++++++++++++++++++++++++++
> >> > 1 file changed, 114 insertions(+)
> >> >
> >> > diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c
> >> > index 1c6d5cc..751ce48 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,118 @@
> >> > #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)
> >> > +{
> >> > + int rc;
> >> > + struct pid *pid;
> >> > + void __user *csb_addr;
> >> > + struct task_struct *tsk;
> >> > + struct kernel_siginfo info;
> >> > + struct coprocessor_status_block csb;
> >>
> >> csb is on the stack, and later copied to user, which is a risk for
> >> creating an infoleak.
> >>
> >> Also please use reverse Christmas tree layout for your variables.
> >>
> >> > +
> >> > + /*
> >> > + * NX user space windows can not be opened for task->mm=NULL
> >> > + * and faults will not be generated for kernel requests.
> >> > + */
> >> > + if (!window->mm || !window->user_win)
> >> > + return;
> >>
> >> If that's a should-never-happen condition then should it do a
> >> WARN_ON_ONCE() rather than silently returning?
> >
> > Will add WARN_ON
> >
> >>
> >> > + csb_addr = (void __user *)be64_to_cpu(crb->csb_addr);
> >> > +
> >> > + 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;
> >>
> >> I'm pretty sure this has initialised all the fields of csb.
> >>
> >> But, I'd still be much happier if you zeroed the whole struct to begin
> >> with, that way we know for sure we can't leak any uninitialised bytes to
> >> userspace. It's only 16 bytes so it shouldn't add any noticeable
> >> overhead.
> > Sure, will initialize csb
> >>
> >> > +
> >> > + 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;
> >>
> >> Similar question on WARN_ON_ONCE()
> > Yes, we can add WARN_ON
> >>
> >> > + }
> >> > +
> >> > + /* Return if the task is exiting. */
> >>
> >> Why? Just because it's no use? It's racy isn't it, so it can't be for
> >> correctness?
> > Yes process is exiting and no need to update CSB. We release the
> > task->usage refcount after copy_to_user().
> >
> >>
> >> > + if (tsk->flags & PF_EXITING) {
> >> > + put_task_struct(tsk);
> >> > + return;
> >> > + }
> >> > +
> >> > + use_mm(window->mm);
> >>
> >> There's no check that csb_addr is actually pointing into userspace, but
> >> copy_to_user() does it for you.
> >>
> >> > + 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();
> >>
> >> You only need to order the stores above vs the store below to csb.flags.
> >> So you should only need an smp_wmb() here.
> > Sure, will add
> > if (!rc) {
> > csb.flags = CSB_V;
> > smp_mb();
> > rc = copy_to_user(csb_addr, &csb, sizeof(u8));
> > }
> >
> >>
> >> > + 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_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.
> >> > + */
>
> The code would read a bit better if this comment goes at the start of
> this error handling process it describes (before the error message).
>
> And I feel a bit hypocritical complaining about readability, but it
> could possibly do with some work.
>
> /*
> * The application should have provided a valid mapping for the
> * csb, and not unmap it before the csb.flags update, so the
> * copy_to_user should not fail.
> *
> * If the application fails to follow this protocol, log a kernel
> * error and send a SEGV to the pid. This signal may be ignored,
> * so can't use force_sig_fault_to_task()
> */
>
> Something like tthat?
Thanks Nick, will update.
>
> >> > + rcu_read_lock();
> >> > + rc = kill_pid_info(SIGSEGV, &info, pid);
> >> > + rcu_read_unlock();
>
> ipc/mqueue.c says kill_pid_info doesn't need rcu_read_lock(). AFAIKS
> it's held around kill_pid_info in kernel/signal.c for the find_vpid().
I was following as in kill_proc_info(). Will remove rcu_read_lock().
>
> Thanks,
> Nick
>
> >>
> >> Shouldn't this be using force_sig_fault_to_task() or another helper,
> >> rather than open-coding?
> >
> > Applications or nxz library can ignore this signal based on si_addr or
> > take action like resend new request with valid csb_addr. Hence I did not
> > use force_sig_info_to_task().
>
> >
> >>
> >> > +
> >> > + 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 +224,7 @@ irqreturn_t vas_fault_thread_fn(int irq, void *data)
> >> > return IRQ_HANDLED;
> >> > }
> >> >
> >> > + update_csb(window, crb);
> >> > } while (true);
> >> > }
> >> >
> >> > --
> >> > 1.8.3.1
> >>
> >> cheers
> >
> >
> >
^ permalink raw reply
* Re: [PATCH v8 03/14] powerpc/vas: Define nx_fault_stamp in coprocessor_request_block
From: Haren Myneni @ 2020-03-23 0:57 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: mikey, ajd, hch, oohall, sukadev, linuxppc-dev, herbert
In-Reply-To: <1584923120.arc9bj6gmg.astroid@bobo.none>
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
>
^ permalink raw reply
* Re: [PATCH v3 9/9] Documentation/powerpc: VAS API
From: Haren Myneni @ 2020-03-23 0:54 UTC (permalink / raw)
To: Daniel Axtens
Cc: mikey, herbert, npiggin, linux-crypto, sukadev, linuxppc-dev
In-Reply-To: <87blorw6wc.fsf@dja-thinkpad.axtens.net>
On Fri, 2020-03-20 at 23:24 +1100, Daniel Axtens wrote:
> Hi Haren,
>
> This is good documentation.
>
> > Power9 introduced Virtual Accelerator Switchboard (VAS) which allows
> > userspace to communicate with Nest Accelerator (NX) directly. But
> > kernel has to establish channel to NX for userspace. This document
> > describes user space API that application can use to establish
> > communication channel.
> >
> > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> > ---
> > Documentation/powerpc/index.rst | 1 +
> > Documentation/powerpc/vas-api.rst | 246 ++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 247 insertions(+)
> > create mode 100644 Documentation/powerpc/vas-api.rst
> >
> > diff --git a/Documentation/powerpc/index.rst b/Documentation/powerpc/index.rst
> > index 0d45f0f..afe2d5e 100644
> > --- a/Documentation/powerpc/index.rst
> > +++ b/Documentation/powerpc/index.rst
> > @@ -30,6 +30,7 @@ powerpc
> > syscall64-abi
> > transactional_memory
> > ultravisor
> > + vas-api
> >
> > .. only:: subproject and html
> >
> > diff --git a/Documentation/powerpc/vas-api.rst b/Documentation/powerpc/vas-api.rst
> > new file mode 100644
> > index 0000000..13ce4e7
> > --- /dev/null
> > +++ b/Documentation/powerpc/vas-api.rst
> > @@ -0,0 +1,246 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +.. _VAS-API:
> > +
> > +===================================================
> > +Virtual Accelerator Switchboard (VAS) userspace API
> > +===================================================
> > +
> > +Introduction
> > +============
> > +
> > +Power9 processor introduced Virtual Accelerator Switchboard (VAS) which
> > +allows both userspace and kernel communicate to co-processor
> > +(hardware accelerator) referred to as the Nest Accelerator (NX). The NX
> > +unit comprises of one or more hardware engines or co-processor types
> > +such as 842 compression, GZIP compression and encryption. On power9,
> > +userspace applications will have access to only GZIP Compression engine
> > +which supports ZLIB and GZIP compression algorithms in the hardware.
> > +
> > +To communicate with NX, kernel has to establish a channel or window and
> > +then requests can be submitted directly without kernel involvement.
> > +Requests to the GZIP engine must be formatted as a co-processor Request
> > +Block (CRB) and these CRBs must be submitted to the NX using COPY/PASTE
> > +instructions to paste the CRB to hardware address that is associated with
> > +the engine's request queue.
> > +
> > +The GZIP engine provides two priority levels of requests: Normal and
> > +High. Only Normal requests are supported from userspace right now.
> > +
> > +This document explains userspace API that is used to interact with
> > +kernel to setup channel / window which can be used to send compression
> > +requests directly to NX accelerator.
> > +
> > +
> > +Overview
> > +========
> > +
> > +Application access to the GZIP engine is provided through
> > +/dev/crypto/nx-gzip device node implemented by the VAS/NX device driver.
> > +An application must open the /dev/crypto/nx-gzip device to obtain a file
> > +descriptor (fd). Then should issue VAS_TX_WIN_OPEN ioctl with this fd to
> > +establish connection to the engine. It means send window is opened on GZIP
> > +engine for this process. Once a connection is established, the application
> > +should use the mmap() system call to map the hardware address of engine's
> > +request queue into the application's virtual address space.
> > +
> > +The application can then submit one or more requests to the the engine by
> > +using copy/paste instructions and pasting the CRBs to the virtual address
> > +(aka paste_address) returned by mmap(). User space can close the
> > +established connection or send window by closing the file descriptior
> > +(close(fd)) or upon the process exit.
> > +
> > +Note that applications can send several requests with the same window or
> > +can establish multiple windows, but one window for each file descriptor.
> > +
> > +Following sections provide additional details and references about the
> > +individual steps.
> > +
> > +NX-GZIP Device Node
> > +===================
> > +
> > +There is one /dev/crypto/nx-gzip node in the system and it provides
> > +access to all GZIP engines in the system. The only valid operations on
> > +/dev/crypto/nx-gzip are:
> > +
> > + * open() the device for read and write.
> > + * issue VAS_TX_WIN_OPEN ioctl
> > + * mmap() the engine's request queue into application's virtual
> > + address space (i.e. get a paste_address for the co-processor
> > + engine).
> > + * close the device node.
> > +
> > +Other file operations on this device node are undefined.
> > +
> > +Note that the copy and paste operations go directly to the hardware and
> > +do not go through this device. Refer COPY/PASTE document for more
> > +details.
> > +
> > +Although a system may have several instances of the NX co-processor
> > +engines (typically, one per P9 chip) there is just one
> > +/dev/crypto/nx-gzip device node in the system. When the nx-gzip device
> > +node is opened, Kernel opens send window on a suitable instance of NX
> > +accelerator. It finds CPU on which the user process is executing and
> > +determine the NX instance for the corresponding chip on which this CPU
> > +belongs.
> > +
> > +Applications may chose a specific instance of the NX co-processor using
> > +the vas_id field in the VAS_TX_WIN_OPEN ioctl as detailed below.
> > +
> > +A userspace library libnxz is available here but still in development:
> > + https://github.com/abalib/power-gzip
> > +
> > +Applications that use inflate / deflate calls can link with libnxz
> > +instead of libz and use NX GZIP compression without any modification.
> > +
> > +Open /dev/crypto/nx-gzip
> > +========================
> > +
> > +The nx-gzip device should be opened for read and write. No special
> > +privileges are needed to open the device. Each window coreesponds to one
>
> s/coreesponds/corresponds/
>
> > +file descriptor. So if the userspace process needs multiple windows,
> > +several open calls have to be issued.
> > +
> > +See open(2) system call man pages for other details such as return values,
> > +error codes and restrictions.
> > +codes and restrictions.
>
> You have 'codes and restrictions' twice here.
>
> > +
> > +VAS_TX_WIN_OPEN ioctl
> > +=====================
> > +
> > +Applications should use the VAS_TX_WIN_OPEN ioctl as follows to establish
> > +a connection with NX co-processor engine:
> > +
> > + ::
> > + struct vas_tx_win_open_attr {
> > + __u32 version;
> > + __s16 vas_id; /* specific instance of vas or -1
> > + for default */
> > + __u16 reserved1;
> > + __u64 flags; /* For future use */
> > + __u64 reserved2[6];
> > + };
> > +
> > + version: The version field must be currently set to 1.
> > + vas_id: If '-1' is passed, kernel will make a best-effort attempt
> > + to assign an optimal instance of NX for the process. To
> > + select the specific VAS instance, refer
> > + "Discovery of available VAS engines" section below.
> > +
> > + flags, reserved1 and reserved2[6] fields are for future extension
> > + and must be set to 0.
> > +
> > + The attributes attr for the VAS_TX_WIN_OPEN ioctl are defined as
> > + follows:
> > + #define VAS_MAGIC 'v'
> > + #define VAS_TX_WIN_OPEN _IOW(VAS_MAGIC, 1,
> > + struct vas_tx_win_open_attr)
> > +
> > + struct vas_tx_win_open_attr attr;
> > + rc = ioctl(fd, VAS_TX_WIN_OPEN, &attr);
> > +
> > + The VAS_TX_WIN_OPEN ioctl returns 0 on success. On errors, it
> > + returns -1 and sets the errno variable to indicate the error.
> > +
> > + Error conditions:
> > + EINVAL fd does not refer to a valid VAS device.
> > + EINVAL Invalid vas ID
> > + EINVAL version is not set with proper value
> > + EEXIST Window is already opened for the given fd
> > + ENOMEM Memory is not available to allocate window
> > + ENOSPC System has too many active windows (connections)
> > + opened
> > + EINVAL reserved fields are not set to 0.
> > +
> > + See the ioctl(2) man page for more details, error codes and
> > + restrictions.
> > +
> > +mmap() NX-GZIP device
> > +=====================
> > +
> > +The mmap() system call for a NX-GZIP device fd returns a paste_address
> > +that the application can use to copy/paste its CRB to the hardware engines.
> > + ::
> > +
> > + paste_addr = mmap(addr, size, prot, flags, fd, offset);
> > +
> > + Only restrictions on mmap for a NX-GZIP device fd are:
> > + * size should be 4K page size
>
> Patch 3 seems to allow a 64k page if the system is compiled for 64k
> pages... Should it restrict it to 4K?
Yes restricted to 4K for NX-GZIP
^ permalink raw reply
* Re: [PATCH v8 03/14] powerpc/vas: Define nx_fault_stamp in coprocessor_request_block
From: Nicholas Piggin @ 2020-03-23 0:30 UTC (permalink / raw)
To: Haren Myneni, mpe; +Cc: mikey, ajd, hch, oohall, sukadev, linuxppc-dev, herbert
In-Reply-To: <1584598437.9256.15247.camel@hbabu-laptop>
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
^ permalink raw reply
* Re: [PATCH v8 01/14] powerpc/xive: Define xive_native_alloc_irq_on_chip()
From: Nicholas Piggin @ 2020-03-23 0:20 UTC (permalink / raw)
To: Haren Myneni, mpe
Cc: mikey, ajd, hch, oohall, Cédric Le Goater, sukadev,
linuxppc-dev, herbert
In-Reply-To: <1584598352.9256.15242.camel@hbabu-laptop>
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
>
>
>
>
^ permalink raw reply
* Re: [PATCH V7 09/14] powerpc/vas: Update CSB and notify process for fault CRBs
From: Nicholas Piggin @ 2020-03-23 0:06 UTC (permalink / raw)
To: Haren Myneni, Michael Ellerman
Cc: mikey, ajd, hch, oohall, sukadev, linuxppc-dev, herbert
In-Reply-To: <1584473263.9256.14791.camel@hbabu-laptop>
Haren Myneni's on March 18, 2020 5:27 am:
> On Tue, 2020-03-17 at 16:28 +1100, Michael Ellerman wrote:
>> Haren Myneni <haren@linux.ibm.com> writes:
>> > 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 | 114 +++++++++++++++++++++++++++++
>> > 1 file changed, 114 insertions(+)
>> >
>> > diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c
>> > index 1c6d5cc..751ce48 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,118 @@
>> > #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)
>> > +{
>> > + int rc;
>> > + struct pid *pid;
>> > + void __user *csb_addr;
>> > + struct task_struct *tsk;
>> > + struct kernel_siginfo info;
>> > + struct coprocessor_status_block csb;
>>
>> csb is on the stack, and later copied to user, which is a risk for
>> creating an infoleak.
>>
>> Also please use reverse Christmas tree layout for your variables.
>>
>> > +
>> > + /*
>> > + * NX user space windows can not be opened for task->mm=NULL
>> > + * and faults will not be generated for kernel requests.
>> > + */
>> > + if (!window->mm || !window->user_win)
>> > + return;
>>
>> If that's a should-never-happen condition then should it do a
>> WARN_ON_ONCE() rather than silently returning?
>
> Will add WARN_ON
>
>>
>> > + csb_addr = (void __user *)be64_to_cpu(crb->csb_addr);
>> > +
>> > + 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;
>>
>> I'm pretty sure this has initialised all the fields of csb.
>>
>> But, I'd still be much happier if you zeroed the whole struct to begin
>> with, that way we know for sure we can't leak any uninitialised bytes to
>> userspace. It's only 16 bytes so it shouldn't add any noticeable
>> overhead.
> Sure, will initialize csb
>>
>> > +
>> > + 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;
>>
>> Similar question on WARN_ON_ONCE()
> Yes, we can add WARN_ON
>>
>> > + }
>> > +
>> > + /* Return if the task is exiting. */
>>
>> Why? Just because it's no use? It's racy isn't it, so it can't be for
>> correctness?
> Yes process is exiting and no need to update CSB. We release the
> task->usage refcount after copy_to_user().
>
>>
>> > + if (tsk->flags & PF_EXITING) {
>> > + put_task_struct(tsk);
>> > + return;
>> > + }
>> > +
>> > + use_mm(window->mm);
>>
>> There's no check that csb_addr is actually pointing into userspace, but
>> copy_to_user() does it for you.
>>
>> > + 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();
>>
>> You only need to order the stores above vs the store below to csb.flags.
>> So you should only need an smp_wmb() here.
> Sure, will add
> if (!rc) {
> csb.flags = CSB_V;
> smp_mb();
> rc = copy_to_user(csb_addr, &csb, sizeof(u8));
> }
>
>>
>> > + 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_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.
>> > + */
The code would read a bit better if this comment goes at the start of
this error handling process it describes (before the error message).
And I feel a bit hypocritical complaining about readability, but it
could possibly do with some work.
/*
* The application should have provided a valid mapping for the
* csb, and not unmap it before the csb.flags update, so the
* copy_to_user should not fail.
*
* If the application fails to follow this protocol, log a kernel
* error and send a SEGV to the pid. This signal may be ignored,
* so can't use force_sig_fault_to_task()
*/
Something like tthat?
>> > + rcu_read_lock();
>> > + rc = kill_pid_info(SIGSEGV, &info, pid);
>> > + rcu_read_unlock();
ipc/mqueue.c says kill_pid_info doesn't need rcu_read_lock(). AFAIKS
it's held around kill_pid_info in kernel/signal.c for the find_vpid().
Thanks,
Nick
>>
>> Shouldn't this be using force_sig_fault_to_task() or another helper,
>> rather than open-coding?
>
> Applications or nxz library can ignore this signal based on si_addr or
> take action like resend new request with valid csb_addr. Hence I did not
> use force_sig_info_to_task().
>
>>
>> > +
>> > + 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 +224,7 @@ irqreturn_t vas_fault_thread_fn(int irq, void *data)
>> > return IRQ_HANDLED;
>> > }
>> >
>> > + update_csb(window, crb);
>> > } while (true);
>> > }
>> >
>> > --
>> > 1.8.3.1
>>
>> cheers
>
>
>
^ permalink raw reply
* Re: [PATCH 18/15] kvm: Replace vcpu->swait with rcuwait
From: Peter Zijlstra @ 2020-03-22 22:32 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: rdunlap, linux-pci, bigeasy, linux-kernel, joel, will, mingo,
arnd, Davidlohr Bueso, torvalds, paulmck, linuxppc-dev, rostedt,
bhelgaas, kurt.schwemmer, kvalo, balbi, gregkh, linux-usb,
linux-wireless, oleg, tglx, netdev, Paolo Bonzini, logang, davem
In-Reply-To: <20200322163317.mh4sygr7xcjptmjp@linux-p48b>
On Sun, Mar 22, 2020 at 09:33:17AM -0700, Davidlohr Bueso wrote:
> On Fri, 20 Mar 2020, Peter Zijlstra wrote:
>
> > On Fri, Mar 20, 2020 at 01:55:26AM -0700, Davidlohr Bueso wrote:
> > > - swait_event_interruptible_exclusive(*wq, ((!vcpu->arch.power_off) &&
> > > - (!vcpu->arch.pause)));
> > > + rcuwait_wait_event(*wait,
> > > + (!vcpu->arch.power_off) && (!vcpu->arch.pause),
> > > + TASK_INTERRUPTIBLE);
> >
> > > - for (;;) {
> > > - prepare_to_swait_exclusive(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
> > > -
> > > - if (kvm_vcpu_check_block(vcpu) < 0)
> > > - break;
> > > -
> > > - waited = true;
> > > - schedule();
> > > - }
> > > -
> > > - finish_swait(&vcpu->wq, &wait);
> > > + rcuwait_wait_event(&vcpu->wait,
> > > + (block_check = kvm_vcpu_check_block(vcpu)) < 0,
> > > + TASK_INTERRUPTIBLE);
> >
> > Are these yet more instances that really want to be TASK_IDLE ?
>
> Hmm probably as it makes sense for a blocked vcpu not to be contributing to
> the loadavg. So if this is the only reason to use interruptible, then yes we
> ought to change it.
>
> However, I'll make this a separate patch, given this (ab)use isn't as obvious
> as the PS3 case, which is a kthread and therefore signals are masked.
The thing that was a dead give-away was that the return value of the
interruptible wait wasn't used.
^ permalink raw reply
* Re: [PATCH v3 3/9] powerpc/vas: Add VAS user space API
From: Haren Myneni @ 2020-03-22 20:50 UTC (permalink / raw)
To: Daniel Axtens
Cc: mikey, herbert, npiggin, linux-crypto, sukadev, linuxppc-dev
In-Reply-To: <87fte3w775.fsf@dja-thinkpad.axtens.net>
On Fri, 2020-03-20 at 23:18 +1100, Daniel Axtens wrote:
> Haren Myneni <haren@linux.ibm.com> writes:
>
> > On power9, userspace can send GZIP compression requests directly to NX
> > once kernel establishes NX channel / window with VAS. This patch provides
> > user space API which allows user space to establish channel using open
> > VAS_TX_WIN_OPEN ioctl, mmap and close operations.
> >
> > Each window corresponds to file descriptor and application can open
> > multiple windows. After the window is opened, VAS_TX_WIN_OPEN icoctl to
> > open a window on specific VAS instance, mmap() system call to map
> > the hardware address of engine's request queue into the application's
> > virtual address space.
> >
> > Then the application can then submit one or more requests to the the
> > engine by using the copy/paste instructions and pasting the CRBs to
> > the virtual address (aka paste_address) returned by mmap().
> >
> > Only NX GZIP coprocessor type is supported right now and allow GZIP
> > engine access via /dev/crypto/nx-gzip device node.
> >
> > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> > ---
> > arch/powerpc/include/asm/vas.h | 11 ++
> > arch/powerpc/platforms/powernv/Makefile | 2 +-
> > arch/powerpc/platforms/powernv/vas-api.c | 290 ++++++++++++++++++++++++++++
> > arch/powerpc/platforms/powernv/vas-window.c | 6 +-
> > arch/powerpc/platforms/powernv/vas.h | 2 +
> > 5 files changed, 307 insertions(+), 4 deletions(-)
> > create mode 100644 arch/powerpc/platforms/powernv/vas-api.c
> >
> > diff --git a/arch/powerpc/include/asm/vas.h b/arch/powerpc/include/asm/vas.h
> > index f93e6b0..e064953 100644
> > --- a/arch/powerpc/include/asm/vas.h
> > +++ b/arch/powerpc/include/asm/vas.h
> > @@ -163,4 +163,15 @@ struct vas_window *vas_tx_win_open(int vasid, enum vas_cop_type cop,
> > */
> > int vas_paste_crb(struct vas_window *win, int offset, bool re);
> >
> > +/*
> > + * Register / unregister coprocessor type to VAS API which will be exported
> > + * to user space. Applications can use this API to open / close window
> > + * which can be used to send / receive requests directly to cooprcessor.
> > + *
> > + * Only NX GZIP coprocessor type is supported now, but this API can be
> > + * used for others in future.
> > + */
> > +int vas_register_coproc_api(struct module *mod);
> > +void vas_unregister_coproc_api(void);
> > +
> > #endif /* __ASM_POWERPC_VAS_H */
> > diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
> > index 395789f..fe3f0fb 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 vas-fault.o
> > +obj-$(CONFIG_PPC_VAS) += vas.o vas-window.o vas-debug.o vas-fault.o vas-api.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-api.c b/arch/powerpc/platforms/powernv/vas-api.c
> > new file mode 100644
> > index 0000000..3473a4a
> > --- /dev/null
> > +++ b/arch/powerpc/platforms/powernv/vas-api.c
> > @@ -0,0 +1,290 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * VAS user space API for its accelerators (Only NX-GZIP is supported now)
> > + * Copyright (C) 2019 Haren Myneni, IBM Corp
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/device.h>
> > +#include <linux/cdev.h>
> > +#include <linux/fs.h>
> > +#include <linux/slab.h>
> > +#include <linux/uaccess.h>
> > +#include <asm/vas.h>
> > +#include <uapi/asm/vas-api.h>
> > +#include "vas.h"
> > +
> > +/*
> > + * The driver creates the device node that can be used as follows:
> > + * For NX-GZIP
> > + *
> > + * fd = open("/dev/crypto/nx-gzip", O_RDWR);
> > + * rc = ioctl(fd, VAS_TX_WIN_OPEN, &attr);
> > + * paste_addr = mmap(NULL, PAGE_SIZE, prot, MAP_SHARED, fd, 0ULL).
> > + * vas_copy(&crb, 0, 1);
> > + * vas_paste(paste_addr, 0, 1);
> > + * close(fd) or exit process to close window.
> > + *
> > + * where "vas_copy" and "vas_paste" are defined in copy-paste.h.
> > + * copy/paste returns to the user space directly. So refer NX hardware
> > + * documententation for excat copy/paste usage and completion / error
> > + * conditions.
> > + */
> > +
> > +static char *coproc_dev_name = "nx-gzip";
> > +static atomic_t coproc_instid = ATOMIC_INIT(0);
> > +
> > +/*
> > + * Wrapper object for the nx-gzip device - there is just one instance of
> > + * this node for the whole system.
> > + */
> > +static struct coproc_dev {
> > + struct cdev cdev;
> > + struct device *device;
> > + char *name;
> > + dev_t devt;
> > + struct class *class;
> > +} coproc_device;
> > +
> > +/*
> > + * One instance per open of a nx-gzip device. Each coproc_instance is
> > + * associated with a VAS window after the caller issues
> > + * VAS_GZIP_TX_WIN_OPEN ioctl.
> > + */
> > +struct coproc_instance {
> > + int id;
> > + struct vas_window *txwin;
> > +};
> > +
> > +static char *coproc_devnode(struct device *dev, umode_t *mode)
> > +{
> > + return kasprintf(GFP_KERNEL, "crypto/%s", dev_name(dev));
> > +}
> > +
> > +static int coproc_open(struct inode *inode, struct file *fp)
> > +{
> > + struct coproc_instance *instance;
> > +
> > + instance = kzalloc(sizeof(*instance), GFP_KERNEL);
> > + if (!instance)
> > + return -ENOMEM;
> > +
> > + instance->id = atomic_inc_return(&coproc_instid);
>
> I don't understand what this instance->id field does - I can't find any
> other uses of it in these series.
>
> I'm also not sure that this gives you a unique id - you increment it
> here and decrement it in coproc_release, but I'm not sure what prevents
> the same ID being given to multiple instances, e.g. the following
> sequence
>
> coproc_open(inode, file A) -> instance with id 0, coproc_instid = 1
> coproc_open(inode, file B) -> instance with id 1, coproc_instid = 2
> coproc_release(inode, file A) -> release id 0, coproc_instid = 1
> coproc_open(inode, file C) -> instance with id 1, coproc_instid = 2
>
> File B and C both have ID = 1, unless I'm misunderstanding something.
Thanks for your comments.
coproc_instid is not needed. Sorry My mistake, it was added in prototype
code, but forgot to remove. Added to determine how many windows are
active, but not needed now.
I will repost the patch with this change.
>
> > +
> > + fp->private_data = instance;
> > + return 0;
> > +}
> > +
> > +static int coproc_ioc_tx_win_open(struct file *fp, unsigned long arg)
> > +{
> > + int rc, vasid;
> > + struct vas_tx_win_attr txattr;
> > + struct vas_tx_win_open_attr uattr;
> > + void __user *uptr = (void __user *)arg;
> > + struct vas_window *txwin;
> > + struct coproc_instance *nxti = fp->private_data;
> > +
> > + if (!nxti)
> > + return -EINVAL;
> > +
> > + /*
> > + * One window for file descriptor
> > + */
> > + if (nxti->txwin)
> > + return -EEXIST;
> > +
> > + rc = copy_from_user(&uattr, uptr, sizeof(uattr));
> > + if (rc) {
> > + pr_err("%s(): copy_from_user() returns %d\n", __func__, rc);
> > + return -EFAULT;
> > + }
> > +
> > + if (uattr.version != 1) {
> > + pr_err("Invalid version\n");
> > + return -EINVAL;
> > + }
> > +
> > + vasid = uattr.vas_id;
> > +
> > + memset(&txattr, 0, sizeof(struct vas_tx_win_attr));
>
> You could define txattr with `struct vas_tx_win_attr txattr = {};` and
> avoid the explicit memset.
>
> > + vas_init_tx_win_attr(&txattr, VAS_COP_TYPE_GZIP);
> > +
> > + txattr.lpid = mfspr(SPRN_LPID);
> > + txattr.pidr = mfspr(SPRN_PID);
> > + txattr.user_win = true;
> > + txattr.rsvd_txbuf_count = false;
> > + txattr.pswid = false;
> > + /*
> > + * txattr.wcreds_max is set to VAS_WCREDS_DEFAULT (1024) in
> > + * vas-window.c, but can be changed specific to GZIP depends
> > + * on user space need.
> > + * If needed to set txattr.wcreds_max here.
> > + */
>
> Who could set this? You mention userspace need but it looks like the
> user cannot set this. Is this a message to future kernel developers?
set the default value in vax_tx_win_open() (vas_window.c) if it is not
set here. credits limit can set here if we decide to use non-default
value for user space windows. Not allowing user space to set this value.
Yes, this comment is for kernel developers.
I can remove this comment to remove the confusion.
>
> > +
> > + pr_devel("Pid %d: Opening txwin, PIDR %ld\n", txattr.pidr,
> > + mfspr(SPRN_PID));
> > +
> > + txwin = vas_tx_win_open(vasid, VAS_COP_TYPE_GZIP, &txattr);
> > + if (IS_ERR(txwin)) {
> > + pr_err("%s() vas_tx_win_open() failed, %ld\n", __func__,
> > + PTR_ERR(txwin));
> > + return PTR_ERR(txwin);
> > + }
> > +
> > + nxti->txwin = txwin;
> > +
> > + return 0;
> > +}
> > +
> > +static int coproc_release(struct inode *inode, struct file *fp)
> > +{
> > + struct coproc_instance *instance;
> > +
> > + instance = fp->private_data;
> > +
> > + if (instance && instance->txwin) {
> > + vas_win_close(instance->txwin);
> > + instance->txwin = NULL;
> > + }
> > +
> > + /*
> > + * We don't know here if user has other receive windows
> > + * open, so we can't really call clear_thread_tidr().
> > + * So, once the process calls set_thread_tidr(), the
> > + * TIDR value sticks around until process exits, resulting
> > + * in an extra copy in restore_sprs().
> > + */
> > +
> > + kfree(instance);
> > + fp->private_data = NULL;
> > + atomic_dec(&coproc_instid);
> > +
> > + return 0;
> > +}
> > +
> > +static int coproc_mmap(struct file *fp, struct vm_area_struct *vma)
> > +{
> > + int rc;
> > + pgprot_t prot;
> > + u64 paste_addr;
> > + unsigned long pfn;
> > + struct coproc_instance *instance = fp->private_data;
> > +
> > + if ((vma->vm_end - vma->vm_start) > PAGE_SIZE) {
> > + pr_debug("%s(): size 0x%zx, PAGE_SIZE 0x%zx\n", __func__,
> > + (vma->vm_end - vma->vm_start), PAGE_SIZE);
> > + return -EINVAL;
> > + }
> > +
> > + /* Ensure instance has an open send window */
> > + if (!instance->txwin) {
> > + pr_err("%s(): No send window open?\n", __func__);
> > + return -EINVAL;
> > + }
> > +
> > + vas_win_paste_addr(instance->txwin, &paste_addr, NULL);
> > + pfn = paste_addr >> PAGE_SHIFT;
> > +
> > + /* flags, page_prot from cxl_mmap(), except we want cachable */
> > + vma->vm_flags |= VM_IO | VM_PFNMAP;
> > + vma->vm_page_prot = pgprot_cached(vma->vm_page_prot);
> > +
> > + prot = __pgprot(pgprot_val(vma->vm_page_prot) | _PAGE_DIRTY);
> > +
> > + rc = remap_pfn_range(vma, vma->vm_start, pfn + vma->vm_pgoff,
> > + vma->vm_end - vma->vm_start, prot);
> > +
> > + pr_devel("%s(): paste addr %llx at %lx, rc %d\n", __func__,
> > + paste_addr, vma->vm_start, rc);
> > +
> > + return rc;
> > +}
> > +
> > +static long coproc_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
> > +{
> > + switch (cmd) {
> > + case VAS_TX_WIN_OPEN:
> > + return coproc_ioc_tx_win_open(fp, arg);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static struct file_operations coproc_fops = {
> > + .open = coproc_open,
> > + .release = coproc_release,
> > + .mmap = coproc_mmap,
> > + .unlocked_ioctl = coproc_ioctl,
> > +};
> > +
> > +/*
> > + * Supporting only nx-gzip coprocessor type now, but this API code
> > + * extended to other coprocessor types later.
> > + */
> > +int vas_register_coproc_api(struct module *mod)
> > +{
> > + int rc = -EINVAL;
> > + dev_t devno;
> > +
> > + rc = alloc_chrdev_region(&coproc_device.devt, 1, 1, "nx-gzip");
> > + if (rc) {
> > + pr_err("Unable to allocate coproc major number: %i\n", rc);
> > + return rc;
> > + }
> > +
> > + pr_devel("NX-GZIP device allocated, dev [%i,%i]\n",
> > + MAJOR(coproc_device.devt), MINOR(coproc_device.devt));
> > +
> > + coproc_device.class = class_create(mod, "nx-gzip");
> > + if (IS_ERR(coproc_device.class)) {
> > + rc = PTR_ERR(coproc_device.class);
> > + pr_err("Unable to create NX-GZIP class %d\n", rc);
> > + goto err_class;
> > + }
> > + coproc_device.class->devnode = coproc_devnode;
> > +
> > + coproc_fops.owner = mod;
> > + cdev_init(&coproc_device.cdev, &coproc_fops);
>
> Looking into this coproc_fops thing more:
>
> I find this API very confusing. The comment at the top of the function
> says it will be extended, but there's only one coproc_fops, so currently
> it can only be instantiated once and owned by one module. Much of the
> rest of that function is also very much based around the nx-gzip
> coprocessor.
>
> I'm not fully certain about how this should work, but I think probably
> it either needs to be fully generic or fully nx-gzip only for now. I
> would make it fully nx-gzip only and extend it later, but I'm not fussy.
We are adding only for NX-GZIP right now and mentioned nx-gzip. Most of
interfaces in coproc_fops such as open/mmap/close should be common for
any coprocs except ioctl or new ioctl cmd.
register_coproc_api creates device node and register fs API specific to
coprocessor. user space use fs API to establish communication channel
(using open, ioctl, mmap) to NX.
How about defining coproc_nxgzip_fs to remove this confusion.
>
> > +
> > + devno = MKDEV(MAJOR(coproc_device.devt), 0);
> > + rc = cdev_add(&coproc_device.cdev, devno, 1);
> > + if (rc) {
> > + pr_err("cdev_add() failed %d\n", rc);
> > + goto err_cdev;
> > + }
> > +
> > + coproc_device.device = device_create(coproc_device.class, NULL,
> > + devno, NULL, coproc_dev_name, MINOR(devno));
> > + if (IS_ERR(coproc_device.device)) {
> > + rc = PTR_ERR(coproc_device.device);
> > + pr_err("Unable to create coproc-%d %d\n", MINOR(devno), rc);
> > + goto err;
> > + }
> > +
> > + pr_devel("%s: Added dev [%d,%d]\n", __func__, MAJOR(devno),
> > + MINOR(devno));
> > +
> > + return 0;
> > +
> > +err:
> > + cdev_del(&coproc_device.cdev);
> > +err_cdev:
> > + class_destroy(coproc_device.class);
> > +err_class:
> > + unregister_chrdev_region(coproc_device.devt, 1);
> > + return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(vas_register_coproc_api);
> > +
> > +void vas_unregister_coproc_api(void)
> > +{
> > + dev_t devno;
> > +
> > + cdev_del(&coproc_device.cdev);
> > + devno = MKDEV(MAJOR(coproc_device.devt), 0);
> > + device_destroy(coproc_device.class, devno);
> > +
> > + class_destroy(coproc_device.class);
> > + unregister_chrdev_region(coproc_device.devt, 1);
> > +}
> > +EXPORT_SYMBOL_GPL(vas_unregister_coproc_api);
> > diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
> > index e9ab851..7484296 100644
> > --- a/arch/powerpc/platforms/powernv/vas-window.c
> > +++ b/arch/powerpc/platforms/powernv/vas-window.c
> > @@ -26,7 +26,7 @@
> > * Compute the paste address region for the window @window using the
> > * ->paste_base_addr and ->paste_win_id_shift we got from device tree.
> > */
> > -static void compute_paste_address(struct vas_window *window, u64 *addr, int *len)
> > +void vas_win_paste_addr(struct vas_window *window, u64 *addr, int *len)
> > {
> > int winid;
> > u64 base, shift;
> > @@ -80,7 +80,7 @@ static void *map_paste_region(struct vas_window *txwin)
> > goto free_name;
> >
> > txwin->paste_addr_name = name;
> > - compute_paste_address(txwin, &start, &len);
> > + vas_win_paste_addr(txwin, &start, &len);
> >
> > if (!request_mem_region(start, len, name)) {
> > pr_devel("%s(): request_mem_region(0x%llx, %d) failed\n",
> > @@ -138,7 +138,7 @@ static void unmap_paste_region(struct vas_window *window)
> > u64 busaddr_start;
> >
> > if (window->paste_kaddr) {
> > - compute_paste_address(window, &busaddr_start, &len);
> > + vas_win_paste_addr(window, &busaddr_start, &len);
> > unmap_region(window->paste_kaddr, busaddr_start, len);
> > window->paste_kaddr = NULL;
> > kfree(window->paste_addr_name);
> > diff --git a/arch/powerpc/platforms/powernv/vas.h b/arch/powerpc/platforms/powernv/vas.h
> > index 8c39a7d..a10abed 100644
> > --- a/arch/powerpc/platforms/powernv/vas.h
> > +++ b/arch/powerpc/platforms/powernv/vas.h
> > @@ -431,6 +431,8 @@ struct vas_winctx {
> > 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);
> > +extern void vas_win_paste_addr(struct vas_window *window, u64 *addr,
> > + int *len);
> >
> > static inline int vas_window_pid(struct vas_window *window)
> > {
> > --
> > 1.8.3.1
^ permalink raw reply
* Re: [PATCH 18/15] kvm: Replace vcpu->swait with rcuwait
From: Davidlohr Bueso @ 2020-03-22 16:33 UTC (permalink / raw)
To: Peter Zijlstra
Cc: rdunlap, linux-pci, bigeasy, linux-kernel, joel, will, mingo,
arnd, Davidlohr Bueso, torvalds, paulmck, linuxppc-dev, rostedt,
bhelgaas, kurt.schwemmer, kvalo, balbi, gregkh, linux-usb,
linux-wireless, oleg, tglx, netdev, Paolo Bonzini, logang, davem
In-Reply-To: <20200320125455.GE20696@hirez.programming.kicks-ass.net>
On Fri, 20 Mar 2020, Peter Zijlstra wrote:
>On Fri, Mar 20, 2020 at 01:55:26AM -0700, Davidlohr Bueso wrote:
>> - swait_event_interruptible_exclusive(*wq, ((!vcpu->arch.power_off) &&
>> - (!vcpu->arch.pause)));
>> + rcuwait_wait_event(*wait,
>> + (!vcpu->arch.power_off) && (!vcpu->arch.pause),
>> + TASK_INTERRUPTIBLE);
>
>> - for (;;) {
>> - prepare_to_swait_exclusive(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
>> -
>> - if (kvm_vcpu_check_block(vcpu) < 0)
>> - break;
>> -
>> - waited = true;
>> - schedule();
>> - }
>> -
>> - finish_swait(&vcpu->wq, &wait);
>> + rcuwait_wait_event(&vcpu->wait,
>> + (block_check = kvm_vcpu_check_block(vcpu)) < 0,
>> + TASK_INTERRUPTIBLE);
>
>Are these yet more instances that really want to be TASK_IDLE ?
Hmm probably as it makes sense for a blocked vcpu not to be contributing to
the loadavg. So if this is the only reason to use interruptible, then yes we
ought to change it.
However, I'll make this a separate patch, given this (ab)use isn't as obvious
as the PS3 case, which is a kthread and therefore signals are masked.
Thanks,
Davidlohr
^ permalink raw reply
* Re: [PATCH v3] powerpc/kprobes: Ignore traps that happened in real mode
From: Naveen N. Rao @ 2020-03-22 15:54 UTC (permalink / raw)
To: Christophe Leroy, Michael Ellerman; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <f28a0219-abf1-07d4-b98b-b19db4af0f12@c-s.fr>
Christophe Leroy wrote:
> ping
>
>
> Le 18/02/2020 à 20:38, Christophe Leroy a écrit :
>> When a program check exception happens while MMU translation is
>> disabled, following Oops happens in kprobe_handler() in the following
>> code:
>
> Michael, we have several traps in assembly while MMU is still disabled
> (TRACE_IRQFLAGS, KUAP DEBUG, syscall from kernel, machine check in RTAS,
> ...).
> Without this fix, all of them trigger an Oops when CONFIG_KPROBE is set.
For this patch:
Reviewed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
- Naveen
^ permalink raw reply
* Re: [PATCH] powerpc/64: ftrace don't trace real mode
From: Naveen N. Rao @ 2020-03-22 16:17 UTC (permalink / raw)
To: linuxppc-dev, Nicholas Piggin
In-Reply-To: <1584759479.7ueu8qvhgs.astroid@bobo.none>
Nicholas Piggin wrote:
> Naveen N. Rao's on March 21, 2020 4:39 am:
>> Hi Nick,
>>
>> Nicholas Piggin wrote:
>>> This warns and prevents tracing attempted in a real-mode context.
>>
>> Is this something you're seeing often? Last time we looked at this, KVM
>> was the biggest offender and we introduced paca->ftrace_enabled as a way
>> to disable ftrace while in KVM code.
>
> Not often but it has a tendancy to blow up the least tested code at the
> worst times :)
>
> Machine check is bad, I'm sure HMI too but I haven't tested that yet.
>
> I've fixed up most of it with annotations, this is obviously an extra
> safety not something to rely on like ftrace_enabled. Probably even the
> WARN_ON here is dangerous here, but I don't want to leave these bugs
> in there.
Ok, makes sense.
>
> Although the machine check and hmi code touch a fair bit of stuff and
> annotating is a bit fragile. It might actually be better if the
> paca->ftrace_enabled could be a nesting counter, then we could use it
> in machine checks too and avoid a lot of annotations.
I'm not too familiar with MC/HMI, but I suppose those aren't re-entrant?
If those have access to an emergency stack, can we save/restore
ftrace_enabled state across the handlers?
We're primarily disabling ftrace across idle/offline/KVM right now. I'm
not sure if nesting is useful there.
>
>> While this is cheap when handling ftrace_regs_caller() as done in this
>> patch, for simple function tracing (see below), we will have to grab the
>> MSR which will slow things down slightly.
>
> mfmsr is not too bad these days.
That's great!
Thanks,
Naveen
^ permalink raw reply
* [PATCH v3] powerpc/pseries: Handle UE event for memcpy_mcsafe
From: Ganesh Goudar @ 2020-03-22 16:05 UTC (permalink / raw)
To: mpe, linuxppc-dev; +Cc: mahesh, santosh, Ganesh Goudar
If we hit UE at an instruction with a fixup entry, flag to
ignore the event and set nip to continue execution at the
fixup entry.
For powernv these changes are already made by
commit 895e3dceeb97 ("powerpc/mce: Handle UE event for memcpy_mcsafe")
Reviewed-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
Reviewed-by: Santosh S <santosh@fossix.org>
Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
---
V2: Fixes a trivial checkpatch error in commit msg.
V3: Use proper subject prefix.
---
arch/powerpc/platforms/pseries/ras.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
index 43710b69e09e..58e2483fbb1a 100644
--- a/arch/powerpc/platforms/pseries/ras.c
+++ b/arch/powerpc/platforms/pseries/ras.c
@@ -10,6 +10,7 @@
#include <linux/fs.h>
#include <linux/reboot.h>
#include <linux/irq_work.h>
+#include <linux/extable.h>
#include <asm/machdep.h>
#include <asm/rtas.h>
@@ -505,6 +506,7 @@ static int mce_handle_error(struct pt_regs *regs, struct rtas_error_log *errp)
int initiator = rtas_error_initiator(errp);
int severity = rtas_error_severity(errp);
u8 error_type, err_sub_type;
+ const struct exception_table_entry *entry;
if (initiator == RTAS_INITIATOR_UNKNOWN)
mce_err.initiator = MCE_INITIATOR_UNKNOWN;
@@ -558,6 +560,12 @@ static int mce_handle_error(struct pt_regs *regs, struct rtas_error_log *errp)
switch (mce_log->error_type) {
case MC_ERROR_TYPE_UE:
mce_err.error_type = MCE_ERROR_TYPE_UE;
+ entry = search_kernel_exception_table(regs->nip);
+ if (entry) {
+ mce_err.ignore_event = true;
+ regs->nip = extable_fixup(entry);
+ disposition = RTAS_DISP_FULLY_RECOVERED;
+ }
switch (err_sub_type) {
case MC_ERROR_UE_IFETCH:
mce_err.u.ue_error_type = MCE_UE_ERROR_IFETCH;
--
2.17.2
^ permalink raw reply related
* Re: [patch V3 04/20] orinoco_usb: Use the regular completion interfaces
From: Kalle Valo @ 2020-03-22 14:42 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Randy Dunlap, linux-ia64, Peter Zijlstra, linux-pci,
Sebastian Siewior, tel.com, Oleg Nesterov, Guo Ren,
Joel Fernandes, Vincent Chen, Ingo Molnar, Jonathan Corbet,
Davidlohr Bueso, Paul E . McKenney, Brian Cain, linux-acpi,
linux-hexagon, Rafael J. Wysocki, linux-csky, Linus Torvalds,
Darren Hart, Zhang Rui, Len Brown, Fenghua Yu, Arnd Bergmann,
linux-pm, kbuild test robot, linuxppc-dev, Greentime Hu,
Bjorn Helgaas, Kurt Schwemmer, platform-driver-x86, Felipe Balbi,
Michal Simek, Tony Luck, Nick Hu, Geoff Levand,
Greg Kroah-Hartman, linux-usb, linux-wireless, LKML,
Davidlohr Bueso, netdev, Logan Gunthorpe, David S. Miller,
Andy Shevchenko
In-Reply-To: <20200321113241.150783464@linutronix.de>
Thomas Gleixner <tglx@linutronix.de> writes:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> The completion usage in this driver is interesting:
>
> - it uses a magic complete function which according to the comment was
> implemented by invoking complete() four times in a row because
> complete_all() was not exported at that time.
>
> - it uses an open coded wait/poll which checks completion:done. Only one wait
> side (device removal) uses the regular wait_for_completion() interface.
>
> The rationale behind this is to prevent that wait_for_completion() consumes
> completion::done which would prevent that all waiters are woken. This is not
> necessary with complete_all() as that sets completion::done to UINT_MAX which
> is left unmodified by the woken waiters.
>
> Replace the magic complete function with complete_all() and convert the
> open coded wait/poll to regular completion interfaces.
>
> This changes the wait to exclusive wait mode. But that does not make any
> difference because the wakers use complete_all() which ignores the
> exclusive mode.
>
> Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Kalle Valo <kvalo@codeaurora.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-wireless@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: linux-usb@vger.kernel.org
> ---
> V2: New patch to avoid conversion to swait functions later.
> ---
> drivers/net/wireless/intersil/orinoco/orinoco_usb.c | 21 ++++----------------
> 1 file changed, 5 insertions(+), 16 deletions(-)
I assume this is going via some other than wireless-drivers so:
Acked-by: Kalle Valo <kvalo@codeaurora.org>
--
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH v5 1/7] ASoC: dt-bindings: fsl_asrc: Add new property fsl, asrc-format
From: Shengjiu Wang @ 2020-03-22 10:47 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux-ALSA, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang,
Takashi Iwai, Liam Girdwood, Nicolin Chen, Mark Brown,
linuxppc-dev, linux-kernel
In-Reply-To: <20200320173213.GA9093@bogus>
On Sat, Mar 21, 2020 at 1:34 AM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Mar 09, 2020 at 02:19:44PM -0700, Nicolin Chen wrote:
> > On Mon, Mar 09, 2020 at 11:58:28AM +0800, Shengjiu Wang wrote:
> > > In order to support new EASRC and simplify the code structure,
> > > We decide to share the common structure between them. This bring
> > > a problem that EASRC accept format directly from devicetree, but
> > > ASRC accept width from devicetree.
> > >
> > > In order to align with new ESARC, we add new property fsl,asrc-format.
> > > The fsl,asrc-format can replace the fsl,asrc-width, then driver
> > > can accept format from devicetree, don't need to convert it to
> > > format through width.
> > >
> > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > ---
> > > Documentation/devicetree/bindings/sound/fsl,asrc.txt | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/sound/fsl,asrc.txt b/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > index cb9a25165503..780455cf7f71 100644
> > > --- a/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > +++ b/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > @@ -51,6 +51,11 @@ Optional properties:
> > > will be in use as default. Otherwise, the big endian
> > > mode will be in use for all the device registers.
> > >
> > > + - fsl,asrc-format : Defines a mutual sample format used by DPCM Back
> > > + Ends, which can replace the fsl,asrc-width.
> > > + The value is SNDRV_PCM_FORMAT_S16_LE, or
> > > + SNDRV_PCM_FORMAT_S24_LE
> >
> > I am still holding the concern at the DT binding of this format,
> > as it uses values from ASoC header file instead of a dt-binding
> > header file -- not sure if we can do this. Let's wait for Rob's
> > comments.
>
> I assume those are an ABI as well, so it's okay to copy them unless we
> already have some format definitions for DT. But it does need to be copy
> in a header under include/dt-bindings/.
Thanks for reviewing. seems it is not a good time to add a new header
file in include/dt-bindings/ in this patch serial. I will drop this change
this time, that still using the "fsl,asrc-width".
best regards
wang shengjiu
^ permalink raw reply
* Re: [PATCH v3] powerpc/kprobes: Ignore traps that happened in real mode
From: Christophe Leroy @ 2020-03-22 10:00 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <424331e2006e7291a1bfe40e7f3fa58825f565e1.1582054578.git.christophe.leroy@c-s.fr>
ping
Le 18/02/2020 à 20:38, Christophe Leroy a écrit :
> When a program check exception happens while MMU translation is
> disabled, following Oops happens in kprobe_handler() in the following
> code:
Michael, we have several traps in assembly while MMU is still disabled
(TRACE_IRQFLAGS, KUAP DEBUG, syscall from kernel, machine check in RTAS,
...).
Without this fix, all of them trigger an Oops when CONFIG_KPROBE is set.
Christophe
>
> } else if (*addr != BREAKPOINT_INSTRUCTION) {
>
> [ 33.098554] BUG: Unable to handle kernel data access on read at 0x0000e268
> [ 33.105091] Faulting instruction address: 0xc000ec34
> [ 33.110010] Oops: Kernel access of bad area, sig: 11 [#1]
> [ 33.115348] BE PAGE_SIZE=16K PREEMPT CMPC885
> [ 33.119540] Modules linked in:
> [ 33.122591] CPU: 0 PID: 429 Comm: cat Not tainted 5.6.0-rc1-s3k-dev-00824-g84195dc6c58a #3267
> [ 33.131005] NIP: c000ec34 LR: c000ecd8 CTR: c019cab8
> [ 33.136002] REGS: ca4d3b58 TRAP: 0300 Not tainted (5.6.0-rc1-s3k-dev-00824-g84195dc6c58a)
> [ 33.144324] MSR: 00001032 <ME,IR,DR,RI> CR: 2a4d3c52 XER: 00000000
> [ 33.150699] DAR: 0000e268 DSISR: c0000000
> [ 33.150699] GPR00: c000b09c ca4d3c10 c66d0620 00000000 ca4d3c60 00000000 00009032 00000000
> [ 33.150699] GPR08: 00020000 00000000 c087de44 c000afe0 c66d0ad0 100d3dd6 fffffff3 00000000
> [ 33.150699] GPR16: 00000000 00000041 00000000 ca4d3d70 00000000 00000000 0000416d 00000000
> [ 33.150699] GPR24: 00000004 c53b6128 00000000 0000e268 00000000 c07c0000 c07bb6fc ca4d3c60
> [ 33.188015] NIP [c000ec34] kprobe_handler+0x128/0x290
> [ 33.192989] LR [c000ecd8] kprobe_handler+0x1cc/0x290
> [ 33.197854] Call Trace:
> [ 33.200340] [ca4d3c30] [c000b09c] program_check_exception+0xbc/0x6fc
> [ 33.206590] [ca4d3c50] [c000e43c] ret_from_except_full+0x0/0x4
> [ 33.212392] --- interrupt: 700 at 0xe268
> [ 33.270401] Instruction dump:
> [ 33.273335] 913e0008 81220000 38600001 3929ffff 91220000 80010024 bb410008 7c0803a6
> [ 33.280992] 38210020 4e800020 38600000 4e800020 <813b0000> 6d2a7fe0 2f8a0008 419e0154
> [ 33.288841] ---[ end trace 5b9152d4cdadd06d ]---
>
> kprobe is not prepared to handle events in real mode and functions
> running in real mode should have been blacklisted, so kprobe_handler()
> can safely bail out telling 'this trap is not mine' for any trap that
> happened while in real-mode.
>
> If the trap happened with MSR_IR or MSR_DR cleared, return 0 immediately.
>
> Reported-by: Larry Finger <Larry.Finger@lwfinger.net>
> Fixes: 6cc89bad60a6 ("powerpc/kprobes: Invoke handlers directly")
> Cc: stable@vger.kernel.org
> Cc: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>
> ---
> v3: Also bail out if MSR_DR is cleared.
>
> Resending v2 with a more appropriate name
>
> v2: bailing out instead of converting real-time address to virtual and continuing.
>
> The bug might have existed even before that commit from Naveen.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/kernel/kprobes.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 2d27ec4feee4..9b340af02c38 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -264,6 +264,9 @@ int kprobe_handler(struct pt_regs *regs)
> if (user_mode(regs))
> return 0;
>
> + if (!(regs->msr & MSR_IR) || !(regs->msr & MSR_DR))
> + return 0;
> +
> /*
> * We don't want to be preempted for the entire
> * duration of kprobe processing
>
^ permalink raw reply
* Re: [PATCH v5 6/7] ASoC: dt-bindings: fsl_easrc: Add document for EASRC
From: Shengjiu Wang @ 2020-03-22 8:02 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux-ALSA, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang,
Takashi Iwai, Liam Girdwood, Nicolin Chen, Mark Brown,
linuxppc-dev, linux-kernel
In-Reply-To: <20200320174812.GA27070@bogus>
On Sat, Mar 21, 2020 at 1:50 AM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Mar 09, 2020 at 11:58:33AM +0800, Shengjiu Wang wrote:
> > EASRC (Enhanced Asynchronous Sample Rate Converter) is a new
> > IP module found on i.MX8MN.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> > .../devicetree/bindings/sound/fsl,easrc.yaml | 101 ++++++++++++++++++
> > 1 file changed, 101 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/sound/fsl,easrc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/sound/fsl,easrc.yaml b/Documentation/devicetree/bindings/sound/fsl,easrc.yaml
> > new file mode 100644
> > index 000000000000..ff22f8056a63
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/fsl,easrc.yaml
> > @@ -0,0 +1,101 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/fsl,easrc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP Asynchronous Sample Rate Converter (ASRC) Controller
> > +
> > +maintainers:
> > + - Shengjiu Wang <shengjiu.wang@nxp.com>
> > +
> > +properties:
> > + $nodename:
> > + pattern: "^easrc@.*"
> > +
> > + compatible:
> > + const: fsl,imx8mn-easrc
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + clocks:
> > + items:
> > + - description: Peripheral clock
> > +
> > + clock-names:
> > + items:
> > + - const: mem
> > +
> > + dmas:
> > + maxItems: 8
> > +
> > + dma-names:
> > + items:
> > + - const: ctx0_rx
> > + - const: ctx0_tx
> > + - const: ctx1_rx
> > + - const: ctx1_tx
> > + - const: ctx2_rx
> > + - const: ctx2_tx
> > + - const: ctx3_rx
> > + - const: ctx3_tx
> > +
> > + fsl,easrc-ram-script-name:
>
> 'firmware-name' is the established property name for this.
will use "firmware-name"
>
> > + allOf:
> > + - $ref: /schemas/types.yaml#/definitions/string
> > + - const: imx/easrc/easrc-imx8mn.bin
>
> Though if there's only 1 possible value, why does this need to be in DT?
>
> > + description: The coefficient table for the filters
>
> If the firmware is only 1 thing, then perhaps this should just be a DT
> property rather than a separate file. It depends on who owns/creates
> this file. If fixed for the platform, then DT is a good fit. If updated
> separately from DT and boot firmware, then keeping it separate makes
> sense.
>
The firmware is not fixed for the platform, it is updated separately from
DT. So we can keep it separately.
best regards
wang shengjiu
^ permalink raw reply
* Re: [patch V3 05/20] acpi: Remove header dependency
From: Rafael J. Wysocki @ 2020-03-22 7:02 UTC (permalink / raw)
To: Thomas Gleixner
Cc: open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:, linux-ia64,
Peter Zijlstra, Linux PCI, Sebastian Siewior, Oleg Nesterov,
Guo Ren, Joel Fernandes, Vincent Chen, Ingo Molnar,
Davidlohr Bueso, kbuild test robot, Brian Cain, Jonathan Corbet,
Paul E . McKenney, linux-hexagon, Rafael J. Wysocki, linux-csky,
ACPI Devel Maling List, Darren Hart, Zhang Rui, Len Brown,
Fenghua Yu, Randy Dunlap, Arnd Bergmann, Linux PM, linuxppc-dev,
Greentime Hu, Bjorn Helgaas, Kurt Schwemmer, Platform Driver,
Kalle Valo, Felipe Balbi, Michal Simek, Tony Luck, Nick Hu,
Geoff Levand, Greg Kroah-Hartman, Linus Torvalds,
open list:NETWORKING DRIVERS (WIRELESS), LKML, Davidlohr Bueso,
netdev, Logan Gunthorpe, David S. Miller, Andy Shevchenko
In-Reply-To: <20200321113241.246190285@linutronix.de>
On Sat, Mar 21, 2020 at 12:35 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> From: Peter Zijlstra <peterz@infradead.org>
>
> In order to avoid future header hell, remove the inclusion of
> proc_fs.h from acpi_bus.h. All it needs is a forward declaration of a
> struct.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Darren Hart <dvhart@infradead.org>
> Cc: Andy Shevchenko <andy@infradead.org>
> Cc: platform-driver-x86@vger.kernel.org
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> Cc: linux-pm@vger.kernel.org
> Cc: Len Brown <lenb@kernel.org>
> Cc: linux-acpi@vger.kernel.org
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/platform/x86/dell-smo8800.c | 1 +
> drivers/platform/x86/wmi.c | 1 +
> drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c | 1 +
> include/acpi/acpi_bus.h | 2 +-
> 4 files changed, 4 insertions(+), 1 deletion(-)
>
> --- a/drivers/platform/x86/dell-smo8800.c
> +++ b/drivers/platform/x86/dell-smo8800.c
> @@ -16,6 +16,7 @@
> #include <linux/interrupt.h>
> #include <linux/miscdevice.h>
> #include <linux/uaccess.h>
> +#include <linux/fs.h>
>
> struct smo8800_device {
> u32 irq; /* acpi device irq */
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -29,6 +29,7 @@
> #include <linux/uaccess.h>
> #include <linux/uuid.h>
> #include <linux/wmi.h>
> +#include <linux/fs.h>
> #include <uapi/linux/wmi.h>
>
> ACPI_MODULE_NAME("wmi");
> --- a/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c
> +++ b/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c
> @@ -19,6 +19,7 @@
> #include <linux/acpi.h>
> #include <linux/uaccess.h>
> #include <linux/miscdevice.h>
> +#include <linux/fs.h>
> #include "acpi_thermal_rel.h"
>
> static acpi_handle acpi_thermal_rel_handle;
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -80,7 +80,7 @@ bool acpi_dev_present(const char *hid, c
>
> #ifdef CONFIG_ACPI
>
> -#include <linux/proc_fs.h>
> +struct proc_dir_entry;
>
> #define ACPI_BUS_FILE_ROOT "acpi"
> extern struct proc_dir_entry *acpi_root_dir;
>
>
^ permalink raw reply
* Re: [PATCH] Documentation: Clarify better about the rwsem non-owner release issue
From: Kalle Valo @ 2020-03-22 6:51 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-doc, Peter Zijlstra, Sebastian Andrzej Siewior,
linux-kernel, netdev, Will Deacon, Thomas Gleixner,
Davidlohr Bueso, Arnd Bergmann, Jonathan Corbet, Ingo Molnar,
Ingo Molnar, Paul E . McKenney, Steven Rostedt, Bjorn Helgaas,
Kurt Schwemmer, linux-pci@vger.kernel.org Felipe Balbi,
".ozlabs.org", Logan Gunthorpe, Randy Dunlap,
linux-wireless, Oleg Nesterov, linuxppc-dev, Greg Kroah-Hartman,
Linus Torvalds, David S. Miller
In-Reply-To: <20200322021938.175736-1-joel@joelfernandes.org>
"Joel Fernandes (Google)" <joel@joelfernandes.org> writes:
> Reword and clarify better about the rwsem non-owner release issue.
>
> Link: https://lore.kernel.org/linux-pci/20200321212144.GA6475@google.com/
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
There's something wrong with your linux-pci and linux-usb addresses:
"linux-pci@vger.kernel.org Felipe Balbi" <balbi@kernel.org>,
"linux-usb@vger.kernel.org Kalle Valo" <kvalo@codeaurora.org>,
--
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* [PATCH] Documentation: Clarify better about the rwsem non-owner release issue
From: Joel Fernandes (Google) @ 2020-03-22 2:19 UTC (permalink / raw)
To: linux-kernel
Cc: linux-doc, Peter Zijlstra, Sebastian Andrzej Siewior, netdev,
Joel Fernandes (Google), Will Deacon, Thomas Gleixner,
Davidlohr Bueso, Arnd Bergmann, Jonathan Corbet, Ingo Molnar,
Ingo Molnar, Paul E . McKenney, linuxppc-dev, Steven Rostedt,
Bjorn Helgaas, Kurt Schwemmer,
linux-usb@vger.kernel.org Kalle Valo,
linux-pci@vger.kernel.org Felipe Balbi, Logan Gunthorpe,
Randy Dunlap, linux-wireless, Oleg Nesterov, Greg Kroah-Hartman,
Linus Torvalds, David S. Miller
Reword and clarify better about the rwsem non-owner release issue.
Link: https://lore.kernel.org/linux-pci/20200321212144.GA6475@google.com/
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
Documentation/locking/locktypes.rst | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/locking/locktypes.rst b/Documentation/locking/locktypes.rst
index 6f2c0f5b041e..656dce72f11f 100644
--- a/Documentation/locking/locktypes.rst
+++ b/Documentation/locking/locktypes.rst
@@ -292,7 +292,7 @@ implementations to provide priority inheritance for all lock types except
the truly spinning ones. Priority inheritance on ownerless locks is
obviously impossible.
-For now the rwsem non-owner release excludes code which utilizes it from
-being used on PREEMPT_RT enabled kernels. In same cases this can be
-mitigated by disabling portions of the code, in other cases the complete
-functionality has to be disabled until a workable solution has been found.
+For now, a PREEMPT_RT kernel just disables code sections that perform a
+non-owner release of an rwsem. In some cases, parts of the code are disabled.
+In other cases, the complete functionality has to be disabled until a workable
+solution has been found.
--
2.25.1.696.g5e7596f4ac-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox