[for-4.20] Re: [PATCH v12 12/12] xen/arm: add cache coloring support for Xen image

[Julien Grall <julien@xxxxxxx>]

Hi,

On 16/12/2024 14:36, Jan Beulich wrote:
> On 16.12.2024 15:28, Carlo Nonato wrote:
> > On Mon, Dec 16, 2024 at 2:56 PM Michal Orzel <michal.orzel@xxxxxxx> wrote:
> > > On 13/12/2024 17:28, Carlo Nonato wrote:
> > > > --- a/xen/arch/arm/arm64/mmu/mm.c
> > > > +++ b/xen/arch/arm/arm64/mmu/mm.c
> > > > @@ -1,6 +1,7 @@
> > > >   /* SPDX-License-Identifier: GPL-2.0-only */
> > > >
> > > >   #include <xen/init.h>
> > > > +#include <xen/llc-coloring.h>
> > > >   #include <xen/mm.h>
> > > >   #include <xen/pfn.h>
> > > >
> > > > @@ -138,8 +139,36 @@ void update_boot_mapping(bool enable)
> > > >   }
> > > >
> > > >   extern void switch_ttbr_id(uint64_t ttbr);
> > > > +extern void relocate_xen(uint64_t ttbr, void *src, void *dst, size_t len);
> > > >
> > > >   typedef void (switch_ttbr_fn)(uint64_t ttbr);
> > > > +typedef void (relocate_xen_fn)(uint64_t ttbr, void *src, void *dst, size_t 
> > > > len);
> > > > +
> > > > +void __init relocate_and_switch_ttbr(uint64_t ttbr) {
> > > CODING_STYLE: { needs to be on its own line
> > >
> > > Also, this function is only executed in case of LLC coloring, so shouldn't it
> > > be #ifdef protected.
> > Here and in other places (patch #8) I'm relying on DCE to remove functions
> > that are not called. This was a suggestion from Jan in that patch. Can we
> > adopt the same here?
> Yet how would the compiler spot that the function is unused? That would only
> work with LTO / WPO. DCE (as I did suggest elsewhere) requires the functions
> in question to be static (allowing the compiler to see enough of the whole
> picture).
Sorry for the late answer. I was away with limited e-mail access. While 
looking what was committing recently, I noticed that a dummy function 
was introduced:
void __init relocate_and_switch_ttbr(uint64_t ttbr) {}

If a function is not supposed to be called, then it should contain a 
BUG_ON() to catch any misusage. Otherwise, this is a recipe for 
disaster. In this case, it would not be trivial to notice the TTBR was 
not switched...
That said I would have actually considered to remove the empty stub. I 
am a bit surprised that DCE wouldn't work in this case because the call 
is protected with "if ( llc_coloring_enabled )". When cache coloring is 
not enabled, this would turn to an "if ( false )" and therefore all the 
code should be removed. What did I miss?
Note that this is what we already rely on for arm32 because there is no 
stub... So if this is problem then we definitely need to fix it on arm32 
as well...
IOW, we either introduce a stub (including the BUG_ON) for both arm32 
and arm64 in the header or we remove the stub completely.
Marco, Michal, can you have a look? Ideally, this should be fixed for 4.20.

Cheers,

--
Julien Grall