Re: [PATCH] target/riscv: reduce overhead of MSTATUS_SUM change

From: Richard Henderson <richard.henderson@linaro.org>
To: "Wu, Fei" <fei2.wu@intel.com>, LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>,
	Alistair Francis <alistair.francis@wdc.com>,
	Bin Meng <bin.meng@windriver.com>,
	Weiwei Li <liweiwei@iscas.ac.cn>,
	Daniel Henrique Barboza <dbarboza@ventanamicro.com>,
	"open list:RISC-V TCG CPUs" <qemu-riscv@nongnu.org>,
	"open list:All patches CC here" <qemu-devel@nongnu.org>
Subject: Re: [PATCH] target/riscv: reduce overhead of MSTATUS_SUM change
Date: Tue, 21 Mar 2023 20:31:25 -0700	[thread overview]
Message-ID: <b48b2cb9-bf2c-f527-435d-df497ea4df76@linaro.org> (raw)
In-Reply-To: <4f63f756-55d1-70f6-10e1-875c1e515fdf@intel.com>

On 3/21/23 19:47, Wu, Fei wrote:
>>> You should be making use of different softmmu indexes, similar to how
>>> ARM uses a separate index for PAN (privileged access never) mode.  If
>>> I read the manual properly, PAN == !SUM.
>>>
>>> When you do this, you need no additional flushing.
>>
>> Hi Fei,
>>
>> Let's follow Richard's advice.
>> Yes, I'm thinking about how to do it, and thank Richard for the advice.
> 
> My question is:
> * If we ensure this separate index (S+SUM) has no overlapping tlb
> entries with S-mode (ignore M-mode so far), during SUM=1, we have to
> look into both (S+SUM) and S index for kernel address translation, that
> should be not desired.

This is an incorrect assumption.  S+SUM may very well have overlapping tlb entries with S.
With SUM=1, you *only* look in S+SUM index; with SUM=0, you *only* look in S index.

The only difference is a check in get_physical_address is no longer against MSTATUS_SUM 
directly, but against the mmu_index.

> * If all the tlb operations are against (S+SUM) during SUM=1, then
> (S+SUM) could contain some duplicated tlb entries of kernel address in S
> index, the duplication means extra tlb lookup and fill.

Yes, if the same address is probed via S and S+SUM, there is a duplicated lookup.  But 
this is harmless.

> Also if we want
> to flush tlb entry of specific addr0, we have to flush both index.

Yes, this is also true.  But so far target/riscv is making no use of per-mmuidx flushing. 
At the moment you're *only* using tlb_flush(cpu), which flushes every mmuidx.  Nor are you 
making use of per-page flushing.

So, really, no change required at all there.

r~