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 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?

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.

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