Re: [PATCH v4 09/10] tools: lib: perf: Implement riscv mmap support

[Jessica Clarke]

On 31 Jul 2023, at 20:47, Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
> 
> On Mon, Jul 31, 2023 at 09:46:07AM -0700, Ian Rogers wrote:
>> On Mon, Jul 31, 2023 at 9:06 AM Alexandre Ghiti <alexghiti@xxxxxxxxxxxx> wrote:
>>> I have just had the answer internally (thanks to @Brendan Sweeney):
>>> csr modifications can alter how the memory is accessed (satp which
>>> changes the address space, sum which allows/disallows userspace
>>> access...), so we need the memory barrier to make sure the compiler
>>> does not reorder the memory accesses.
>> 
>> The conditions you mention shouldn't apply here though? Even if you
>> add a memory barrier for the compiler what is stopping the hardware
>> reordering loads and stores? If it absolutely has to be there then a
>> comment something like "There is a bug is riscv where the csrr
>> instruction can clobber memory breaking future reads and some how this
>> constraint fixes it by ... ".
> 
> If the hardware doesn't know that writing to a csr can change how memory
> accesses are done and reorders memory accesses around that csr write,
> you've got a really broken piece of hardware on your hands ...
> 
> I know nothing about risc-v, and maybe the definition says that you need
> to put a memory barrier before and/or after it in the instruction stream,
> but on all hardware I'm familiar with, writing to a CSR is an implicitly
> serialising operation.

satp has some special rules due to the interaction with TLBs. Enabling
/ disabling translation by toggling between Bare and non-Bare modes
doesn’t require any kind of fence, nor does changing the ASID (if not
recycling), but any other changes to satp (changing between
Sv39/Sv48/Sv57 or changing the page table base address) do require
fences (not really sure why to be honest, maybe something about having
separate kernel and user page tables?..). §5.2.1 Supervisor
Memory-Management Fence Instruction of the privileged spec (the one I’m
looking at is version 20211203 but dated October 4, 2022) details this.

Jess