[PATCH] lib: sbi: Correctly limit flushes to a single ASID/VMID

[Anup Patel]

On Thu, Nov 16, 2023 at 6:05 PM Samuel Holland
<samuel.holland at sifive.com> wrote:
>
> Hi Anup,
>
> On 2023-11-15 10:03 PM, Anup Patel wrote:
> > On Wed, Oct 18, 2023 at 3:50 AM Samuel Holland
> > <samuel.holland at sifive.com> wrote:
> >>
> >> Per the SBI specification, the effects of these functions are limited to
> >> a specific ASID and/or VMID. This applies even when flushing the entire
> >> address space.
> >>
> >> Signed-off-by: Samuel Holland <samuel.holland at sifive.com>
> >
> > This patch looks good to me.
> >
> > Reviewed-by: Anup Patel <anup at brainfault.org>
> >
> > Applied this patch to the riscv/opensbi repo.
> >
> >> ---
> >> The SBI spec doesn't say anything about this `start == 0 && size == 0`
> >> exception, so I wonder if it should be removed entirely.
> >
> > The SBI spec does define behaviour for `start == 0 && size == 0`.
> >
> > At the beginning of "Chapter 8. RFENCE Extension" we have the
> > following text which describes this behavior:
> >
> > "
> > The remote fence function acts as a full TLB flush if
> > * start_addr and size are both 0
> > * size is equal to 2^XLEN-1
> > "
>
> Thanks! I didn't see this paragraph in the spec. That explains the previous
> implementation (even though I still think it was wrong): "acts as a full TLB
> flush" could be interpreted to imply ignoring the ASID.
>
> I would suggest updating the SBI documentation with more precise wording,
> something like "applies to the entire address space". We should also copy this
> paragraph to ext-legacy.adoc for the legacy remote SFENCE.VMA functions, since
> the current paragraph technically does not apply to them.
>
> If you agree, I will add these changes to
> https://github.com/riscv-non-isa/riscv-sbi-doc/pull/128
> (or should I open a separate pull request?)

I think you should just add a separate commit for clarification
in the same PR.

Regards,
Anup

>
> Regards,
> Samuel
>
> >>
> >>  lib/sbi/sbi_tlb.c | 21 +++------------------
> >>  1 file changed, 3 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/lib/sbi/sbi_tlb.c b/lib/sbi/sbi_tlb.c
> >> index ced2854..dad9508 100644
> >> --- a/lib/sbi/sbi_tlb.c
> >> +++ b/lib/sbi/sbi_tlb.c
> >> @@ -111,12 +111,7 @@ void sbi_tlb_local_hfence_vvma_asid(struct sbi_tlb_info *tinfo)
> >>         hgatp = csr_swap(CSR_HGATP,
> >>                          (vmid << HGATP_VMID_SHIFT) & HGATP_VMID_MASK);
> >>
> >> -       if (start == 0 && size == 0) {
> >> -               __sbi_hfence_vvma_all();
> >> -               goto done;
> >> -       }
> >> -
> >> -       if (size == SBI_TLB_FLUSH_ALL) {
> >> +       if ((start == 0 && size == 0) || (size == SBI_TLB_FLUSH_ALL)) {
> >>                 __sbi_hfence_vvma_asid(asid);
> >>                 goto done;
> >>         }
> >> @@ -138,12 +133,7 @@ void sbi_tlb_local_hfence_gvma_vmid(struct sbi_tlb_info *tinfo)
> >>
> >>         sbi_pmu_ctr_incr_fw(SBI_PMU_FW_HFENCE_GVMA_VMID_RCVD);
> >>
> >> -       if (start == 0 && size == 0) {
> >> -               __sbi_hfence_gvma_all();
> >> -               return;
> >> -       }
> >> -
> >> -       if (size == SBI_TLB_FLUSH_ALL) {
> >> +       if ((start == 0 && size == 0) || (size == SBI_TLB_FLUSH_ALL)) {
> >>                 __sbi_hfence_gvma_vmid(vmid);
> >>                 return;
> >>         }
> >> @@ -162,13 +152,8 @@ void sbi_tlb_local_sfence_vma_asid(struct sbi_tlb_info *tinfo)
> >>
> >>         sbi_pmu_ctr_incr_fw(SBI_PMU_FW_SFENCE_VMA_ASID_RCVD);
> >>
> >> -       if (start == 0 && size == 0) {
> >> -               tlb_flush_all();
> >> -               return;
> >> -       }
> >> -
> >>         /* Flush entire MM context for a given ASID */
> >> -       if (size == SBI_TLB_FLUSH_ALL) {
> >> +       if ((start == 0 && size == 0) || (size == SBI_TLB_FLUSH_ALL)) {
> >>                 __asm__ __volatile__("sfence.vma x0, %0"
> >>                                      :
> >>                                      : "r"(asid)
> >> --
> >> 2.41.0
> >>
> >>
> >> --
> >> opensbi mailing list
> >> opensbi at lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/opensbi
> >
> > Thanks,
> > Anup
>