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

From: "Wu, Fei" <fei2.wu@intel.com>
To: Richard Henderson <richard.henderson@linaro.org>,
	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: Wed, 22 Mar 2023 14:40:38 +0800	[thread overview]
Message-ID: <fcac17c1-0a7e-4e84-9fce-904037e52b29@intel.com> (raw)
In-Reply-To: <38598c82-f625-b84b-0f91-30b1fe98e8c6@intel.com>

On 3/22/2023 11:36 AM, Wu, Fei wrote:
> On 3/22/2023 11:31 AM, Richard Henderson wrote:
>> 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.
>>
> Got it, let me try this method.
> 
There seems no room in flags for this extra index, all 3 bits for
mem_idx have been used in target/riscv/cpu.h. We need some trick.

#define TB_FLAGS_PRIV_MMU_MASK                3
#define TB_FLAGS_PRIV_HYP_ACCESS_MASK   (1 << 2)

FIELD(TB_FLAGS, MEM_IDX, 0, 3)
FIELD(TB_FLAGS, LMUL, 3, 3)

Thanks,
Fei.

> Thanks,
> Fei.
> 
>>
>> r~
> 