From: Nicholas Piggin <npiggin@gmail.com>
To: Haren Myneni <haren@linux.ibm.com>,
herbert@gondor.apana.org.au, linux-crypto@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, mpe@ellerman.id.au
Cc: haren@us.ibm.com, hbabu@us.ibm.com
Subject: Re: [PATCH v5 13/17] powerpc/pseries/vas: Setup IRQ and fault handling
Date: Mon, 14 Jun 2021 13:07:02 +1000 [thread overview]
Message-ID: <1623639403.054wgu233i.astroid@bobo.none> (raw)
In-Reply-To: <8a9fe47a17d1f89cc43885d2ef8c2f09e4e90d7a.camel@linux.ibm.com>
Excerpts from Haren Myneni's message of June 13, 2021 9:02 pm:
>
> NX generates an interrupt when sees a fault on the user space
> buffer and the hypervisor forwards that interrupt to OS. Then
> the kernel handles the interrupt by issuing H_GET_NX_FAULT hcall
> to retrieve the fault CRB information.
>
> This patch also adds changes to setup and free IRQ per each
> window and also handles the fault by updating the CSB.
>
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
> arch/powerpc/platforms/pseries/vas.c | 108 +++++++++++++++++++++++++++
> 1 file changed, 108 insertions(+)
>
> diff --git a/arch/powerpc/platforms/pseries/vas.c b/arch/powerpc/platforms/pseries/vas.c
> index fe375f7a7029..55185bdd3776 100644
> --- a/arch/powerpc/platforms/pseries/vas.c
> +++ b/arch/powerpc/platforms/pseries/vas.c
> @@ -11,6 +11,7 @@
> #include <linux/types.h>
> #include <linux/delay.h>
> #include <linux/slab.h>
> +#include <linux/interrupt.h>
> #include <asm/machdep.h>
> #include <asm/hvcall.h>
> #include <asm/plpar_wrappers.h>
> @@ -190,6 +191,58 @@ int h_query_vas_capabilities(const u64 hcall, u8 query_type, u64 result)
> }
> EXPORT_SYMBOL_GPL(h_query_vas_capabilities);
>
> +/*
> + * hcall to get fault CRB from pHyp.
> + */
> +static int h_get_nx_fault(u32 winid, u64 buffer)
> +{
> + long rc;
> +
> + rc = plpar_hcall_norets(H_GET_NX_FAULT, winid, buffer);
> +
> + switch (rc) {
> + case H_SUCCESS:
> + return 0;
> + case H_PARAMETER:
> + pr_err("HCALL(%x): Invalid window ID %u\n", H_GET_NX_FAULT,
> + winid);
> + return -EINVAL;
> + case H_PRIVILEGE:
> + pr_err("HCALL(%x): Window(%u): Invalid fault buffer 0x%llx\n",
> + H_GET_NX_FAULT, winid, buffer);
> + return -EACCES;
> + default:
> + pr_err("HCALL(%x): Failed with error %ld for window(%u)\n",
> + H_GET_NX_FAULT, rc, winid);
> + return -EIO;
3 error messages have 3 different formats for window ID.
I agree with Michael you could just have one error message that reports
the return value. Also "H_GET_NX_FAULT: " would be nicer than "HCALL(380): "
Check how some other hcall failures are reported, "hcall failed:
H_CALL_NAME" seems to have a few takers.
> + }
> +}
> +
> +/*
> + * Handle the fault interrupt.
> + * When the fault interrupt is received for each window, query pHyp to get
> + * the fault CRB on the specific fault. Then process the CRB by updating
> + * CSB or send signal if the user space CSB is invalid.
> + * Note: pHyp forwards an interrupt for each fault request. So one fault
> + * CRB to process for each H_GET_NX_FAULT HCALL.
> + */
> +irqreturn_t pseries_vas_fault_thread_fn(int irq, void *data)
> +{
> + struct pseries_vas_window *txwin = data;
> + struct coprocessor_request_block crb;
> + struct vas_user_win_ref *tsk_ref;
> + int rc;
> +
> + rc = h_get_nx_fault(txwin->vas_win.winid, (u64)virt_to_phys(&crb));
> + if (!rc) {
> + tsk_ref = &txwin->vas_win.task_ref;
> + vas_dump_crb(&crb);
> + vas_update_csb(&crb, tsk_ref);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> /*
> * Allocate window and setup IRQ mapping.
> */
> @@ -201,10 +254,51 @@ static int allocate_setup_window(struct pseries_vas_window *txwin,
> rc = h_allocate_vas_window(txwin, domain, wintype, DEF_WIN_CREDS);
> if (rc)
> return rc;
> + /*
> + * On powerVM, pHyp setup and forwards the fault interrupt per
The hypervisor forwards the fault interrupt per-window...
> + * window. So the IRQ setup and fault handling will be done for
> + * each open window separately.
> + */
> + txwin->fault_virq = irq_create_mapping(NULL, txwin->fault_irq);
> + if (!txwin->fault_virq) {
> + pr_err("Failed irq mapping %d\n", txwin->fault_irq);
> + rc = -EINVAL;
> + goto out_win;
> + }
> +
> + txwin->name = kasprintf(GFP_KERNEL, "vas-win-%d",
> + txwin->vas_win.winid);
> + if (!txwin->name) {
> + rc = -ENOMEM;
> + goto out_irq;
> + }
> +
> + rc = request_threaded_irq(txwin->fault_virq, NULL,
> + pseries_vas_fault_thread_fn, IRQF_ONESHOT,
> + txwin->name, txwin);
> + if (rc) {
> + pr_err("VAS-Window[%d]: Request IRQ(%u) failed with %d\n",
> + txwin->vas_win.winid, txwin->fault_virq, rc);
> + goto out_free;
> + }
>
> txwin->vas_win.wcreds_max = DEF_WIN_CREDS;
>
> return 0;
> +out_free:
> + kfree(txwin->name);
> +out_irq:
> + irq_dispose_mapping(txwin->fault_virq);
> +out_win:
> + h_deallocate_vas_window(txwin->vas_win.winid);
> + return rc;
> +}
> +
> +static inline void free_irq_setup(struct pseries_vas_window *txwin)
> +{
> + free_irq(txwin->fault_virq, txwin);
> + irq_dispose_mapping(txwin->fault_virq);
> + kfree(txwin->name);
Nit, but this freeing is in a different order than the error handling in
the function does. I'd just keep it the same unless there is a reason to
be different, in which case it could use a comment.
So long as the irq can't somehow fire and try to use txwin->name, you
might be okay.
Otherwise I _think_ it's okay, but I don't know the irq APIs well.
Thanks,
Nick
> }
>
> static struct vas_window *vas_allocate_window(struct vas_tx_win_open_attr *uattr,
> @@ -320,6 +414,11 @@ static struct vas_window *vas_allocate_window(struct vas_tx_win_open_attr *uattr
> return &txwin->vas_win;
>
> out_free:
> + /*
> + * Window is not operational. Free IRQ before closing
> + * window so that do not have to hold mutex.
> + */
Why don't you have to hold the mutex in that case?
> + free_irq_setup(txwin);
> h_deallocate_vas_window(txwin->vas_win.winid);
> out:
> atomic_dec(&ct_caps->used_lpar_creds);
> @@ -339,7 +438,16 @@ static int deallocate_free_window(struct pseries_vas_window *win)
> {
> int rc = 0;
>
> + /*
> + * Free IRQ after executing H_DEALLOCATE_VAS_WINDOW HCALL
> + * to close the window. pHyp waits for all requests including
> + * faults are processed before closing the window - Means all
> + * credits are returned. In the case of fault request, credit
> + * is returned after OS issues H_GET_NX_FAULT HCALL.
> + */
> rc = h_deallocate_vas_window(win->vas_win.winid);
> + if (!rc)
> + free_irq_setup(win);
>
> return rc;
> }
> --
> 2.18.2
>
>
>
next prev parent reply other threads:[~2021-06-14 3:07 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-13 10:53 [PATCH v5 00/17] Enable VAS and NX-GZIP support on PowerVM Haren Myneni
2021-06-13 10:54 ` [PATCH v5 01/17] powerpc/powernv/vas: Release reference to tgid during window close Haren Myneni
2021-06-14 2:11 ` Nicholas Piggin
2021-06-13 10:55 ` [PATCH v5 02/17] powerpc/vas: Move VAS API to book3s common platform Haren Myneni
2021-06-14 2:10 ` Nicholas Piggin
2021-06-13 10:56 ` [PATCH v5 03/17] powerpc/powernv/vas: Rename register/unregister functions Haren Myneni
2021-06-13 10:57 ` [PATCH v5 04/17] powerpc/vas: Add platform specific user window operations Haren Myneni
2021-06-14 2:24 ` Nicholas Piggin
2021-06-15 6:37 ` Haren Myneni
2021-06-16 9:11 ` Nicholas Piggin
2021-06-13 10:57 ` [PATCH v5 05/17] powerpc/vas: Create take/drop pid and mm reference functions Haren Myneni
2021-06-14 2:26 ` Nicholas Piggin
2021-06-13 10:58 ` [PATCH v5 06/17] powerpc/vas: Move update_csb/dump_crb to common book3s platform Haren Myneni
2021-06-14 3:34 ` Nicholas Piggin
2021-06-13 10:58 ` [PATCH v5 07/17] powerpc/vas: Define and use common vas_window struct Haren Myneni
2021-06-14 2:28 ` Nicholas Piggin
2021-06-13 10:59 ` [PATCH v5 08/17] powerpc/pseries/vas: Define VAS/NXGZIP hcalls and structs Haren Myneni
2021-06-14 2:32 ` Nicholas Piggin
2021-06-13 11:00 ` [PATCH v5 09/17] powerpc/vas: Define QoS credit flag to allocate window Haren Myneni
2021-06-14 2:32 ` Nicholas Piggin
2021-06-13 11:00 ` [PATCH v5 10/17] powerpc/pseries/vas: Add hcall wrappers for VAS handling Haren Myneni
2021-06-13 11:01 ` [PATCH v5 11/17] powerpc/pseries/vas: Implement getting capabilities from hypervisor Haren Myneni
2021-06-14 2:35 ` Nicholas Piggin
2021-06-13 11:02 ` [PATCH v5 12/17] powerpc/pseries/vas: Integrate API with open/close windows Haren Myneni
2021-06-14 2:55 ` Nicholas Piggin
2021-06-15 6:51 ` Haren Myneni
2021-06-15 7:26 ` Haren Myneni
2021-06-16 9:38 ` Nicholas Piggin
2021-06-13 11:02 ` [PATCH v5 13/17] powerpc/pseries/vas: Setup IRQ and fault handling Haren Myneni
2021-06-14 3:07 ` Nicholas Piggin [this message]
2021-06-15 9:01 ` Haren Myneni
2021-06-16 9:40 ` Nicholas Piggin
2021-06-13 11:03 ` [PATCH v5 14/17] crypto/nx: Rename nx-842-pseries file name to nx-common-pseries Haren Myneni
2021-06-14 3:07 ` Nicholas Piggin
2021-06-13 11:04 ` [PATCH v5 15/17] crypto/nx: Register and unregister VAS interface on PowerVM Haren Myneni
2021-06-14 3:35 ` Nicholas Piggin
2021-06-13 11:04 ` [PATCH v5 16/17] crypto/nx: Get NX capabilities for GZIP coprocessor type Haren Myneni
2021-06-14 3:39 ` Nicholas Piggin
2021-06-15 0:01 ` Haren Myneni
2021-06-13 11:05 ` [PATCH v5 17/17] crypto/nx: Add sysfs interface to export NX capabilities Haren Myneni
2021-06-14 3:46 ` Nicholas Piggin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1623639403.054wgu233i.astroid@bobo.none \
--to=npiggin@gmail.com \
--cc=haren@linux.ibm.com \
--cc=haren@us.ibm.com \
--cc=hbabu@us.ibm.com \
--cc=herbert@gondor.apana.org.au \
--cc=linux-crypto@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mpe@ellerman.id.au \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).