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

[Julien Grall <Julien.Grall@xxxxxxx>]

On 25/04/2019 19:00, Andrii Anisov wrote:
> Hello Julien,

Hi,

> On 17.04.19 20:58, 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
>> we are setting SCTLR_EL2.WXN.
>>
>> There are effectively only one caller (i.e free_init_memory() would
>> who need to invalidate the instruction cache.
>>
>> So rather than keeping around the function flush_xen_text_tlb_local()
>> around, replace it with call to flush_xen_tlb_local() and explicitely
>> flush the cache when necessary.
>>
>> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
>> ---
>>   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.
> I'm not sure about the comment above, it looks a bit confusing to me.
> As per my understanding flushing TLB is called to remove cached entries 
> together with cached SCTLR_EL2.WXN.
> And isb before to tlb flush ensures that WXN is set prior to flush so 
> that all new TLB entries will be fetched
> with this flag set.

The understanding is correct. I am not entirely sure how I can improve 
the comment.

To be honest, I am not entirely sure whether the isb() is necessary 
here. At worst, an entry of an existing mappings is not cached with the 
SCTLR_EL2.WXN set. This may only defer the problem is an entry is 
incorrectly configured before the flush. This is done early at boot (and 
will be done much earlier in [1]). So I am kind of tempted to drop the 
isb() here.

[...]

> With minor notes,
> 
> Reviewed-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
> 

Thank you!

Cheers,

[1] 
https://lists.xenproject.org/archives/html/xen-devel/2019-04/msg01667.html


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