Re: [Qemu-devel] [PATCH-for-4.2 v2 3/6] s390x/tcg: Flush the TLB of all CPUs on SSKE and RRBE

[David Hildenbrand]

On 14.08.19 12:44, Alex Bennée wrote:
> 
> David Hildenbrand <address@hidden> writes:
> 
>> On 14.08.19 12:06, Alex Bennée wrote:
>>>
>>> David Hildenbrand <address@hidden> writes:
>>>
>>>> Whenever we modify a storage key, we shuld flush the TLBs of all CPUs,
>>>> so the MMU fault handling code can properly consider the changed storage
>>>> key (to e.g., properly set the reference and change bit on the next
>>>> accesses).
>>>>
>>>> These functions are barely used in modern Linux guests, so the performance
>>>> implications are neglectable for now.
>>>>
>>>> This is a preparation for better reference and change bit handling for
>>>> TCG, which will require more MMU changes.
>>>>
>>>> Reviewed-by: Cornelia Huck <address@hidden>
>>>> Signed-off-by: David Hildenbrand <address@hidden>
>>>> ---
>>>>  target/s390x/mem_helper.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
>>>> index 29d9eaa5b7..ed54265e03 100644
>>>> --- a/target/s390x/mem_helper.c
>>>> +++ b/target/s390x/mem_helper.c
>>>> @@ -1815,6 +1815,8 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, 
>>>> uint64_t r2)
>>>>
>>>>      key = (uint8_t) r1;
>>>>      skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
>>>> +    /* TODO: Flush only entries with this target address */
>>>> +    tlb_flush_all_cpus_synced(env_cpu(env));
>>>
>>> Doesn't:
>>>
>>>   tlb_flush_page_all_cpus_synced(env_cpu(env), addr & TARGET_PAGE_MASK);
>>>
>>> do what you want here?
>>
>> I would have to flush all TLB entries that target this physical page,
>> not the entry of the single virtual page. So that does, unfortunately,
>> not work.
> 
> Ahh I see. Well maybe that should be the comment instead:
> 
>   /*
>    * As we can only flush by virtual address and not all the entries
>    * that point to a physical address we have to flush the whole TLB
>    * here.
>    */
>    tlb_flush_all_cpus_synced(env_cpu(env));
> 
> ?

I think "entries that target this address" makes it clear that we are
talking about the TLB entry target address, not the virtual source
address. But I can adjust it. Thanks.

-- 

Thanks,

David / dhildenb