Re: [RFC PATCH 2/2] accel/tcg: replace phys_pc with asid in TB htable key

[Vasilev Oleg via <qemu-devel@nongnu.org>]

On 12/23/2021 7:31 PM, Richard Henderson wrote:
> On 12/22/21 8:50 AM, Oleg Vasilev wrote:
>> From: Oleg Vasilev <vasilev.oleg@huawei.com>
>>
>> Using a physical pc requires to translate address every time next block
>> needs to be found and executed. This also contaminates TLB with code-related
>> records.
>>
>> Instead, I suggest we introduce an architecture-specific address space
>> identifier, and use it to distinguish between different AS's
>> translation blocks.
> 
> Why do you believe that asid is sufficient here?  You're not invalidating any more TBs 
> that I can see.  What happens when the kernel re-uses an asid?

Hi,

Sorry, I had some comments for the patch, but forgot to put it in.

So, I think I interpret the term "asid" in some other sense, namely, an
identifier, which is constant during whole lifespan of an address space.
Same as PID in that sense. Do you think this is a viable approach?

If we assume translation table wouldn't change during process life,
after the death of the process, all it address space would be anyway
unmapped and corresponding translation blocks would be invalidated.


> 
> I believe this patch to be fundamentally flawed.

Maybe it is, I just wanted to get feedback from you guys. Do you think
maybe exists some other way, which would not require translating va->pa
every time to look up next block?

More context is in:

 Subject: Suggestions for TCG performance improvements
 Date: Thu, 2 Dec 2021 09:47:13 +0000
 Message-ID: <c76bde31-8f3b-2d03-b7c7-9e026d4b5873@huawei.com>

> 
> All that said,
> 
>> +/* Returns the identifier for a current address space. */
>> +static uint64_t arm_get_asid(CPUState *cs)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(cs);
>> +    CPUARMState *env = &cpu->env;
>> +    ARMMMUIdx mmu_idx = arm_mmu_idx(env);
>> +    uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr;
>> +
>> +#define TCR_A1     (1U << 22)
>> +    return regime_ttbr(env, mmu_idx, (tcr&TCR_A1)>0);
>> +}
> 
> Why are you returning the entire ttbr, and not the asid in the top 16 bits?

Actually, for my particular case I seem to need to return the lowest 40
bits, which is actual base for TT.

Thanks,
Oleg

> 
> 
> r~
>