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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Julien,

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().

> >
> >> And if yes, then is it necessary
> >> to be kept the boot as completed?
> >
> > Nope. __initdata?
>
> Yes.
>
> [...]
>
> >> It feels wrong to keep the full Xen (even temporarily) just for CPU
> >> bring-up. But I don't think this is necessary. The secondary CPUs
> >> outside of code in head.S, secondary CPU should only need to access to
> >> init_ttbr and smp_cpu_up.
> >>
> >> The last one is already questionable because the CPU should never wait
> >> in Xen. Instead they would be held somewhere else. But that's separate
> >> issue.
> >>
> >> Anyway, if you move init_ttbr and smp_cpu_up in the identity mapped
> >> area, then you will not need to copy of Xen. Instead, secondary CPUs
> >> should be able to jump to the new Xen directly.
> >
> > So to recap:
> >
> > 1) How to move variables in the identity map area?
> > __attribute__((section(".text.idmap"))) triggers some warning when 
> > assembling.
> >
> > Warning: setting incorrect section attributes for .text.idmap
> >
> > 2) If I'm not mistaken the identity mapping is read only 
> > (PAGE_HYPERVISOR_RX)
> > and forcing it to be PAGE_HYPERVISOR_RW breaks something else.
> The warning above has nothing to do with the attributes used in the
> page-tables. It is telling you have multiple .text.idmap section with
> different attributes.
>
> There are a couple of ways to solve it:
>     1. Define init_ttbr in head.S
>     2. Use a different section (e.g. .data.idmap) and add it in the linker.

First one seems the easiest.

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

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

> >
> > 4) Maybe I misinterpreted the above comment, but I would still need to copy
> > Xen in the physically colored space. What I can drop is the temporary 
> > virtual
> > space used to access the "old" variables.
>
> Correct.
>
> >
> > 5) The identity mapping at runtime, at the moment, is pointing to the new
> > colored space because of how pte_of_xenaddr is implemented. This means that 
> > if
> > I want to use it to access the old variables, I would need to keep it a real
> > identity mapping, right?
>
> Why would you need to access the old variables?

I hope the above comment is clear enough to answer this point.

> >> This will also avoid to spread cache coloring changes in every Xen
> >> components.
> >
> > Maybe I'm missing something, but even with this identity mapping "shortcut" 
> > I
> > would still need to touch the same amount of files, for example when 
> > init_ttbr
> > or smp_up_cpu are accessed, they would need to use identity virtual 
> > addresses.
>
> My point was not related to the amount of files you are touching. But
> the number of ...
>
> >
> >>> +    if ( llc_coloring_enabled )
>
> ... if ( llc_coloring_enabled ) you sprinkle in Xen. I would really like
> to reduce to the strict minimum. Also...
>
> [...]
>
> >>> @@ -751,8 +899,13 @@ void asmlinkage __init start_xen(unsigned long 
> >>> boot_phys_offset,
> >>>        {
> >>>            if ( !llc_coloring_init() )
> >>>                panic("Xen LLC coloring support: setup failed\n");
> >>> +        xen_bootmodule->size = xen_colored_map_size(_end - _start);
> >>> +        xen_bootmodule->start = get_xen_paddr(xen_bootmodule->size);
> >>
> >> As you update xen_bootmodule, wouldn't this mean that the non-relocated >> 
> >> Xen would could be passed to the bootallocator?
>
> ... 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, then in setup_mm() we would skip this
memory? Am I following you correctly? Sorry those topics are a little out
of my preparation as you probably already guessed.

> Cheers,
>
> --
> Julien Grall

Thanks.