Received-SPF: pass (google.com: domain of linux-kernel+bounces-16654-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240102220134.3229156-1-samuel.holland@sifive.com> <20240102220134.3229156-9-samuel.holland@sifive.com>
In-Reply-To: <20240102220134.3229156-9-samuel.holland@sifive.com>
From: Alexandre Ghiti <alexghiti@rivosinc.com>
Date: Thu, 4 Jan 2024 13:39:20 +0100
Message-ID: <CAHVXubhKrC_7DEBXR_wCP9g7-KfWg9nuyim7+X1Ja2gh8s9+HA@mail.gmail.com>
Subject: Re: [PATCH v4 08/12] riscv: mm: Introduce cntx2asid/cntx2version
 helper macros
To: Samuel Holland <samuel.holland@sifive.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>, linux-riscv@lists.infradead.org, 
	linux-kernel@vger.kernel.org, linux-mm@kvack.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Jan 2, 2024 at 11:01=E2=80=AFPM Samuel Holland
<samuel.holland@sifive.com> wrote:
>
> When using the ASID allocator, the MM context ID contains two values:
> the ASID in the lower bits, and the allocator version number in the
> remaining bits. Use macros to make this separation more obvious.
>
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> ---
>
> (no changes since v1)
>
>  arch/riscv/include/asm/mmu.h |  3 +++
>  arch/riscv/mm/context.c      | 12 ++++++------
>  arch/riscv/mm/tlbflush.c     |  2 +-
>  3 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/arch/riscv/include/asm/mmu.h b/arch/riscv/include/asm/mmu.h
> index 355504b37f8e..a550fbf770be 100644
> --- a/arch/riscv/include/asm/mmu.h
> +++ b/arch/riscv/include/asm/mmu.h
> @@ -26,6 +26,9 @@ typedef struct {
>  #endif
>  } mm_context_t;
>
> +#define cntx2asid(cntx)                ((cntx) & asid_mask)
> +#define cntx2version(cntx)     ((cntx) & ~asid_mask)

Not a big fan of the naming, I would have something like
get_asid_from_context() and get_version_from_context() or something
like that, but up to you of course.

> +
>  void __init create_pgd_mapping(pgd_t *pgdp, uintptr_t va, phys_addr_t pa=
,
>                                phys_addr_t sz, pgprot_t prot);
>  #endif /* __ASSEMBLY__ */
> diff --git a/arch/riscv/mm/context.c b/arch/riscv/mm/context.c
> index 217fd4de6134..43d005f63253 100644
> --- a/arch/riscv/mm/context.c
> +++ b/arch/riscv/mm/context.c
> @@ -81,7 +81,7 @@ static void __flush_context(void)
>                 if (cntx =3D=3D 0)
>                         cntx =3D per_cpu(reserved_context, i);
>
> -               __set_bit(cntx & asid_mask, context_asid_map);
> +               __set_bit(cntx2asid(cntx), context_asid_map);
>                 per_cpu(reserved_context, i) =3D cntx;
>         }
>
> @@ -102,7 +102,7 @@ static unsigned long __new_context(struct mm_struct *=
mm)
>         lockdep_assert_held(&context_lock);
>
>         if (cntx !=3D 0) {
> -               unsigned long newcntx =3D ver | (cntx & asid_mask);
> +               unsigned long newcntx =3D ver | cntx2asid(cntx);
>
>                 /*
>                  * If our current CONTEXT was active during a rollover, w=
e
> @@ -115,7 +115,7 @@ static unsigned long __new_context(struct mm_struct *=
mm)
>                  * We had a valid CONTEXT in a previous life, so try to
>                  * re-use it if possible.
>                  */
> -               if (!__test_and_set_bit(cntx & asid_mask, context_asid_ma=
p))
> +               if (!__test_and_set_bit(cntx2asid(cntx), context_asid_map=
))
>                         return newcntx;
>         }
>
> @@ -168,7 +168,7 @@ static void set_mm_asid(struct mm_struct *mm, unsigne=
d int cpu)
>          */
>         old_active_cntx =3D atomic_long_read(&per_cpu(active_context, cpu=
));
>         if (old_active_cntx &&
> -           ((cntx & ~asid_mask) =3D=3D atomic_long_read(&current_version=
)) &&
> +           (cntx2version(cntx) =3D=3D atomic_long_read(&current_version)=
) &&
>             atomic_long_cmpxchg_relaxed(&per_cpu(active_context, cpu),
>                                         old_active_cntx, cntx))
>                 goto switch_mm_fast;
> @@ -177,7 +177,7 @@ static void set_mm_asid(struct mm_struct *mm, unsigne=
d int cpu)
>
>         /* Check that our ASID belongs to the current_version. */
>         cntx =3D atomic_long_read(&mm->context.id);
> -       if ((cntx & ~asid_mask) !=3D atomic_long_read(&current_version)) =
{
> +       if (cntx2version(cntx) !=3D atomic_long_read(&current_version)) {
>                 cntx =3D __new_context(mm);
>                 atomic_long_set(&mm->context.id, cntx);
>         }
> @@ -191,7 +191,7 @@ static void set_mm_asid(struct mm_struct *mm, unsigne=
d int cpu)
>
>  switch_mm_fast:
>         csr_write(CSR_SATP, virt_to_pfn(mm->pgd) |
> -                 ((cntx & asid_mask) << SATP_ASID_SHIFT) |
> +                 (cntx2asid(cntx) << SATP_ASID_SHIFT) |
>                   satp_mode);
>
>         if (need_flush_tlb)
> diff --git a/arch/riscv/mm/tlbflush.c b/arch/riscv/mm/tlbflush.c
> index 76b24d4ed4ab..5ec621545c69 100644
> --- a/arch/riscv/mm/tlbflush.c
> +++ b/arch/riscv/mm/tlbflush.c
> @@ -85,7 +85,7 @@ static void __flush_tlb_range(struct mm_struct *mm, uns=
igned long start,
>                         return;
>
>                 if (static_branch_unlikely(&use_asid_allocator))
> -                       asid =3D atomic_long_read(&mm->context.id) & asid=
_mask;
> +                       asid =3D cntx2asid(atomic_long_read(&mm->context.=
id));
>         } else {
>                 cmask =3D cpu_online_mask;
>         }
> --
> 2.42.0
>

You can add:

Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>

Thanks,

Alex