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:   Mon, 10 May 2021 15:28:20 +1000
From:   Nicholas Piggin <npiggin@gmail.com>
Subject: Re: [V3 PATCH 03/16] powerpc/vas: Create take/drop task reference
 functions
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: <a910e5bd3f3398b4bd430b25a856500735b993c3.camel@linux.ibm.com>
        <3d634fb22a1d3d87f3172b23414a48650fc9a366.camel@linux.ibm.com>
In-Reply-To: <3d634fb22a1d3d87f3172b23414a48650fc9a366.camel@linux.ibm.com>
MIME-Version: 1.0
Message-Id: <1620624047.yy8sqcrk40.astroid@bobo.none>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Excerpts from Haren Myneni's message of April 18, 2021 7:03 am:
>=20
> Take task reference when each window opens and drops during close.
> This functionality is needed for powerNV and pseries. So this patch
> defines the existing code as functions in common book3s platform
> vas-api.c
>=20
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/vas.h              | 20 ++++++++
>  arch/powerpc/platforms/book3s/vas-api.c     | 51 ++++++++++++++++++
>  arch/powerpc/platforms/powernv/vas-fault.c  | 10 ++--
>  arch/powerpc/platforms/powernv/vas-window.c | 57 ++-------------------
>  arch/powerpc/platforms/powernv/vas.h        |  6 +--
>  5 files changed, 83 insertions(+), 61 deletions(-)
>=20
> diff --git a/arch/powerpc/include/asm/vas.h b/arch/powerpc/include/asm/va=
s.h
> index 6bbade60d8f4..2daaa1a2a9a9 100644
> --- a/arch/powerpc/include/asm/vas.h
> +++ b/arch/powerpc/include/asm/vas.h
> @@ -5,6 +5,9 @@
> =20
>  #ifndef _ASM_POWERPC_VAS_H
>  #define _ASM_POWERPC_VAS_H
> +#include <linux/sched/mm.h>
> +#include <linux/mmu_context.h>
> +#include <asm/icswx.h>
>  #include <uapi/asm/vas-api.h>
> =20
> =20
> @@ -60,6 +63,22 @@ struct vas_user_win_ops {
>  	int (*close_win)(void *);
>  };
> =20
> +struct vas_win_task {
> +	struct pid *pid;	/* Thread group ID of owner */
> +	struct pid *tgid;	/* Linux process mm_struct */
> +	struct mm_struct *mm;	/* Linux process mm_struct */
> +};

Looks okay, happy with struct vas_win_task? (and vas_user_win_ops)?

I'd be happier to have everything related to vas windows prefixed with=20
vas_window_ consistently, and have _user be present always for userspace
windows, but you have to read and type it.

> +
> +static inline void vas_drop_reference_task(struct vas_win_task *task)

This is not dropping a reference task, but a task reference. And it's=20
not really a task reference as far as Linux understands, but a
reference on pid (not task) and mm related to an open vas window. And
specifically a user window (with corresponding vas_user_win_ops).

Could it be called a 'struct vas_window_user_ref' instead?

Thanks,
Nick

> +{
> +	/* Drop references to pid and mm */
> +	put_pid(task->pid);
> +	if (task->mm) {
> +		mm_context_remove_vas_window(task->mm);
> +		mmdrop(task->mm);
> +	}