Re: [PATCH v5 13/13] xen/arm: add cache coloring support for Xen

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Julien,

On Mon, Jan 15, 2024 at 12:18 PM Julien Grall <julien@xxxxxxx> wrote:
> On 15/01/2024 10:11, Carlo Nonato wrote:
> > Hi Julien,
>
> Hi Carlo,
>
> > On Sun, Jan 14, 2024 at 8:22 PM Julien Grall <julien@xxxxxxx> wrote:
> >>
> >> Hi Carlo,
> >>
> >> On 13/01/2024 17:07, Carlo Nonato wrote:
> >>>>> diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
> >>>>> index 37b6d230ad..66b674eeab 100644
> >>>>> --- a/xen/arch/arm/mmu/setup.c
> >>>>> +++ b/xen/arch/arm/mmu/setup.c
> >>>>> @@ -7,6 +7,7 @@
> >>>>>
> >>>>>     #include <xen/init.h>
> >>>>>     #include <xen/libfdt/libfdt.h>
> >>>>> +#include <xen/llc-coloring.h>
> >>>>>     #include <xen/sizes.h>
> >>>>>     #include <xen/vmap.h>
> >>>>>
> >>>>> @@ -39,6 +40,10 @@ DEFINE_PER_CPU(lpae_t *, xen_pgtable);
> >>>>>     static DEFINE_PAGE_TABLE(cpu0_pgtable);
> >>>>>     #endif
> >>>>>
> >>>>> +#ifdef CONFIG_LLC_COLORING
> >>>>> +static DEFINE_PAGE_TABLE(xen_colored_temp);
> >>>>> +#endif
> >>>>
> >>>> Does this actually need to be static?
> >>>
> >>> Why it shouldn't be static? I don't want to access it from another file.
> >>
> >> My question was whether this could be allocated dynamically (or possibly
> >> re-use an existing set of page tables). In particular with the fact that
> >> we will need more than 1 page to cover the whole Xen binary.
> >>
> >> Looking at the use xen_colored_temp. This is pretty much the same as
> >> xen_map[i] but with different permissions. So what you could do is
> >> preparing xen_map[i] with very permissive permissions (i.e. RWX) and
> >> then enforcing the permission once the TTBR has been switched.
> >>
> >> Something like that (tested without cache coloring):
> >>
> >> diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
> >> index a3a263a5d94b..f7ac5cabf92c 100644
> >> --- a/xen/arch/arm/mmu/setup.c
> >> +++ b/xen/arch/arm/mmu/setup.c
> >> @@ -306,7 +306,11 @@ void __init setup_pagetables(unsigned long
> >> boot_phys_offset, paddr_t xen_paddr)
> >>        p[0].pt.table = 1;
> >>        p[0].pt.xn = 0;
> >>
> >> -    /* Break up the Xen mapping into pages and protect them separately. */
> >> +    /*
> >> +     * Break up the Xen mapping into pages. We will protect the
> >> +     * permissions later in order to allow xen_xenmap to be used for
> >> +     * when relocating Xen.
> >> +     */
> >>        for ( i = 0; i < XEN_NR_ENTRIES(3); i++ )
> >>        {
> >>            vaddr_t va = XEN_VIRT_START + (i << PAGE_SHIFT);
> >> @@ -315,13 +319,7 @@ void __init setup_pagetables(unsigned long
> >> boot_phys_offset, paddr_t xen_paddr)
> >>                break;
> >>            pte = pte_of_xenaddr(va);
> >>            pte.pt.table = 1; /* third level mappings always have this bit
> >> set */
> >> -        if ( is_kernel_text(va) || is_kernel_inittext(va) )
> >> -        {
> >> -            pte.pt.xn = 0;
> >> -            pte.pt.ro = 1;
> >> -        }
> >> -        if ( is_kernel_rodata(va) )
> >> -            pte.pt.ro = 1;
> >> +        pte.pt.xn = 0; /* Permissions will be enforced later. Allow
> >> execution */
> >>            xen_xenmap[i] = pte;
> >>        }
> >>
> >> @@ -352,6 +350,37 @@ void __init setup_pagetables(unsigned long
> >> boot_phys_offset, paddr_t xen_paddr)
> >>
> >>        switch_ttbr(ttbr);
> >>
> >> +    /* Protect Xen */
> >> +    for ( i = 0; i < XEN_NR_ENTRIES(3); i++ )
> >> +    {
> >> +        vaddr_t va = XEN_VIRT_START + (i << PAGE_SHIFT);
> >> +        lpae_t *entry = xen_xenmap + i;
> >> +
> >> +        if ( !is_kernel(va) )
> >> +            break;
> >> +
> >> +        pte = read_atomic(entry);
> >> +
> >> +        if ( is_kernel_text(va) || is_kernel_inittext(va) )
> >> +        {
> >> +            pte.pt.xn = 0;
> >> +            pte.pt.ro = 1;
> >> +        } else if ( is_kernel_rodata(va) ) {
> >> +            pte.pt.ro = 1;
> >> +            pte.pt.xn = 1;
> >> +        } else {
> >> +            pte.pt.xn = 1;
> >> +            pte.pt.ro = 0;
> >> +        }
> >> +
> >> +        write_pte(entry, pte);
> >> +    }
> >> +
> >> +    /*
> >> +     * We modified live page-tables. Ensure the TBLs are invalidated
> >> +     * before setting enforcing the WnX permissions.
> >> +     */
> >> +    flush_xen_tlb_local();
> >>        xen_pt_enforce_wnx();
> >>
> >>    #ifdef CONFIG_ARM_32
> >
> > I understand what you're talking about, and it seems reasonable to get rid 
> > of
> > xen_colored_temp[] and create_llc_coloring_mappings() since in the end they
> > serve the purpose of mapping the physically colored space that is already
> > mapped using xen_xenmap[] pagetables.
> > What I don't understand is then how to copy/relocate Xen since I don't have 
> > a
> > destination virtual space anymore to use in relocate_xen().
>
> You will need to link xen_xenmap[] in boot_second[...] as well. With
> that, you will be able to access the new Xen through the temporary area.

Wouldn't it result in overwriting the current virtual space mapping?
boot_second is the live page table and if I link xen_xenmap[] then
XEN_VIRT_START would point to the new colored space which is still empty at
this stage...

> [...]
>
> >> Note that this means the init_ttbr cannot be written directly. But you
> >> can solve this problem by re-mapping the address.
> >
> > How to remap a single address?
>
> You should be able to use map_domain_page() to map the page where
> init_ttbr is.
>
> > And if moving init_ttbr in the identity-mapped area means that it's no 
> > longer
> > writable, so that I need to remap it, why moving it in that area in the 
> > first
> > place. Again I think I'm missing something.
>
> The goal is to have everything used (code, data) before the MMU is
> turned on residing in a single page. So secondary CPUs can directly jump
> to the colored Xen without any trouble.

This is what confuses me. Why having everything on a single page makes
secondary cpus able to jump directly to colored Xen? (also see below)

> >>>
> >>> 3) To access the identity mapping area I would need some accessor that 
> >>> takes
> >>> an address and returns it + phys_offset, or is there a better way to do 
> >>> it?
> >>
> >> I am not sure I understand what you mean. Can you clarify?
> >
> > In my idea, I would use the identity mapping to access the "old" variables,
> > where "old" means non physically colored. init_ttbr is an example. When
> > Xen it's copied on the new physical space, init_ttbr is copied with it and
> > if the boot cpu modifies this variable, it's actually touching the colored
> > one and not the old one. This means that secondary CPUs that still haven't
> > jumped to the new space, won't be able to see the new value and will never
> > go online.
> > So to access this "old" init_ttbr variable I need it's identity address,
> > which is its current virtual address + some physical offset. I was asking
> > you if this is the right approach to use the identity mapping.
>
> Secondary CPUs would directly start on the colored Xen. So they will be
> able to access the "new" init_ttbr & co.

How can this be true? I mean, in call_psci_cpu_on() I can start those CPUs in
the colored space, but they still use the boot_* pagetables and there I can't
easily link the new colored space, or, at least, I'm not succeding in doing
that. What I tried at the moment is to link xen_xenmap in boot_second after
switch_ttbr because of the problem I described above. But then secondary
CPUs never go online...

> [...]
>
> >> ... as I wrote ealier your current approach seems to have a flaw. As you
> >> overwrite xen_bootmodule->{start, size}. setup_mm() will end up to add
> >> the old Xen region to the boot allocator. This is before any secondary
> >> CPUs are booted up.
> >>
> >> IOW, the allocator may provide some memory from the old Xen and nothing
> >> good will happen from that.
> >>
> >> The only way to solve it is to add another module. So the memory is
> >> skipped by setup_mm(). However see below.
> >>
> >>>
> >>> Yes that should be memory that in the end would not be needed so it must
> >>> return to the boot-allocator (if that's what you mean). But how to do
> >>> that?
> >>
> >> You can't really discard the old temporary Xen. This may work today
> >> because we don't support CPU hotplug or suspend/resume. But there was
> >> some series on the ML to enable it and I don't see any reason why
> >> someone would not want to use the features with cache coloring.
> >>
> >> So the old temporary Xen would have to be kept around forever. This is
> >> up to 8MB of memory wasted.
> >>
> >> The right approach is to have the secondary CPU boot code (including the
> >> variables it is using) fitting in the same page (or possibly multiple so
> >> long this is small and physically contiguous). With that it doesn't
> >> matter where is the trampoline, it could stay at the old place, but we
> >> would only waste a few pages rather than up 8MB as it is today.
> >
> > So what are you suggesting is to create a new section in the linker script
> > for the trampoline code and data,
>
> We already have a section for that in place (see .idmap.*) which happens
> to be at the beginning of Xen. Right now, the section is in text. Which
> is why it is read-only executable.
>
> > then in setup_mm() we would skip this
> > memory?
>
> We should not need this. Secondary boot CPUs should boot direclty on the
> colored Xen.
>
> > Am I following you correctly? Sorry those topics are a little out
> > of my preparation as you probably already guessed.
>
> No worries. I am happy to go in as much details as necessary. I can also
> attempt to write a patch if that helps. (unless someone else in the Arm
> maintainers want to give a try).

Yes this would help. Thanks.

>
> Cheers,
>
> --
> Julien Grall