Re: [Xen-devel] [PATCH v2 2/7] xen/arm: Remove flush_xen_text_tlb_local()

[Julien Grall <Julien.Grall@xxxxxxx>]

Hi,

On 09/05/2019 21:03, Stefano Stabellini wrote:
> On Wed, 8 May 2019, Julien Grall wrote:
>> The function flush_xen_text_tlb_local() has been misused and will result
>> to invalidate the instruction cache more than necessary.
>>
>> For instance, there are no need to invalidate the instruction cache if
>                         ^ is
> 
> 
>> we are setting SCTLR_EL2.WXN.
>>
>> There are effectively only one caller (i.e free_init_memory() would
>          ^ is
> 
>> who need to invalidate the instruction cache.
>    ^ would who / who would
> 
>>
>> So rather than keeping around the function flush_xen_text_tlb_local()
>> around, replace it with call to flush_xen_tlb_local() and explicitely
>    ^ remove

I will fix the typoes in the next version.

> 
> 
>> flush the cache when necessary.
>>
>> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
>> Reviewed-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
>>
>> ---
>>      Changes in v2:
>>          - Add Andrii's reviewed-by
>> ---
>>   xen/arch/arm/mm.c                | 17 ++++++++++++++---
>>   xen/include/asm-arm/arm32/page.h | 23 +++++++++--------------
>>   xen/include/asm-arm/arm64/page.h | 21 +++++----------------
>>   3 files changed, 28 insertions(+), 33 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 93ad118183..dfbe39c70a 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -610,8 +610,12 @@ void __init remove_early_mappings(void)
>>   static void xen_pt_enforce_wnx(void)
>>   {
>>       WRITE_SYSREG32(READ_SYSREG32(SCTLR_EL2) | SCTLR_WXN, SCTLR_EL2);
>> -    /* Flush everything after setting WXN bit. */
>> -    flush_xen_text_tlb_local();
>> +    /*
>> +     * The TLBs may cache SCTLR_EL2.WXN. So ensure it is synchronized
>> +     * before flushing the TLBs.
>> +     */
>> +    isb();
>> +    flush_xen_data_tlb_local();
>>   }
>>   
>>   extern void switch_ttbr(uint64_t ttbr);
>> @@ -1123,7 +1127,7 @@ static void set_pte_flags_on_range(const char *p, 
>> unsigned long l, enum mg mg)
>>           }
>>           write_pte(xen_xenmap + i, pte);
>>       }
>> -    flush_xen_text_tlb_local();
>> +    flush_xen_data_tlb_local();
> 
> I think it would make sense to move the remaining call to
> flush_xen_data_tlb_local from set_pte_flags_on_range to free_init_memory
> before the call to invalidate_icache_local. What do you think?

We still need the TLB flush for the two callers. The first one for 
remove all TLBs with the previous permission, the second when the 
mappings are removed from the TLBs.

Today, it is not possible to re-use the virtual address of the init 
section, so it is arguably not necessary. However, I don't want to take 
the chance to introduce potential coherency issues if the TLBs entries 
where still present when re-using the virtual address.

Cheers,

-- 
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel