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

[Julien Grall <julien@xxxxxxx>]

Hi Carlo,

On 03/12/2024 11:37, Carlo Nonato wrote:
> On Tue, Dec 3, 2024 at 11:36 AM Julien Grall <julien@xxxxxxx> wrote:
> > On 03/12/2024 10:08, Carlo Nonato wrote:
> > > Hi Julien,
> > >
> > > On Mon, Dec 2, 2024 at 10:44 PM Julien Grall <julien@xxxxxxx> wrote:
> > > > Hi Carlo,
> > > [...]
> > >
> > > > > diff --git a/xen/arch/arm/arm64/mmu/mm.c b/xen/arch/arm/arm64/mmu/mm.c
> > > > > index 671eaadbc1..3732d5897e 100644
> > > > > --- 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,27 +139,46 @@ 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 switch_ttbr(uint64_t ttbr)
> > > > Given the change below, I think this function needs to be renamed.
> > > > Possibly to relocate_and_jump() with a comment explaning that the
> > > > relocation only happen for cache-coloring.
> > > Changing the name of switch_ttbr() to relocate_and_jump() seems a bit
> > > misleading to me. First I need to change the name also for arm32 where there's
> > > no relocation at all. Second, relocation is something that happens
> > > conditionally so I don't think it's a good name for the function.
> > Feel free to propose a new name. The main thing is the current name
> > can't stay "switch_ttbr()" because you are doing more than switching the
> > TTBR.
> >
> > The other solution is to have a separate call for relocating xen (which
> > will fall-through to switch_ttbr) and another one for those that only to
> > switch TTBR.
> What about a function like this one, defined in xen/arch/arm/arm64/mmu/mm.c:
>
> typedef void (relocate_xen_fn)(uint64_t ttbr, void *src, void *dst, size_t len);
>
> void __init relocate_and_switch_ttbr(uint64_t ttbr) {
>      vaddr_t id_addr = virt_to_maddr(relocate_xen);
>      relocate_xen_fn *fn = (relocate_xen_fn *)id_addr;
>      lpae_t pte;
>
>      /* Enable the identity mapping in the boot page tables */
>      update_identity_mapping(true);
>
>      /* Enable the identity mapping in the runtime page tables */
>      pte = pte_of_xenaddr((vaddr_t)relocate_xen);
>      pte.pt.table = 1;
>      pte.pt.xn = 0;
>      pte.pt.ro = 1;
>      write_pte(&xen_third_id[third_table_offset(id_addr)], pte);
>
>      /* Relocate Xen and switch TTBR */
>      fn(ttbr, _start, (void *)BOOT_RELOC_VIRT_START, _end - _start);
>
>      /*
>       * Disable the identity mapping in the runtime page tables.
>       * Note it is not necessary to disable it in the boot page tables
>       * because they are not going to be used by this CPU anymore.
>       */
>      update_identity_mapping(false);
> }
>
> which is actually a clone of switch_ttbr() but it does relocation. I would
> then call it in case of coloring in setup_pagetables(). This should go in the
> direction you suggested, but it would duplicate a bit of code. What do you
> think about it?
I think the duplication is fine here.

It would be possible to reduce the duplication is we introduce an helper 
for call update_identity_mapping(true) and update the PTE. But I am not 
sure it is worth it.
Cheers,

--
Julien Grall