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

[Julien Grall <Julien.Grall@xxxxxxx>]

Hi,

On 10/05/2019 18:57, Stefano Stabellini wrote:
> On Fri, 10 May 2019, Julien Grall wrote:
>> On 09/05/2019 22:46, Julien Grall wrote:
>>> 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.
>>
>> I thought about this a bit more and chat with my team at Arm. Xen on Arm only
>> support Cortex-A7, Cortex-A15 and Brahma 15 (see the CPU ID check in
>> arm32/head.S).
>>      
>> None of them are actually using VIVT instruction caches. In general, VIVT
>> caches are more difficult to deal with because they require more flush. So I
>> would be more incline to deny booting Xen on platform where the instruction
>> caches don't support IVIVT extension.
>>
>> I don't think that will have a major impact on the user because of my point
>> above.
> 
> Thanks for looking this up in details. I think there are two interesting
> points here:
> 
> 1) what to do with VIVT
> 2) what to write in the in-code comment
> 
> For 1) I think it would be OK to deny booting. For sure we need at least
> a warning. Would you be able to add the warning/boot-denial as part of
> this series, or at least an in-code comment?

I am planning to deny booting Xen on such platforms.

> 
> For 2) I would like this reasonining to be captured somewhere with a
> in-code comment, if nothing else as a reference to what to search in
> the Arm Arm. I don't know where is the best place for it. If
> invalidate_icache_local is not good place for the comment please suggest
> a better location.

I still don't understand what reasoning you want to write. If we don't 
support VIVT then the instruction cache is very easy to maintain. I.e 
"You flush if you modify the instruction".

I am worry that if we overdo the explanation in the code, then you are 
going to confuse more than one person. So it would be better to blank 
out "VIVT" completely from then.

Feel free to suggest an in-code comment so we can discuss on the worthiness.

Cheers,

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