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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 10 May 2019, Julien Grall wrote:
> 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.

I suggest something like the following:

 /* 
  * Flush all hypervisor mappings from the TLB of the local processor. Note
  * that instruction cache maintenance might also be required when self
  * modifying Xen code, see D5-2522 in ARM DDI 0487D.a and B3.11.2 in ARM
  * DDI 0406C.c.
  */

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