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

2019-08-14 Thread David Hildenbrand
On 14.08.19 12:44, Alex Bennée wrote:
> 
> David Hildenbrand  writes:
> 
>> On 14.08.19 12:06, Alex Bennée wrote:
>>>
>>> David Hildenbrand  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 
 Signed-off-by: David Hildenbrand 
 ---
  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, );
 +/* 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



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

2019-08-14 Thread Alex Bennée


David Hildenbrand  writes:

> On 14.08.19 12:06, Alex Bennée wrote:
>>
>> David Hildenbrand  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 
>>> Signed-off-by: David Hildenbrand 
>>> ---
>>>  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, );
>>> +/* 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));

?


--
Alex Bennée



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

2019-08-14 Thread David Hildenbrand
On 14.08.19 12:06, Alex Bennée wrote:
> 
> David Hildenbrand  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 
>> Signed-off-by: David Hildenbrand 
>> ---
>>  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, );
>> +/* 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.

-- 

Thanks,

David / dhildenb



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

2019-08-14 Thread Alex Bennée


David Hildenbrand  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 
> Signed-off-by: David Hildenbrand 
> ---
>  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, );
> +/* 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?

>  }
>
>  /* reset reference bit extended */
> @@ -1843,6 +1845,8 @@ uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2)
>  if (skeyclass->set_skeys(ss, r2 / TARGET_PAGE_SIZE, 1, )) {
>  return 0;
>  }
> +/* TODO: Flush only entries with this target address */
> +tlb_flush_all_cpus_synced(env_cpu(env));
>
>  /*
>   * cc


--
Alex Bennée