Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190327100201.32220-1-anup.patel@wdc.com> <7edf91f1-1df5-5513-d372-d84edd12edf3@garyguo.net>
 <CAAhSdy1+rc9ge02uqNvse5Z6XmDxsRH0G=5GnMsh+6m1LMLFYg@mail.gmail.com> <49628dc9-c56d-6b60-9520-e4eb0e3b3424@garyguo.net>
In-Reply-To: <49628dc9-c56d-6b60-9520-e4eb0e3b3424@garyguo.net>
From:   Anup Patel <anup@brainfault.org>
Date:   Wed, 27 Mar 2019 19:40:39 +0530
Message-ID: <CAAhSdy1gUYZWZsaN7JESgKHv_GyMKq56_Fmg0dPspUaiBbSCtg@mail.gmail.com>
Subject: Re: [PATCH] RISC-V: Implement ASID allocator
To:     Gary Guo <gary@garyguo.net>
Cc:     Anup Patel <Anup.Patel@wdc.com>,
        Palmer Dabbelt <palmer@sifive.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Atish Patra <Atish.Patra@wdc.com>,
        Christoph Hellwig <hch@infradead.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Mike Rapoport <rppt@linux.ibm.com>,
        "linux-riscv@lists.infradead.org" <linux-riscv@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Mar 27, 2019 at 7:08 PM Gary Guo <gary@garyguo.net> wrote:
>
> On 27/03/2019 11:42, Anup Patel wrote:
> > On Wed, Mar 27, 2019 at 4:57 PM Gary Guo <gary@garyguo.net> wrote:
> >>
> >> Hi Anup,
> >>
> >> This won't work in an actual hardware with ASID support. There're more
> >
> > Can you elaborate why >
> >
> > This implementation is based on Linux ARM64 ASID allocator which is
> > tested for large number of CPUs on real HW.
> >
> >> interactions with TLB flushes that need to be considered. You won't see
> >
> > Yap, already considered. Please point me to unhandled case.
> >
> When memory mapping is changed, you need to flush it from all cores that
> previously have that process executed, etc. Our patches both take
> inspiration from ARM's code, but the major difference between my code is
> handling of cache invalidations, see my code's cache_mask, etc. This is
> actually the most error-prone part, and I spent more time trying to find
> an optimal solution for this than porting the ASID allocator. The major
> difference is that ARM has a much more expressive sets of TLB flush
> instructions compared to RISC-V.

We should not require explicit cache maintenance anywhere in RISC-V
because caches are transparent to SW in RISC-V. The HW can use
"sfence.vma" hints for cache maintenance.

Further, (just like ARM world) the page table walks are cache coherent in
RISC-V so we should not require any cache flushes along with TLB flushes.

Now if you seeing inconsistent cache contents then it might due to some
bug in your HW. I am just guessing here.

Regarding changing memory mapping, I am sure generic Linux kernel will
issue appropriate flush_tlb_xyz() calls.

>  >
> >> this on both QEMU and SiFive board, as QEMU does not have ASID, so it
> >> pretends that ASID is supported by just flushing its TLB everytime you
> >
> > Nope, it does not. It detects whether ASID is supported or not. If supported
> > it will also figure-out number of ASID bits supported by HW.
> >
> Except that you can detect that QEMU supports ASID, but actually it does
> not. However QEMU is still correct because it always flush TLB when you
> set SATP/SPTBR. You won't be able to find out bugs in your code by just
> testing on QEMU.

I am not advocating that testing on QEMU is sufficient. Its just functionally
correct and works on HW without ASID support.

>
> > SiFive board does not have ASID bits so this implementation successfully
> > detects that number of ASID bits are 0 and fallbacks to original way of
> > local TLB flushes. >
> >> change sptbr. I suspect the performance gain you see is just due to
> >> saved TLB flush as TLB flush is super expensive in QEMU (all translation
> >> block jumps need to be cleared).
> >
> > Yes, performance gain is due to saved TLB flushes.
> >
> > On HW which supports ASID bits, we will see more performance
> > improvements. >
> A hardware TLB flush is cheaper than QEMU' TLB flush. As no hardware
> supports ASID at the moment the performance gain is minimal.
> >>
> >> I have my version here https://github.com/nbdd0121/linux/tree/asid. I
> >> haven't done code cleanups yet, but this version has correctness of its
> >> ASID code tested on our TLB simulator tool (which unfortunately I can't
> >> share right now as it involves with unpublished works).
> >
> > Except few minor differences. You version of ASID allocator is same as
> > mine. In fact there are lot of similar code framgements in your version
> > compared to Linux ARM64 as well. I am sure this patch will work for you.
> > >>
> >> In fact my submit my previous patch series exactly as the basis of this
> >> patch.
> >
> > This patch is based your patch series so I suggest you take this patch
> > and try it on your simulator.
> >
> I've tested, and it does not boot.

Thanks for the info. Now help me make this patch better.

Regards,
Anup