Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 05 Jun 2021 10:43:19 +1000
From:   Nicholas Piggin <npiggin@gmail.com>
Subject: Re: [PATCH v4 12/16] powerpc/pseries/vas: Setup IRQ and fault
 handling
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
References: <8d219c0816133a8643d650709066cf04c9c77322.camel@linux.ibm.com>
        <5ac32e4d07bd048e3d687354501d36c334f1c8e0.camel@linux.ibm.com>
        <1622697882.lu1gj10oe8.astroid@bobo.none>
        <ab831a47cae65c67e1fae41acd9dcb8f3c55ac76.camel@linux.ibm.com>
In-Reply-To: <ab831a47cae65c67e1fae41acd9dcb8f3c55ac76.camel@linux.ibm.com>
MIME-Version: 1.0
Message-Id: <1622853468.3f8ohl3pt0.astroid@bobo.none>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Excerpts from Haren Myneni's message of June 4, 2021 11:19 am:
> On Thu, 2021-06-03 at 15:48 +1000, Nicholas Piggin wrote:
>> Excerpts from Haren Myneni's message of May 21, 2021 7:39 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.
>> >=20
>> > This patch also adds changes to setup and free IRQ per each
>> > window and also handles the fault by updating the CSB.
>> >=20
>> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
>> > ---
>> >  arch/powerpc/platforms/pseries/vas.c | 111
>> > +++++++++++++++++++++++++++
>> >  1 file changed, 111 insertions(+)
>> >=20
>> > diff --git a/arch/powerpc/platforms/pseries/vas.c
>> > b/arch/powerpc/platforms/pseries/vas.c
>> > index ef0c455f6e93..31dc17573f50 100644
>> > --- a/arch/powerpc/platforms/pseries/vas.c
>> > +++ b/arch/powerpc/platforms/pseries/vas.c
>> > @@ -224,6 +224,62 @@ int plpar_vas_query_capabilities(const u64
>> > hcall, u8 query_type,
>> >  }
>> >  EXPORT_SYMBOL_GPL(plpar_vas_query_capabilities);
>> > =20
>> > +/*
>> > + * HCALL to get fault CRB from pHyp.
>> > + */
>> > +static int plpar_get_nx_fault(u32 winid, u64 buffer)
>> > +{
>> > +	int64_t rc;
>> > +
>> > +	rc =3D 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_STATE:
>> > +		pr_err("HCALL(%x): No outstanding faults for 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): Unexpected error %lld for
>> > window(%u)\n",
>> > +		       H_GET_NX_FAULT, rc, winid);
>> > +		return -EIO;
>> > +	}
>> > +}
>>=20
>> Out of curiosity, you get one of these errors and it just drops the
>> interrupt on the floor. Then what happens, I assume everything
>> stops. Should it put some error in the csb, or signal the process or
>> something? Or is there nothing very sane that can be done?
>=20
> The user space polls on CSB with timout interval. If the kernel or NX
> does not return, the request will be timeout.

Okay, if there is no sane way it can respond to the different error=20
cases that's not necessarily unreasonable to just print something in the=20
kernel log. Hopefully the kernel log would be useful to the=20
administrator / developer / etc, but that's pretty rarely the case for=20
Linux errors as it is.

> The hypervisor returns the credit after H_GET_NX_FAULT HCALL is
> successful. Also one credit is assigned for each window. So in this
> case, the error is coming from the hypervisor and the application can
> not issue another request on the same window.=20
>=20
>>=20
>> > +
>> > +/*
>> > + * 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 vas_window *txwin =3D data;
>> > +	struct coprocessor_request_block crb;
>> > +	struct vas_user_win_ref *tsk_ref;
>> > +	int rc;
>> > +
>> > +	rc =3D plpar_get_nx_fault(txwin->winid, (u64)virt_to_phys(&crb));
>> > +	if (!rc) {
>> > +		tsk_ref =3D &txwin->task_ref;
>> > +		vas_dump_crb(&crb);
>>=20
>> This (and existing powernv vas code) is printk()ing a lot of lines
>> per=20
>> fault. This should be pretty normal operation I think? It should
>> avoid
>> filling the kernel logs, if so. Particularly if it can be triggered
>> by=20
>> userspace.
>>=20
>> I know it's existing code, so could be fixed separately from the
>> series.
>=20
> printk messages are only if HCALL returns failure or kernel issue (ex:
> not valid window and etc on powerNV). These errors should not be
> depending on the iser space requests. So generally we should not get
> these errors. =20

Ah I was looking at dump_crb but that's using pr_devel so that's=20
probably okay.

Thanks,
Nick