Re: [Xen-devel] [PATCH v2 4/7] xen/arm: page: Clarify the Xen TLBs helpers name

[Julien Grall <Julien.Grall@xxxxxxx>]

Hi,

On 09/05/2019 21:32, Julien Grall wrote:
> Hi,
> 
> On 09/05/2019 21:13, Stefano Stabellini wrote:
>> On Wed, 8 May 2019, Julien Grall wrote:
>>>   /* Release all __init and __initdata ranges to be reused */
>>> diff --git a/xen/include/asm-arm/arm32/page.h 
>>> b/xen/include/asm-arm/arm32/page.h
>>> index 40a77daa9d..0b41b9214b 100644
>>> --- a/xen/include/asm-arm/arm32/page.h
>>> +++ b/xen/include/asm-arm/arm32/page.h
>>> @@ -61,12 +61,8 @@ static inline void invalidate_icache_local(void)
>>>       isb();                      /* Synchronize fetched instruction 
>>> stream. */
>>>   }
>>> -/*
>>> - * Flush all hypervisor mappings from the data TLB of the local
>>> - * processor. This is not sufficient when changing code mappings or
>>> - * for self modifying code.
>>> - */
>>> -static inline void flush_xen_data_tlb_local(void)
>>> +/* Flush all hypervisor mappings from the TLB of the local 
>>> processor. */
>>
>> I realize that the statement "This is not sufficient when changing code
>> mappings or for self modifying code" is not quite accurate, but I would
>> prefer not to remove it completely. It would be good to retain a warning
>> somewhere about IC been needed when changing Xen's own mappings. Maybe
>> on top of invalidate_icache_local?
> 
> Can you please expand in which circumstance you need to invalid the 
> instruction cache when changing Xen's own mappings?

Reading the Armv7 (B3.11.2 in ARM DDI 0406C.c) and Armv8 (D5.11.2 in ARM 
DDI 0487D.a), most of the instruction caches implement the IVIPT 
extension. This means that instruction cache maintenance is required 
only after write new data to a PA that holds instructions (see D5-2522 
in ARM DDI 0487D.a and B3.11.2 in ARM DDI 0406C.c).

The only one that differs with that behavior is ASID and VMID tagged 
VIVT instruction caches which is only present in Armv7 (I can't remember 
why it was dropped in Armv8). Instruction cache maintenance can be 
required when changing the translation of a virtual address to a 
physical address.

There are only a few limited places where Xen mappings can change and a 
instruction cache flush is required (namely livepatch, changing 
permission, free init). All the others are not necessary.

A comment on top of invalidate_icache_local() is not going to help as 
you rely on the developer knows which function to use. The one on top of 
flush tlb helpers is at best misleading without a long explanation. At 
which point, you better read the Arm Arm.

Cheers,

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