| Message ID | 20240102095138.17933-14-carlo.nonato@minervasys.tech (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Arm cache coloring | expand |
Hi Carlo, On 02/01/2024 09:51, Carlo Nonato wrote: > This commit adds the cache coloring support for Xen own physical space. > > It extends the implementation of setup_pagetables() to make use of Xen > cache coloring configuration. Page tables construction is essentially the > same except for the fact that PTEs point to a new temporary mapped, > physically colored space. > > The temporary mapping is also used to relocate Xen to the new physical > space starting at the address taken from the old get_xen_paddr() function > which is brought back for the occasion. > The temporary mapping is finally converted to a mapping of the "old" > (meaning the original physical space) Xen code, so that the boot CPU can > actually address the variables and functions used by secondary CPUs until > they enable the MMU. This happens when the boot CPU needs to bring up other > CPUs (psci.c and smpboot.c) and when the TTBR value is passed to them > (prepare_secondary_mm()). > > Finally, since the alternative framework needs to remap the Xen text and > inittext sections, this operation must be done in a coloring-aware way. > The function xen_remap_colored() is introduced for that. > > Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech> > Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech> > --- > v5: > - FIXME: consider_modules copy pasted since it got moved > v4: > - removed set_value_for_secondary() because it was wrongly cleaning cache > - relocate_xen() now calls switch_ttbr_id() > --- > xen/arch/arm/alternative.c | 9 +- > xen/arch/arm/arm64/mmu/head.S | 48 +++++++ > xen/arch/arm/arm64/mmu/mm.c | 26 +++- > xen/arch/arm/include/asm/llc-coloring.h | 16 +++ > xen/arch/arm/include/asm/mm.h | 7 +- > xen/arch/arm/llc-coloring.c | 44 +++++++ > xen/arch/arm/mmu/setup.c | 82 +++++++++++- > xen/arch/arm/mmu/smpboot.c | 11 +- > xen/arch/arm/psci.c | 9 +- > xen/arch/arm/setup.c | 165 +++++++++++++++++++++++- > xen/arch/arm/smpboot.c | 9 +- This patch has is touching a lot of different components. I think this want to be split in more smaller chunk. It would also be help to mention what code has been copied from previous Xen. For instance, relocate_xen() is clearly a copy of f60658c6ae47. > 11 files changed, 406 insertions(+), 20 deletions(-) > > diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c > index 016e66978b..54cbc2afad 100644 > --- a/xen/arch/arm/alternative.c > +++ b/xen/arch/arm/alternative.c > @@ -9,6 +9,7 @@ > #include <xen/init.h> > #include <xen/types.h> > #include <xen/kernel.h> > +#include <xen/llc-coloring.h> > #include <xen/mm.h> > #include <xen/vmap.h> > #include <xen/smp.h> > @@ -209,8 +210,12 @@ void __init apply_alternatives_all(void) > * The text and inittext section are read-only. So re-map Xen to > * be able to patch the code. > */ > - xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR, > - VMAP_DEFAULT); > + if ( llc_coloring_enabled ) > + xenmap = xen_remap_colored(xen_mfn, xen_size); > + else > + xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR, > + VMAP_DEFAULT); > + > /* Re-mapping Xen is not expected to fail during boot. */ > BUG_ON(!xenmap); > > diff --git a/xen/arch/arm/arm64/mmu/head.S b/xen/arch/arm/arm64/mmu/head.S > index 10774f30e4..6f0cc72897 100644 > --- a/xen/arch/arm/arm64/mmu/head.S > +++ b/xen/arch/arm/arm64/mmu/head.S > @@ -419,6 +419,54 @@ fail: PRINT("- Boot failed -\r\n") > b 1b > ENDPROC(fail) > > +/* Copy Xen to new location and switch TTBR > + * x0 ttbr > + * x1 source address > + * x2 destination address > + * x3 length > + * > + * Source and destination must be word aligned, length is rounded up > + * to a 16 byte boundary. > + * > + * MUST BE VERY CAREFUL when saving things to RAM over the copy */ If you plan to re-introduce code, then please at least make sure it match the coding style. For comments, it should be: /* * Foo * Bar */ > +ENTRY(relocate_xen) > + /* Copy 16 bytes at a time using: > + * x9: counter > + * x10: data > + * x11: data > + * x12: source > + * x13: destination > + */ > + mov x9, x3 > + mov x12, x1 > + mov x13, x2 > + > +1: ldp x10, x11, [x12], #16 > + stp x10, x11, [x13], #16 > + > + subs x9, x9, #16 > + bgt 1b > + > + /* Flush destination from dcache using: I would explain why you need the flush. AFAICT, this is because you want the data to be visible to the instruction cache. I would also point out that you need the instruction cache flush in switch_ttbr_id() where the sentence "This should not be necessary ..." should be now reworked (AFAIK it is mandatory for cache coloring). > + * x9: counter > + * x10: step > + * x11: vaddr > + */ > + dsb sy /* So the CPU issues all writes to the range */ > + > + mov x9, x3 > + ldr x10, =dcache_line_bytes /* x10 := step */ > + ldr x10, [x10] > + mov x11, x2 > + > +1: dc cvac, x11 > + > + add x11, x11, x10 > + subs x9, x9, x10 > + bgt 1b > + I would add a comment here explaining you are relying on the dsb/isb in switch_ttbr_id(). > + b switch_ttbr_id > + > /* > * Switch TTBR > * > diff --git a/xen/arch/arm/arm64/mmu/mm.c b/xen/arch/arm/arm64/mmu/mm.c > index d2651c9486..5a26d64ab7 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 */ > > #include <xen/init.h> > +#include <xen/llc-coloring.h> > #include <xen/mm.h> > #include <xen/pfn.h> > > @@ -125,27 +126,44 @@ void update_identity_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) > { > - vaddr_t id_addr = virt_to_maddr(switch_ttbr_id); > - switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr; > + vaddr_t vaddr, id_addr; > lpae_t pte; > > + if ( llc_coloring_enabled ) > + vaddr = (vaddr_t)relocate_xen; > + else > + vaddr = (vaddr_t)switch_ttbr_id; > + > + id_addr = virt_to_maddr(vaddr); > + > /* 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)switch_ttbr_id); > + pte = pte_of_xenaddr(vaddr); > pte.pt.table = 1; > pte.pt.xn = 0; > pte.pt.ro = 1; > write_pte(&xen_third_id[third_table_offset(id_addr)], pte); > > /* Switch TTBR */ > - fn(ttbr); > + if ( llc_coloring_enabled ) > + { > + relocate_xen_fn *fn = (relocate_xen_fn *)id_addr; Coding style: We tend to add a new line after variable declaration. > + fn(ttbr, _start, (void *)BOOT_RELOC_VIRT_START, _end - _start); > + } > + else > + { > + switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr; Ditto for the coding style. > + fn(ttbr); > + } > > /* > * Disable the identity mapping in the runtime page tables. > diff --git a/xen/arch/arm/include/asm/llc-coloring.h b/xen/arch/arm/include/asm/llc-coloring.h > index 5f9b0a8121..4d6071e50b 100644 > --- a/xen/arch/arm/include/asm/llc-coloring.h > +++ b/xen/arch/arm/include/asm/llc-coloring.h > @@ -12,11 +12,27 @@ > #define __ASM_ARM_COLORING_H__ > > #include <xen/init.h> > +#include <xen/mm-frame.h> > + > +/** > + * Iterate over each Xen mfn in the colored space. > + * @mfn: the current mfn. The first non colored mfn must be provided as the > + * starting point. > + * @i: loop index. > + */ > +#define for_each_xen_colored_mfn(mfn, i) \ > + for ( i = 0, mfn = xen_colored_mfn(mfn); \ > + i < (_end - _start) >> PAGE_SHIFT; \ > + i++, mfn = xen_colored_mfn(mfn_add(mfn, 1)) ) > > bool __init llc_coloring_init(void); > int dom0_set_llc_colors(struct domain *d); > int domain_set_llc_colors_from_str(struct domain *d, const char *str); > > +paddr_t xen_colored_map_size(paddr_t size); > +mfn_t xen_colored_mfn(mfn_t mfn); > +void *xen_remap_colored(mfn_t xen_fn, paddr_t xen_size); > + > #endif /* __ASM_ARM_COLORING_H__ */ > > /* > diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h > index 1829c559d6..311f092cf2 100644 > --- a/xen/arch/arm/include/asm/mm.h > +++ b/xen/arch/arm/include/asm/mm.h > @@ -203,12 +203,17 @@ extern unsigned long frametable_base_pdx; > > #define PDX_GROUP_SHIFT SECOND_SHIFT > > +#define virt_to_reloc_virt(virt) \ > + (((vaddr_t)virt) - XEN_VIRT_START + BOOT_RELOC_VIRT_START) > + > /* Boot-time pagetable setup */ > -extern void setup_pagetables(unsigned long boot_phys_offset); > +extern void setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr); > /* Map FDT in boot pagetable */ > extern void *early_fdt_map(paddr_t fdt_paddr); > /* Remove early mappings */ > extern void remove_early_mappings(void); > +/* Remove early LLC coloring mappings */ > +extern void remove_llc_coloring_mappings(void); > /* Prepare the memory subystem to bring-up the given secondary CPU */ > extern int prepare_secondary_mm(int cpu); > /* Map a frame table to cover physical addresses ps through pe */ > diff --git a/xen/arch/arm/llc-coloring.c b/xen/arch/arm/llc-coloring.c > index 99ea69ad39..f434efc45b 100644 > --- a/xen/arch/arm/llc-coloring.c > +++ b/xen/arch/arm/llc-coloring.c > @@ -14,6 +14,7 @@ > #include <xen/llc-coloring.h> > #include <xen/param.h> > #include <xen/types.h> > +#include <xen/vmap.h> > > #include <asm/processor.h> > #include <asm/sysregs.h> > @@ -38,6 +39,7 @@ static unsigned int __ro_after_init xen_num_colors; > > #define mfn_color_mask (nr_colors - 1) > #define mfn_to_color(mfn) (mfn_x(mfn) & mfn_color_mask) > +#define mfn_set_color(mfn, color) ((mfn_x(mfn) & ~mfn_color_mask) | (color)) > > /* > * Parse the coloring configuration given in the buf string, following the > @@ -354,6 +356,48 @@ unsigned int get_nr_llc_colors(void) > return nr_colors; > } > > +paddr_t xen_colored_map_size(paddr_t size) > +{ > + return ROUNDUP(size * nr_colors, XEN_PADDR_ALIGN); > +} > + > +mfn_t xen_colored_mfn(mfn_t mfn) Is this going to be used outside of boot? If not, then please add __init. If yes, then can you point me where? > +{ > + unsigned int i, color = mfn_to_color(mfn); > + > + for( i = 0; i < xen_num_colors; i++ ) > + { > + if ( color == xen_colors[i] ) > + return mfn; > + else if ( color < xen_colors[i] ) > + return mfn_set_color(mfn, xen_colors[i]); > + } > + > + /* Jump to next color space (nr_colors mfns) and use the first color */ > + return mfn_set_color(mfn_add(mfn, nr_colors), xen_colors[0]); > +} > + > +void *xen_remap_colored(mfn_t xen_mfn, paddr_t xen_size) I think this function can be __init. > +{ > + unsigned int i; > + void *xenmap; > + mfn_t *xen_colored_mfns; > + > + xen_colored_mfns = xmalloc_array(mfn_t, xen_size >> PAGE_SHIFT); > + if ( !xen_colored_mfns ) > + panic("Can't allocate LLC colored MFNs\n"); Let's try to limit the number of panic(). In this case, I think you should return NULL and let the caller decide. > + > + for_each_xen_colored_mfn( xen_mfn, i ) > + { > + xen_colored_mfns[i] = xen_mfn; > + } > + > + xenmap = vmap(xen_colored_mfns, xen_size >> PAGE_SHIFT); > + xfree(xen_colored_mfns); > + > + return xenmap; > +} > + > /* > * Local variables: > * mode: C > 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? And if yes, then is it necessary to be kept the boot as completed? Also, this is not going to be enough to cover Xen. See above. > + > /* Common pagetable leaves */ > /* Second level page table used to cover Xen virtual address space */ > static DEFINE_PAGE_TABLE(xen_second); > @@ -130,7 +135,12 @@ static void __init __maybe_unused build_assertions(void) > > lpae_t __init pte_of_xenaddr(vaddr_t va) > { > - paddr_t ma = va + phys_offset; > + paddr_t ma; > + > + if ( llc_coloring_enabled ) > + ma = virt_to_maddr(virt_to_reloc_virt(va)); > + else > + ma = va + phys_offset; > > return mfn_to_xen_entry(maddr_to_mfn(ma), MT_NORMAL); > } > @@ -216,11 +226,55 @@ static void xen_pt_enforce_wnx(void) > flush_xen_tlb_local(); > } > > +#ifdef CONFIG_LLC_COLORING > +static void __init create_llc_coloring_mappings(paddr_t xen_paddr) > +{ > + lpae_t pte; > + unsigned int i; > + mfn_t mfn = maddr_to_mfn(xen_paddr); > + > + for_each_xen_colored_mfn( mfn, i ) > + { > + pte = mfn_to_xen_entry(mfn, MT_NORMAL); > + pte.pt.table = 1; /* level 3 mappings always have this bit set */ > + xen_colored_temp[i] = pte; > + } > + > + pte = mfn_to_xen_entry(virt_to_mfn(xen_colored_temp), MT_NORMAL); > + pte.pt.table = 1; > + write_pte(&boot_second[second_table_offset(BOOT_RELOC_VIRT_START)], pte); > +} > + > +void __init remove_llc_coloring_mappings(void) > +{ > + int rc; > + > + /* destroy the _PAGE_BLOCK mapping */ > + rc = modify_xen_mappings(BOOT_RELOC_VIRT_START, > + BOOT_RELOC_VIRT_START + SZ_2M, See above, Xen can now be bigger than 2MB. The limit is 8MB and could change in the future. > + _PAGE_BLOCK); > + BUG_ON(rc); > +} > +#else > +static void __init create_llc_coloring_mappings(paddr_t xen_paddr) {} > +void __init remove_llc_coloring_mappings(void) {} Both should never be called when !CONFIG_LCC_COLORING, correct? If so, then please add ASSERT_UNREACHABLE() in their body. > +#endif /* CONFIG_LLC_COLORING */ > + > /* > - * Boot-time pagetable setup. > + * Boot-time pagetable setup with coloring support > * Changes here may need matching changes in head.S > + * > + * The coloring support consists of: > + * - Create a temporary colored mapping that conforms to Xen color selection. > + * - pte_of_xenaddr takes care of translating the virtual addresses to the > + * new colored physical space and the returns the pte, so that the page table > + * initialization can remain the same. > + * - Copy Xen to the new colored physical space by exploiting the temporary > + * mapping. > + * - Update TTBR0_EL2 with the new root page table address. > */ > -void __init setup_pagetables(unsigned long boot_phys_offset) > + > +void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr) > { > uint64_t ttbr; > lpae_t pte, *p; > @@ -228,6 +282,9 @@ void __init setup_pagetables(unsigned long boot_phys_offset) > > phys_offset = boot_phys_offset; > > + if ( llc_coloring_enabled ) > + create_llc_coloring_mappings(xen_paddr); > + > arch_setup_page_tables(); > > #ifdef CONFIG_ARM_64 > @@ -281,10 +338,13 @@ void __init setup_pagetables(unsigned long boot_phys_offset) > pte.pt.table = 1; > xen_second[second_table_offset(FIXMAP_ADDR(0))] = pte; > > + if ( llc_coloring_enabled ) > + ttbr = virt_to_maddr(virt_to_reloc_virt(xen_pgtable)); xen_pgtable is only valid for Arm64. But rather than ifdef-ing. I would consder to move... > + else > #ifdef CONFIG_ARM_64 > - ttbr = (uintptr_t) xen_pgtable + phys_offset; > + ttbr = (uintptr_t) xen_pgtable + phys_offset; > #else > - ttbr = (uintptr_t) cpu0_pgtable + phys_offset; > + ttbr = (uintptr_t) cpu0_pgtable + phys_offset; > #endif > > switch_ttbr(ttbr); > @@ -294,6 +354,18 @@ void __init setup_pagetables(unsigned long boot_phys_offset) > #ifdef CONFIG_ARM_32 > per_cpu(xen_pgtable, 0) = cpu0_pgtable; > #endif .. these two lines before hand so you can use THIS_CPU_PGTABLE. > + > + /* Coding style: It looks like you have one space too much before /*. > + * Keep original Xen memory mapped because secondary CPUs still point to it > + * and a few variables needs to be accessed by the master CPU in order to > + * let them boot. This mapping will also replace the one created at the > + * beginning of setup_pagetables. > + */ 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. This will also avoid to spread cache coloring changes in every Xen components. > + if ( llc_coloring_enabled ) > + map_pages_to_xen(BOOT_RELOC_VIRT_START, > + maddr_to_mfn(XEN_VIRT_START + phys_offset), > + SZ_2M >> PAGE_SHIFT, PAGE_HYPERVISOR_RW | _PAGE_BLOCK); > + > } > > void *__init arch_vmap_virt_end(void) > diff --git a/xen/arch/arm/mmu/smpboot.c b/xen/arch/arm/mmu/smpboot.c > index b6fc0aae07..a69183ec88 100644 > --- a/xen/arch/arm/mmu/smpboot.c > +++ b/xen/arch/arm/mmu/smpboot.c > @@ -6,6 +6,7 @@ > */ > > #include <xen/domain_page.h> > +#include <xen/llc-coloring.h> > > #include <asm/setup.h> > > @@ -71,14 +72,20 @@ static void clear_boot_pagetables(void) > #ifdef CONFIG_ARM_64 > int prepare_secondary_mm(int cpu) > { > + uint64_t *init_ttbr_addr = &init_ttbr; > + > clear_boot_pagetables(); > > + if ( llc_coloring_enabled ) > + init_ttbr_addr = (uint64_t *)virt_to_reloc_virt(&init_ttbr); > + > /* > * Set init_ttbr for this CPU coming up. All CPUs share a single setof > * pagetables, but rewrite it each time for consistency with 32 bit. > */ > - init_ttbr = virt_to_maddr(xen_pgtable); > - clean_dcache(init_ttbr); > + *init_ttbr_addr = virt_to_maddr(xen_pgtable); > + clean_dcache(*init_ttbr_addr); > + > return 0; > } > #else > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c > index 695d2fa1f1..23e298c477 100644 > --- a/xen/arch/arm/psci.c > +++ b/xen/arch/arm/psci.c > @@ -11,6 +11,7 @@ > > #include <xen/types.h> > #include <xen/init.h> > +#include <xen/llc-coloring.h> > #include <xen/mm.h> > #include <xen/smp.h> > #include <asm/cpufeature.h> > @@ -39,9 +40,13 @@ static uint32_t psci_cpu_on_nr; > int call_psci_cpu_on(int cpu) > { > struct arm_smccc_res res; > + vaddr_t init_secondary_addr = (vaddr_t)init_secondary; > > - arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary), > - &res); > + if ( llc_coloring_enabled ) > + init_secondary_addr = virt_to_reloc_virt(init_secondary); > + > + arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), > + __pa(init_secondary_addr), &res); > > return PSCI_RET(res); > } [...] > +#ifdef CONFIG_LLC_COLORING > +/** > + * get_xen_paddr - get physical address to relocate Xen to > + * > + * Xen is relocated to as near to the top of RAM as possible and > + * aligned to a XEN_PADDR_ALIGN boundary. > + */ > +static paddr_t __init get_xen_paddr(uint32_t xen_size) > +{ > + struct meminfo *mi = &bootinfo.mem; > + paddr_t min_size; > + paddr_t paddr = 0; > + int i; > + > + min_size = (xen_size + (XEN_PADDR_ALIGN-1)) & ~(XEN_PADDR_ALIGN-1); > + > + /* Find the highest bank with enough space. */ > + for ( i = 0; i < mi->nr_banks; i++ ) > + { > + const struct membank *bank = &mi->bank[i]; > + paddr_t s, e; > + > + if ( bank->size >= min_size ) > + { > + e = consider_modules(bank->start, bank->start + bank->size, > + min_size, XEN_PADDR_ALIGN, 0); > + if ( !e ) > + continue; > + > +#ifdef CONFIG_ARM_32 > + /* Xen must be under 4GB */ > + if ( e > 0x100000000ULL ) > + e = 0x100000000ULL; > + if ( e < bank->start ) > + continue; > +#endif > + > + s = e - min_size; > + > + if ( s > paddr ) > + paddr = s; > + } > + } > + > + if ( !paddr ) > + panic("Not enough memory to relocate Xen\n"); > + > + printk("Placing Xen at 0x%"PRIpaddr"-0x%"PRIpaddr"\n", > + paddr, paddr + min_size); > + > + return paddr; > +} > +#else > +static paddr_t __init get_xen_paddr(uint32_t xen_size) { return 0; } > +#endif > + > void __init init_pdx(void) > { > paddr_t bank_start, bank_size, bank_end; > @@ -724,8 +874,6 @@ void asmlinkage __init start_xen(unsigned long boot_phys_offset, > /* Initialize traps early allow us to get backtrace when an error occurred */ > init_traps(); > > - setup_pagetables(boot_phys_offset); > - > smp_clear_cpu_maps(); > > device_tree_flattened = early_fdt_map(fdt_paddr); > @@ -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? > } > > + setup_pagetables(boot_phys_offset, xen_bootmodule->start); The new placement of setup_pagetables() deserve an explanation. > + device_tree_flattened = early_fdt_map(fdt_paddr); > + > setup_mm(); > > /* Parse the ACPI tables for possible boot-time configuration */ > @@ -867,6 +1020,14 @@ void asmlinkage __init start_xen(unsigned long boot_phys_offset, > > setup_virt_paging(); > > + /* > + * The removal is done earlier than discard_initial_modules beacuse the Typo: s/beacuase/because/ > + * livepatch init uses a virtual address equal to BOOT_RELOC_VIRT_START. > + * Remove LLC coloring mappings to expose a clear state to the livepatch > + * module. > + */ > + if ( llc_coloring_enabled ) > + remove_llc_coloring_mappings(); > do_initcalls(); > > /* > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c > index 7110bc11fc..7ed7357d58 100644 > --- a/xen/arch/arm/smpboot.c > +++ b/xen/arch/arm/smpboot.c > @@ -14,6 +14,7 @@ > #include <xen/domain_page.h> > #include <xen/errno.h> > #include <xen/init.h> > +#include <xen/llc-coloring.h> > #include <xen/mm.h> > #include <xen/param.h> > #include <xen/sched.h> > @@ -444,6 +445,7 @@ int __cpu_up(unsigned int cpu) > { > int rc; > s_time_t deadline; > + unsigned long *smp_up_cpu_addr = &smp_up_cpu; > > printk("Bringing up CPU%d\n", cpu); > > @@ -459,9 +461,12 @@ int __cpu_up(unsigned int cpu) > /* Tell the remote CPU what its logical CPU ID is. */ > init_data.cpuid = cpu; > > + if ( llc_coloring_enabled ) > + smp_up_cpu_addr = (unsigned long *)virt_to_reloc_virt(&smp_up_cpu); > + > /* Open the gate for this CPU */ > - smp_up_cpu = cpu_logical_map(cpu); > - clean_dcache(smp_up_cpu); > + *smp_up_cpu_addr = cpu_logical_map(cpu); > + clean_dcache(*smp_up_cpu_addr); > > rc = arch_cpu_up(cpu); > Cheers,
Hi Julien, On Fri, Jan 5, 2024 at 8:12 PM Julien Grall <julien@xen.org> wrote: > > Hi Carlo, > > On 02/01/2024 09:51, Carlo Nonato wrote: > > This commit adds the cache coloring support for Xen own physical space. > > > > It extends the implementation of setup_pagetables() to make use of Xen > > cache coloring configuration. Page tables construction is essentially the > > same except for the fact that PTEs point to a new temporary mapped, > > physically colored space. > > > > The temporary mapping is also used to relocate Xen to the new physical > > space starting at the address taken from the old get_xen_paddr() function > > which is brought back for the occasion. > > The temporary mapping is finally converted to a mapping of the "old" > > (meaning the original physical space) Xen code, so that the boot CPU can > > actually address the variables and functions used by secondary CPUs until > > they enable the MMU. This happens when the boot CPU needs to bring up other > > CPUs (psci.c and smpboot.c) and when the TTBR value is passed to them > > (prepare_secondary_mm()). > > > > Finally, since the alternative framework needs to remap the Xen text and > > inittext sections, this operation must be done in a coloring-aware way. > > The function xen_remap_colored() is introduced for that. > > > > Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech> > > Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech> > > --- > > v5: > > - FIXME: consider_modules copy pasted since it got moved > > v4: > > - removed set_value_for_secondary() because it was wrongly cleaning cache > > - relocate_xen() now calls switch_ttbr_id() > > --- > > xen/arch/arm/alternative.c | 9 +- > > xen/arch/arm/arm64/mmu/head.S | 48 +++++++ > > xen/arch/arm/arm64/mmu/mm.c | 26 +++- > > xen/arch/arm/include/asm/llc-coloring.h | 16 +++ > > xen/arch/arm/include/asm/mm.h | 7 +- > > xen/arch/arm/llc-coloring.c | 44 +++++++ > > xen/arch/arm/mmu/setup.c | 82 +++++++++++- > > xen/arch/arm/mmu/smpboot.c | 11 +- > > xen/arch/arm/psci.c | 9 +- > > xen/arch/arm/setup.c | 165 +++++++++++++++++++++++- > > xen/arch/arm/smpboot.c | 9 +- > > This patch has is touching a lot of different components. I think this > want to be split in more smaller chunk. It would also be help to mention > what code has been copied from previous Xen. For instance, > relocate_xen() is clearly a copy of f60658c6ae47. > > > 11 files changed, 406 insertions(+), 20 deletions(-) > > > > diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c > > index 016e66978b..54cbc2afad 100644 > > --- a/xen/arch/arm/alternative.c > > +++ b/xen/arch/arm/alternative.c > > @@ -9,6 +9,7 @@ > > #include <xen/init.h> > > #include <xen/types.h> > > #include <xen/kernel.h> > > +#include <xen/llc-coloring.h> > > #include <xen/mm.h> > > #include <xen/vmap.h> > > #include <xen/smp.h> > > @@ -209,8 +210,12 @@ void __init apply_alternatives_all(void) > > * The text and inittext section are read-only. So re-map Xen to > > * be able to patch the code. > > */ > > - xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR, > > - VMAP_DEFAULT); > > + if ( llc_coloring_enabled ) > > + xenmap = xen_remap_colored(xen_mfn, xen_size); > > + else > > + xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR, > > + VMAP_DEFAULT); > > + > > /* Re-mapping Xen is not expected to fail during boot. */ > > BUG_ON(!xenmap); > > > > diff --git a/xen/arch/arm/arm64/mmu/head.S b/xen/arch/arm/arm64/mmu/head.S > > index 10774f30e4..6f0cc72897 100644 > > --- a/xen/arch/arm/arm64/mmu/head.S > > +++ b/xen/arch/arm/arm64/mmu/head.S > > @@ -419,6 +419,54 @@ fail: PRINT("- Boot failed -\r\n") > > b 1b > > ENDPROC(fail) > > > > +/* Copy Xen to new location and switch TTBR > > + * x0 ttbr > > + * x1 source address > > + * x2 destination address > > + * x3 length > > + * > > + * Source and destination must be word aligned, length is rounded up > > + * to a 16 byte boundary. > > + * > > + * MUST BE VERY CAREFUL when saving things to RAM over the copy */ > > If you plan to re-introduce code, then please at least make sure it > match the coding style. For comments, it should be: > > /* > * Foo > * Bar > */ > > > +ENTRY(relocate_xen) > > + /* Copy 16 bytes at a time using: > > + * x9: counter > > + * x10: data > > + * x11: data > > + * x12: source > > + * x13: destination > > + */ > > + mov x9, x3 > > + mov x12, x1 > > + mov x13, x2 > > + > > +1: ldp x10, x11, [x12], #16 > > + stp x10, x11, [x13], #16 > > + > > + subs x9, x9, #16 > > + bgt 1b > > + > > + /* Flush destination from dcache using: > > I would explain why you need the flush. AFAICT, this is because you want > the data to be visible to the instruction cache. I would also point out > that you need the instruction cache flush in switch_ttbr_id() where the > sentence "This should not be necessary ..." should be now reworked > (AFAIK it is mandatory for cache coloring). > > > + * x9: counter > > + * x10: step > > + * x11: vaddr > > + */ > > + dsb sy /* So the CPU issues all writes to the range */ > > + > > + mov x9, x3 > > + ldr x10, =dcache_line_bytes /* x10 := step */ > > + ldr x10, [x10] > > + mov x11, x2 > > + > > +1: dc cvac, x11 > > + > > + add x11, x11, x10 > > + subs x9, x9, x10 > > + bgt 1b > > + > > I would add a comment here explaining you are relying on the dsb/isb in > switch_ttbr_id(). > > > + b switch_ttbr_id > > + > > /* > > * Switch TTBR > > * > > diff --git a/xen/arch/arm/arm64/mmu/mm.c b/xen/arch/arm/arm64/mmu/mm.c > > index d2651c9486..5a26d64ab7 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 */ > > > > #include <xen/init.h> > > +#include <xen/llc-coloring.h> > > #include <xen/mm.h> > > #include <xen/pfn.h> > > > > @@ -125,27 +126,44 @@ void update_identity_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) > > { > > - vaddr_t id_addr = virt_to_maddr(switch_ttbr_id); > > - switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr; > > + vaddr_t vaddr, id_addr; > > lpae_t pte; > > > > + if ( llc_coloring_enabled ) > > + vaddr = (vaddr_t)relocate_xen; > > + else > > + vaddr = (vaddr_t)switch_ttbr_id; > > + > > + id_addr = virt_to_maddr(vaddr); > > + > > /* 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)switch_ttbr_id); > > + pte = pte_of_xenaddr(vaddr); > > pte.pt.table = 1; > > pte.pt.xn = 0; > > pte.pt.ro = 1; > > write_pte(&xen_third_id[third_table_offset(id_addr)], pte); > > > > /* Switch TTBR */ > > - fn(ttbr); > > + if ( llc_coloring_enabled ) > > + { > > + relocate_xen_fn *fn = (relocate_xen_fn *)id_addr; > > Coding style: We tend to add a new line after variable declaration. > > > + fn(ttbr, _start, (void *)BOOT_RELOC_VIRT_START, _end - _start); > > + } > > + else > > + { > > + switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr; > > Ditto for the coding style. > > > + fn(ttbr); > > + } > > > > /* > > * Disable the identity mapping in the runtime page tables. > > diff --git a/xen/arch/arm/include/asm/llc-coloring.h b/xen/arch/arm/include/asm/llc-coloring.h > > index 5f9b0a8121..4d6071e50b 100644 > > --- a/xen/arch/arm/include/asm/llc-coloring.h > > +++ b/xen/arch/arm/include/asm/llc-coloring.h > > @@ -12,11 +12,27 @@ > > #define __ASM_ARM_COLORING_H__ > > > > #include <xen/init.h> > > +#include <xen/mm-frame.h> > > + > > +/** > > + * Iterate over each Xen mfn in the colored space. > > + * @mfn: the current mfn. The first non colored mfn must be provided as the > > + * starting point. > > + * @i: loop index. > > + */ > > +#define for_each_xen_colored_mfn(mfn, i) \ > > + for ( i = 0, mfn = xen_colored_mfn(mfn); \ > > + i < (_end - _start) >> PAGE_SHIFT; \ > > + i++, mfn = xen_colored_mfn(mfn_add(mfn, 1)) ) > > > > bool __init llc_coloring_init(void); > > int dom0_set_llc_colors(struct domain *d); > > int domain_set_llc_colors_from_str(struct domain *d, const char *str); > > > > +paddr_t xen_colored_map_size(paddr_t size); > > +mfn_t xen_colored_mfn(mfn_t mfn); > > +void *xen_remap_colored(mfn_t xen_fn, paddr_t xen_size); > > + > > #endif /* __ASM_ARM_COLORING_H__ */ > > > > /* > > diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h > > index 1829c559d6..311f092cf2 100644 > > --- a/xen/arch/arm/include/asm/mm.h > > +++ b/xen/arch/arm/include/asm/mm.h > > @@ -203,12 +203,17 @@ extern unsigned long frametable_base_pdx; > > > > #define PDX_GROUP_SHIFT SECOND_SHIFT > > > > +#define virt_to_reloc_virt(virt) \ > > + (((vaddr_t)virt) - XEN_VIRT_START + BOOT_RELOC_VIRT_START) > > + > > /* Boot-time pagetable setup */ > > -extern void setup_pagetables(unsigned long boot_phys_offset); > > +extern void setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr); > > /* Map FDT in boot pagetable */ > > extern void *early_fdt_map(paddr_t fdt_paddr); > > /* Remove early mappings */ > > extern void remove_early_mappings(void); > > +/* Remove early LLC coloring mappings */ > > +extern void remove_llc_coloring_mappings(void); > > /* Prepare the memory subystem to bring-up the given secondary CPU */ > > extern int prepare_secondary_mm(int cpu); > > /* Map a frame table to cover physical addresses ps through pe */ > > diff --git a/xen/arch/arm/llc-coloring.c b/xen/arch/arm/llc-coloring.c > > index 99ea69ad39..f434efc45b 100644 > > --- a/xen/arch/arm/llc-coloring.c > > +++ b/xen/arch/arm/llc-coloring.c > > @@ -14,6 +14,7 @@ > > #include <xen/llc-coloring.h> > > #include <xen/param.h> > > #include <xen/types.h> > > +#include <xen/vmap.h> > > > > #include <asm/processor.h> > > #include <asm/sysregs.h> > > @@ -38,6 +39,7 @@ static unsigned int __ro_after_init xen_num_colors; > > > > #define mfn_color_mask (nr_colors - 1) > > #define mfn_to_color(mfn) (mfn_x(mfn) & mfn_color_mask) > > +#define mfn_set_color(mfn, color) ((mfn_x(mfn) & ~mfn_color_mask) | (color)) > > > > /* > > * Parse the coloring configuration given in the buf string, following the > > @@ -354,6 +356,48 @@ unsigned int get_nr_llc_colors(void) > > return nr_colors; > > } > > > > +paddr_t xen_colored_map_size(paddr_t size) > > +{ > > + return ROUNDUP(size * nr_colors, XEN_PADDR_ALIGN); > > +} > > + > +mfn_t xen_colored_mfn(mfn_t mfn) > > Is this going to be used outside of boot? If not, then please add > __init. If yes, then can you point me where? > > > +{ > > + unsigned int i, color = mfn_to_color(mfn); > > + > > + for( i = 0; i < xen_num_colors; i++ ) > > + { > > + if ( color == xen_colors[i] ) > > + return mfn; > > + else if ( color < xen_colors[i] ) > > + return mfn_set_color(mfn, xen_colors[i]); > > + } > > + > > + /* Jump to next color space (nr_colors mfns) and use the first color */ > > + return mfn_set_color(mfn_add(mfn, nr_colors), xen_colors[0]); > > +} > > + > > +void *xen_remap_colored(mfn_t xen_mfn, paddr_t xen_size) > > I think this function can be __init. > > > +{ > > + unsigned int i; > > + void *xenmap; > > + mfn_t *xen_colored_mfns; > > + > > + xen_colored_mfns = xmalloc_array(mfn_t, xen_size >> PAGE_SHIFT); > > + if ( !xen_colored_mfns ) > > + panic("Can't allocate LLC colored MFNs\n"); > Let's try to limit the number of panic(). In this case, I think you > should return NULL and let the caller decide. > > > + > > + for_each_xen_colored_mfn( xen_mfn, i ) > > + { > > + xen_colored_mfns[i] = xen_mfn; > > + } > > + > > + xenmap = vmap(xen_colored_mfns, xen_size >> PAGE_SHIFT); > > + xfree(xen_colored_mfns); > > + > > + return xenmap; > > +} > > + > > /* > > * Local variables: > > * mode: C > > 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. > And if yes, then is it necessary > to be kept the boot as completed? Nope. __initdata? > Also, this is not going to be enough to cover Xen. See above. > > > > + > > /* Common pagetable leaves */ > > /* Second level page table used to cover Xen virtual address space */ > > static DEFINE_PAGE_TABLE(xen_second); > > @@ -130,7 +135,12 @@ static void __init __maybe_unused build_assertions(void) > > > > lpae_t __init pte_of_xenaddr(vaddr_t va) > > { > > - paddr_t ma = va + phys_offset; > > + paddr_t ma; > > + > > + if ( llc_coloring_enabled ) > > + ma = virt_to_maddr(virt_to_reloc_virt(va)); > > + else > > + ma = va + phys_offset; > > > > return mfn_to_xen_entry(maddr_to_mfn(ma), MT_NORMAL); > > } > > @@ -216,11 +226,55 @@ static void xen_pt_enforce_wnx(void) > > flush_xen_tlb_local(); > > } > > > > +#ifdef CONFIG_LLC_COLORING > > +static void __init create_llc_coloring_mappings(paddr_t xen_paddr) > > +{ > > + lpae_t pte; > > + unsigned int i; > > + mfn_t mfn = maddr_to_mfn(xen_paddr); > > + > > + for_each_xen_colored_mfn( mfn, i ) > > + { > > + pte = mfn_to_xen_entry(mfn, MT_NORMAL); > > + pte.pt.table = 1; /* level 3 mappings always have this bit set */ > > + xen_colored_temp[i] = pte; > > + } > > + > > + pte = mfn_to_xen_entry(virt_to_mfn(xen_colored_temp), MT_NORMAL); > > + pte.pt.table = 1; > > + write_pte(&boot_second[second_table_offset(BOOT_RELOC_VIRT_START)], pte); > > +} > > + > > +void __init remove_llc_coloring_mappings(void) > > +{ > > + int rc; > > + > > + /* destroy the _PAGE_BLOCK mapping */ > > + rc = modify_xen_mappings(BOOT_RELOC_VIRT_START, > > + BOOT_RELOC_VIRT_START + SZ_2M, > > See above, Xen can now be bigger than 2MB. The limit is 8MB and could > change in the future. > > > + _PAGE_BLOCK); > > + BUG_ON(rc); > > +} > > +#else > > +static void __init create_llc_coloring_mappings(paddr_t xen_paddr) {} > > +void __init remove_llc_coloring_mappings(void) {} > > Both should never be called when !CONFIG_LCC_COLORING, correct? If so, > then please add ASSERT_UNREACHABLE() in their body. > > > +#endif /* CONFIG_LLC_COLORING */ > > + > > /* > > - * Boot-time pagetable setup. > > + * Boot-time pagetable setup with coloring support > > * Changes here may need matching changes in head.S > > + * > > + * The coloring support consists of: > > + * - Create a temporary colored mapping that conforms to Xen color selection. > > + * - pte_of_xenaddr takes care of translating the virtual addresses to the > > + * new colored physical space and the returns the pte, so that the page table > > + * initialization can remain the same. > > + * - Copy Xen to the new colored physical space by exploiting the temporary > > + * mapping. > > + * - Update TTBR0_EL2 with the new root page table address. > > */ > > -void __init setup_pagetables(unsigned long boot_phys_offset) > > + > > +void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr) > > { > > uint64_t ttbr; > > lpae_t pte, *p; > > @@ -228,6 +282,9 @@ void __init setup_pagetables(unsigned long boot_phys_offset) > > > > phys_offset = boot_phys_offset; > > > > + if ( llc_coloring_enabled ) > > + create_llc_coloring_mappings(xen_paddr); > > + > > arch_setup_page_tables(); > > > > #ifdef CONFIG_ARM_64 > > @@ -281,10 +338,13 @@ void __init setup_pagetables(unsigned long boot_phys_offset) > > pte.pt.table = 1; > > xen_second[second_table_offset(FIXMAP_ADDR(0))] = pte; > > > > + if ( llc_coloring_enabled ) > > + ttbr = virt_to_maddr(virt_to_reloc_virt(xen_pgtable)); > > xen_pgtable is only valid for Arm64. But rather than ifdef-ing. I would > consder to move... > > > + else > > #ifdef CONFIG_ARM_64 > > - ttbr = (uintptr_t) xen_pgtable + phys_offset; > > + ttbr = (uintptr_t) xen_pgtable + phys_offset; > > #else > > - ttbr = (uintptr_t) cpu0_pgtable + phys_offset; > > + ttbr = (uintptr_t) cpu0_pgtable + phys_offset; > > #endif > > > > switch_ttbr(ttbr); > > @@ -294,6 +354,18 @@ void __init setup_pagetables(unsigned long boot_phys_offset) > > #ifdef CONFIG_ARM_32 > > per_cpu(xen_pgtable, 0) = cpu0_pgtable; > > #endif > > .. these two lines before hand so you can use THIS_CPU_PGTABLE. > > > + > > + /* > > Coding style: It looks like you have one space too much before /*. > > > + * Keep original Xen memory mapped because secondary CPUs still point to it > > + * and a few variables needs to be accessed by the master CPU in order to > > + * let them boot. This mapping will also replace the one created at the > > + * beginning of setup_pagetables. > > + */ > > 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. 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? 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. 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? > 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. > > + if ( llc_coloring_enabled ) > > + map_pages_to_xen(BOOT_RELOC_VIRT_START, > > + maddr_to_mfn(XEN_VIRT_START + phys_offset), > > + SZ_2M >> PAGE_SHIFT, PAGE_HYPERVISOR_RW | _PAGE_BLOCK); > > + > > } > > > > void *__init arch_vmap_virt_end(void) > > diff --git a/xen/arch/arm/mmu/smpboot.c b/xen/arch/arm/mmu/smpboot.c > > index b6fc0aae07..a69183ec88 100644 > > --- a/xen/arch/arm/mmu/smpboot.c > > +++ b/xen/arch/arm/mmu/smpboot.c > > @@ -6,6 +6,7 @@ > > */ > > > > #include <xen/domain_page.h> > > +#include <xen/llc-coloring.h> > > > > #include <asm/setup.h> > > > > @@ -71,14 +72,20 @@ static void clear_boot_pagetables(void) > > #ifdef CONFIG_ARM_64 > > int prepare_secondary_mm(int cpu) > > { > > + uint64_t *init_ttbr_addr = &init_ttbr; > > + > > clear_boot_pagetables(); > > > > + if ( llc_coloring_enabled ) > > + init_ttbr_addr = (uint64_t *)virt_to_reloc_virt(&init_ttbr); > > + > > /* > > * Set init_ttbr for this CPU coming up. All CPUs share a single setof > > * pagetables, but rewrite it each time for consistency with 32 bit. > > */ > > - init_ttbr = virt_to_maddr(xen_pgtable); > > - clean_dcache(init_ttbr); > > + *init_ttbr_addr = virt_to_maddr(xen_pgtable); > > + clean_dcache(*init_ttbr_addr); > > + > > return 0; > > } > > #else > > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c > > index 695d2fa1f1..23e298c477 100644 > > --- a/xen/arch/arm/psci.c > > +++ b/xen/arch/arm/psci.c > > @@ -11,6 +11,7 @@ > > > > #include <xen/types.h> > > #include <xen/init.h> > > +#include <xen/llc-coloring.h> > > #include <xen/mm.h> > > #include <xen/smp.h> > > #include <asm/cpufeature.h> > > @@ -39,9 +40,13 @@ static uint32_t psci_cpu_on_nr; > > int call_psci_cpu_on(int cpu) > > { > > struct arm_smccc_res res; > > + vaddr_t init_secondary_addr = (vaddr_t)init_secondary; > > > > - arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary), > > - &res); > > + if ( llc_coloring_enabled ) > > + init_secondary_addr = virt_to_reloc_virt(init_secondary); > > + > > + arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), > > + __pa(init_secondary_addr), &res); > > > > return PSCI_RET(res); > > } > > [...] > > > +#ifdef CONFIG_LLC_COLORING > > +/** > > + * get_xen_paddr - get physical address to relocate Xen to > > + * > > + * Xen is relocated to as near to the top of RAM as possible and > > + * aligned to a XEN_PADDR_ALIGN boundary. > > + */ > > +static paddr_t __init get_xen_paddr(uint32_t xen_size) > > +{ > > + struct meminfo *mi = &bootinfo.mem; > > + paddr_t min_size; > > + paddr_t paddr = 0; > > + int i; > > + > > + min_size = (xen_size + (XEN_PADDR_ALIGN-1)) & ~(XEN_PADDR_ALIGN-1); > > + > > + /* Find the highest bank with enough space. */ > > + for ( i = 0; i < mi->nr_banks; i++ ) > > + { > > + const struct membank *bank = &mi->bank[i]; > > + paddr_t s, e; > > + > > + if ( bank->size >= min_size ) > > + { > > + e = consider_modules(bank->start, bank->start + bank->size, > > + min_size, XEN_PADDR_ALIGN, 0); > > + if ( !e ) > > + continue; > > + > > +#ifdef CONFIG_ARM_32 > > + /* Xen must be under 4GB */ > > + if ( e > 0x100000000ULL ) > > + e = 0x100000000ULL; > > + if ( e < bank->start ) > > + continue; > > +#endif > > + > > + s = e - min_size; > > + > > + if ( s > paddr ) > > + paddr = s; > > + } > > + } > > + > > + if ( !paddr ) > > + panic("Not enough memory to relocate Xen\n"); > > + > > + printk("Placing Xen at 0x%"PRIpaddr"-0x%"PRIpaddr"\n", > > + paddr, paddr + min_size); > > + > > + return paddr; > > +} > > +#else > > +static paddr_t __init get_xen_paddr(uint32_t xen_size) { return 0; } > > +#endif > > + > > void __init init_pdx(void) > > { > > paddr_t bank_start, bank_size, bank_end; > > @@ -724,8 +874,6 @@ void asmlinkage __init start_xen(unsigned long boot_phys_offset, > > /* Initialize traps early allow us to get backtrace when an error occurred */ > > init_traps(); > > > > - setup_pagetables(boot_phys_offset); > > - > > smp_clear_cpu_maps(); > > > > device_tree_flattened = early_fdt_map(fdt_paddr); > > @@ -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? 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? > > } > > > + setup_pagetables(boot_phys_offset, xen_bootmodule->start); > > The new placement of setup_pagetables() deserve an explanation. > > > + device_tree_flattened = early_fdt_map(fdt_paddr); > > + > > setup_mm(); > > > > /* Parse the ACPI tables for possible boot-time configuration */ > > @@ -867,6 +1020,14 @@ void asmlinkage __init start_xen(unsigned long boot_phys_offset, > > > > setup_virt_paging(); > > > > + /* > > + * The removal is done earlier than discard_initial_modules beacuse the > > Typo: s/beacuase/because/ > > > + * livepatch init uses a virtual address equal to BOOT_RELOC_VIRT_START. > > + * Remove LLC coloring mappings to expose a clear state to the livepatch > > + * module. > > + */ > > + if ( llc_coloring_enabled ) > > + remove_llc_coloring_mappings(); > > do_initcalls(); > > > > /* > > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c > > index 7110bc11fc..7ed7357d58 100644 > > --- a/xen/arch/arm/smpboot.c > > +++ b/xen/arch/arm/smpboot.c > > @@ -14,6 +14,7 @@ > > #include <xen/domain_page.h> > > #include <xen/errno.h> > > #include <xen/init.h> > > +#include <xen/llc-coloring.h> > > #include <xen/mm.h> > > #include <xen/param.h> > > #include <xen/sched.h> > > @@ -444,6 +445,7 @@ int __cpu_up(unsigned int cpu) > > { > > int rc; > > s_time_t deadline; > > + unsigned long *smp_up_cpu_addr = &smp_up_cpu; > > > > printk("Bringing up CPU%d\n", cpu); > > > > @@ -459,9 +461,12 @@ int __cpu_up(unsigned int cpu) > > /* Tell the remote CPU what its logical CPU ID is. */ > > init_data.cpuid = cpu; > > > > + if ( llc_coloring_enabled ) > > + smp_up_cpu_addr = (unsigned long *)virt_to_reloc_virt(&smp_up_cpu); > > + > > /* Open the gate for this CPU */ > > - smp_up_cpu = cpu_logical_map(cpu); > > - clean_dcache(smp_up_cpu); > > + *smp_up_cpu_addr = cpu_logical_map(cpu); > > + clean_dcache(*smp_up_cpu_addr); > > > > rc = arch_cpu_up(cpu); > > > > Cheers, > > -- > Julien Grall Thanks.
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 > >> 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. Note that this means the init_ttbr cannot be written directly. But you can solve this problem by re-mapping the address. > > 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? > > 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? >> 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. Cheers,
Hi Julien, On Sun, Jan 14, 2024 at 8:22 PM Julien Grall <julien@xen.org> 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.
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@xen.org> 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. [...] >> 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. >>> >>> 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. [...] >> ... 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). Cheers,
Hi Julien, On Mon, Jan 15, 2024 at 12:18 PM Julien Grall <julien@xen.org> 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@xen.org> 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
On 15/01/2024 15:43, Carlo Nonato wrote: > Hi Julien, Hi Carlo, > On Mon, Jan 15, 2024 at 12:18 PM Julien Grall <julien@xen.org> wrote: >> On 15/01/2024 10:11, Carlo Nonato wrote: >>> 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... If you link at XEN_VIRT_START then yes. But you could link at BOOT_RELOC_VIRT_START like you already do today. > >> [...] >> >>>> 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) Because the code running with the MMU off can access easily access everything. > >>>>> >>>>> 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 Are you looking at the 64-bit or 32-bit code? For 64-bit, staging is not using boot_* pagetable anymore for secondary CPUs. Instead, they directly jump to the runtime page-tables. > 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... It would be helpful if you share some code. > >> [...] >> >>>> ... 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. I will try to have a look this evening. If I can't, it may have to wait a couple of weeks unless someone has time before hand. Cheers,
Hi Julien, On Mon, Jan 15, 2024 at 5:16 PM Julien Grall <julien@xen.org> wrote: > On 15/01/2024 15:43, Carlo Nonato wrote: > > Hi Julien, > > Hi Carlo, > > > On Mon, Jan 15, 2024 at 12:18 PM Julien Grall <julien@xen.org> wrote: > >> On 15/01/2024 10:11, Carlo Nonato wrote: > >>> 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... > > If you link at XEN_VIRT_START then yes. But you could link at > BOOT_RELOC_VIRT_START like you already do today. Ok I think I got it. > > > >> [...] > >> > >>>> 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) > > Because the code running with the MMU off can access easily access > everything. This was what I got wrong. Now it's clear. > > > >>>>> > >>>>> 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 > > Are you looking at the 64-bit or 32-bit code? For 64-bit, staging is not > using boot_* pagetable anymore for secondary CPUs. Instead, they > directly jump to the runtime page-tables. Again, my fault. Got it. > > 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... > > It would be helpful if you share some code. Given the newfound knowledge, I'll think I can get further. Thanks. > > > >> [...] > >> > >>>> ... 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. > > I will try to have a look this evening. If I can't, it may have to wait > a couple of weeks unless someone has time before hand. > > Cheers, > > -- > Julien Grall
Hi, On 15/01/2024 16:16, Julien Grall wrote: > On 15/01/2024 15:43, Carlo Nonato wrote: >> Hi Julien, > > Hi Carlo, > >> On Mon, Jan 15, 2024 at 12:18 PM Julien Grall <julien@xen.org> wrote: >>> On 15/01/2024 10:11, Carlo Nonato wrote: >>>> 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... > > If you link at XEN_VIRT_START then yes. But you could link at > BOOT_RELOC_VIRT_START like you already do today. > >> >>> [...] >>> >>>>> 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) > > Because the code running with the MMU off can access easily access > everything. > >> >>>>>> >>>>>> 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 > > Are you looking at the 64-bit or 32-bit code? For 64-bit, staging is not > using boot_* pagetable anymore for secondary CPUs. Instead, they > directly jump to the runtime page-tables. > >> 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... > > It would be helpful if you share some code. > >> >>> [...] >>> >>>>> ... 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. > > I will try to have a look this evening. If I can't, it may have to wait > a couple of weeks unless someone has time before hand. Series sent [1]. This is not fully complete for cache coloring. You will need to modify the identity functions to take into account that the identity map will be different. You will also want to check the new identity map area is still below (see the check in prepare_boot_identity_mapping()). Cheers, [1] https://lore.kernel.org/xen-devel/20240116115509.77545-1-julien@xen.org/
Hi Julien, On Tue, Jan 16, 2024 at 12:59 PM Julien Grall <julien@xen.org> wrote: > > Hi, > > On 15/01/2024 16:16, Julien Grall wrote: > > On 15/01/2024 15:43, Carlo Nonato wrote: > >> Hi Julien, > > > > Hi Carlo, > > > >> On Mon, Jan 15, 2024 at 12:18 PM Julien Grall <julien@xen.org> wrote: > >>> On 15/01/2024 10:11, Carlo Nonato wrote: > >>>> 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... > > > > If you link at XEN_VIRT_START then yes. But you could link at > > BOOT_RELOC_VIRT_START like you already do today. > > > >> > >>> [...] > >>> > >>>>> 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) > > > > Because the code running with the MMU off can access easily access > > everything. > > > >> > >>>>>> > >>>>>> 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 > > > > Are you looking at the 64-bit or 32-bit code? For 64-bit, staging is not > > using boot_* pagetable anymore for secondary CPUs. Instead, they > > directly jump to the runtime page-tables. > > > >> 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... > > > > It would be helpful if you share some code. > > > >> > >>> [...] > >>> > >>>>> ... 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. > > > > I will try to have a look this evening. If I can't, it may have to wait > > a couple of weeks unless someone has time before hand. > > Series sent [1]. This is not fully complete for cache coloring. You will > need to modify the identity functions to take into account that the > identity map will be different. > > You will also want to check the new identity map area is still below > (see the check in prepare_boot_identity_mapping()). > > Cheers, > > [1] https://lore.kernel.org/xen-devel/20240116115509.77545-1-julien@xen.org/ Thank you very much for your help. I succeeded in applying your patches and reworking setup_pagetables() as we discussed previously. I dropped xen_colored_temp[], the temporary mapping at the end of setup_pagetables() and remove_llc_coloring_mappings(). Now, what to do with your patches? Will you push those before this series? I still have a few other minor problems to face in other patches of my series, but then I should be ready for v6. > -- > Julien Grall Thanks.
Hi Carlo, On 17/01/2024 17:38, Carlo Nonato wrote: > On Tue, Jan 16, 2024 at 12:59 PM Julien Grall <julien@xen.org> wrote: >> >> Hi, >> >> On 15/01/2024 16:16, Julien Grall wrote: >>> On 15/01/2024 15:43, Carlo Nonato wrote: >>>> Hi Julien, >>> >>> Hi Carlo, >>> >>>> On Mon, Jan 15, 2024 at 12:18 PM Julien Grall <julien@xen.org> wrote: >>>>> On 15/01/2024 10:11, Carlo Nonato wrote: >>>>>> 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... >>> >>> If you link at XEN_VIRT_START then yes. But you could link at >>> BOOT_RELOC_VIRT_START like you already do today. >>> >>>> >>>>> [...] >>>>> >>>>>>> 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) >>> >>> Because the code running with the MMU off can access easily access >>> everything. >>> >>>> >>>>>>>> >>>>>>>> 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 >>> >>> Are you looking at the 64-bit or 32-bit code? For 64-bit, staging is not >>> using boot_* pagetable anymore for secondary CPUs. Instead, they >>> directly jump to the runtime page-tables. >>> >>>> 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... >>> >>> It would be helpful if you share some code. >>> >>>> >>>>> [...] >>>>> >>>>>>> ... 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. >>> >>> I will try to have a look this evening. If I can't, it may have to wait >>> a couple of weeks unless someone has time before hand. >> >> Series sent [1]. This is not fully complete for cache coloring. You will >> need to modify the identity functions to take into account that the >> identity map will be different. >> >> You will also want to check the new identity map area is still below >> (see the check in prepare_boot_identity_mapping()). >> >> Cheers, >> >> [1] https://lore.kernel.org/xen-devel/20240116115509.77545-1-julien@xen.org/ > > Thank you very much for your help. I succeeded in applying your patches and > reworking setup_pagetables() as we discussed previously. I dropped > xen_colored_temp[], the temporary mapping at the end of setup_pagetables() > and remove_llc_coloring_mappings(). > > Now, what to do with your patches? Will you push those before this series? Michal reviewed the patches. I still want to give an opportunity for the other Arm maintainers to reply, so I plan to commit the patches early next week. You can then rebase your code on top. If your v6 is ready earlier, you could apply the first 3 patches of the series (patch #4 is just for testing) and send your work. If you plan to do that, then please mention it in the cover letter (better if you can even provide a branch with everything applied). Cheers,
diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c index 016e66978b..54cbc2afad 100644 --- a/xen/arch/arm/alternative.c +++ b/xen/arch/arm/alternative.c @@ -9,6 +9,7 @@ #include <xen/init.h> #include <xen/types.h> #include <xen/kernel.h> +#include <xen/llc-coloring.h> #include <xen/mm.h> #include <xen/vmap.h> #include <xen/smp.h> @@ -209,8 +210,12 @@ void __init apply_alternatives_all(void) * The text and inittext section are read-only. So re-map Xen to * be able to patch the code. */ - xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR, - VMAP_DEFAULT); + if ( llc_coloring_enabled ) + xenmap = xen_remap_colored(xen_mfn, xen_size); + else + xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR, + VMAP_DEFAULT); + /* Re-mapping Xen is not expected to fail during boot. */ BUG_ON(!xenmap); diff --git a/xen/arch/arm/arm64/mmu/head.S b/xen/arch/arm/arm64/mmu/head.S index 10774f30e4..6f0cc72897 100644 --- a/xen/arch/arm/arm64/mmu/head.S +++ b/xen/arch/arm/arm64/mmu/head.S @@ -419,6 +419,54 @@ fail: PRINT("- Boot failed -\r\n") b 1b ENDPROC(fail) +/* Copy Xen to new location and switch TTBR + * x0 ttbr + * x1 source address + * x2 destination address + * x3 length + * + * Source and destination must be word aligned, length is rounded up + * to a 16 byte boundary. + * + * MUST BE VERY CAREFUL when saving things to RAM over the copy */ +ENTRY(relocate_xen) + /* Copy 16 bytes at a time using: + * x9: counter + * x10: data + * x11: data + * x12: source + * x13: destination + */ + mov x9, x3 + mov x12, x1 + mov x13, x2 + +1: ldp x10, x11, [x12], #16 + stp x10, x11, [x13], #16 + + subs x9, x9, #16 + bgt 1b + + /* Flush destination from dcache using: + * x9: counter + * x10: step + * x11: vaddr + */ + dsb sy /* So the CPU issues all writes to the range */ + + mov x9, x3 + ldr x10, =dcache_line_bytes /* x10 := step */ + ldr x10, [x10] + mov x11, x2 + +1: dc cvac, x11 + + add x11, x11, x10 + subs x9, x9, x10 + bgt 1b + + b switch_ttbr_id + /* * Switch TTBR * diff --git a/xen/arch/arm/arm64/mmu/mm.c b/xen/arch/arm/arm64/mmu/mm.c index d2651c9486..5a26d64ab7 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 */ #include <xen/init.h> +#include <xen/llc-coloring.h> #include <xen/mm.h> #include <xen/pfn.h> @@ -125,27 +126,44 @@ void update_identity_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) { - vaddr_t id_addr = virt_to_maddr(switch_ttbr_id); - switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr; + vaddr_t vaddr, id_addr; lpae_t pte; + if ( llc_coloring_enabled ) + vaddr = (vaddr_t)relocate_xen; + else + vaddr = (vaddr_t)switch_ttbr_id; + + id_addr = virt_to_maddr(vaddr); + /* 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)switch_ttbr_id); + pte = pte_of_xenaddr(vaddr); pte.pt.table = 1; pte.pt.xn = 0; pte.pt.ro = 1; write_pte(&xen_third_id[third_table_offset(id_addr)], pte); /* Switch TTBR */ - fn(ttbr); + if ( llc_coloring_enabled ) + { + relocate_xen_fn *fn = (relocate_xen_fn *)id_addr; + fn(ttbr, _start, (void *)BOOT_RELOC_VIRT_START, _end - _start); + } + else + { + switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr; + fn(ttbr); + } /* * Disable the identity mapping in the runtime page tables. diff --git a/xen/arch/arm/include/asm/llc-coloring.h b/xen/arch/arm/include/asm/llc-coloring.h index 5f9b0a8121..4d6071e50b 100644 --- a/xen/arch/arm/include/asm/llc-coloring.h +++ b/xen/arch/arm/include/asm/llc-coloring.h @@ -12,11 +12,27 @@ #define __ASM_ARM_COLORING_H__ #include <xen/init.h> +#include <xen/mm-frame.h> + +/** + * Iterate over each Xen mfn in the colored space. + * @mfn: the current mfn. The first non colored mfn must be provided as the + * starting point. + * @i: loop index. + */ +#define for_each_xen_colored_mfn(mfn, i) \ + for ( i = 0, mfn = xen_colored_mfn(mfn); \ + i < (_end - _start) >> PAGE_SHIFT; \ + i++, mfn = xen_colored_mfn(mfn_add(mfn, 1)) ) bool __init llc_coloring_init(void); int dom0_set_llc_colors(struct domain *d); int domain_set_llc_colors_from_str(struct domain *d, const char *str); +paddr_t xen_colored_map_size(paddr_t size); +mfn_t xen_colored_mfn(mfn_t mfn); +void *xen_remap_colored(mfn_t xen_fn, paddr_t xen_size); + #endif /* __ASM_ARM_COLORING_H__ */ /* diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h index 1829c559d6..311f092cf2 100644 --- a/xen/arch/arm/include/asm/mm.h +++ b/xen/arch/arm/include/asm/mm.h @@ -203,12 +203,17 @@ extern unsigned long frametable_base_pdx; #define PDX_GROUP_SHIFT SECOND_SHIFT +#define virt_to_reloc_virt(virt) \ + (((vaddr_t)virt) - XEN_VIRT_START + BOOT_RELOC_VIRT_START) + /* Boot-time pagetable setup */ -extern void setup_pagetables(unsigned long boot_phys_offset); +extern void setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr); /* Map FDT in boot pagetable */ extern void *early_fdt_map(paddr_t fdt_paddr); /* Remove early mappings */ extern void remove_early_mappings(void); +/* Remove early LLC coloring mappings */ +extern void remove_llc_coloring_mappings(void); /* Prepare the memory subystem to bring-up the given secondary CPU */ extern int prepare_secondary_mm(int cpu); /* Map a frame table to cover physical addresses ps through pe */ diff --git a/xen/arch/arm/llc-coloring.c b/xen/arch/arm/llc-coloring.c index 99ea69ad39..f434efc45b 100644 --- a/xen/arch/arm/llc-coloring.c +++ b/xen/arch/arm/llc-coloring.c @@ -14,6 +14,7 @@ #include <xen/llc-coloring.h> #include <xen/param.h> #include <xen/types.h> +#include <xen/vmap.h> #include <asm/processor.h> #include <asm/sysregs.h> @@ -38,6 +39,7 @@ static unsigned int __ro_after_init xen_num_colors; #define mfn_color_mask (nr_colors - 1) #define mfn_to_color(mfn) (mfn_x(mfn) & mfn_color_mask) +#define mfn_set_color(mfn, color) ((mfn_x(mfn) & ~mfn_color_mask) | (color)) /* * Parse the coloring configuration given in the buf string, following the @@ -354,6 +356,48 @@ unsigned int get_nr_llc_colors(void) return nr_colors; } +paddr_t xen_colored_map_size(paddr_t size) +{ + return ROUNDUP(size * nr_colors, XEN_PADDR_ALIGN); +} + +mfn_t xen_colored_mfn(mfn_t mfn) +{ + unsigned int i, color = mfn_to_color(mfn); + + for( i = 0; i < xen_num_colors; i++ ) + { + if ( color == xen_colors[i] ) + return mfn; + else if ( color < xen_colors[i] ) + return mfn_set_color(mfn, xen_colors[i]); + } + + /* Jump to next color space (nr_colors mfns) and use the first color */ + return mfn_set_color(mfn_add(mfn, nr_colors), xen_colors[0]); +} + +void *xen_remap_colored(mfn_t xen_mfn, paddr_t xen_size) +{ + unsigned int i; + void *xenmap; + mfn_t *xen_colored_mfns; + + xen_colored_mfns = xmalloc_array(mfn_t, xen_size >> PAGE_SHIFT); + if ( !xen_colored_mfns ) + panic("Can't allocate LLC colored MFNs\n"); + + for_each_xen_colored_mfn( xen_mfn, i ) + { + xen_colored_mfns[i] = xen_mfn; + } + + xenmap = vmap(xen_colored_mfns, xen_size >> PAGE_SHIFT); + xfree(xen_colored_mfns); + + return xenmap; +} + /* * Local variables: * mode: C 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 + /* Common pagetable leaves */ /* Second level page table used to cover Xen virtual address space */ static DEFINE_PAGE_TABLE(xen_second); @@ -130,7 +135,12 @@ static void __init __maybe_unused build_assertions(void) lpae_t __init pte_of_xenaddr(vaddr_t va) { - paddr_t ma = va + phys_offset; + paddr_t ma; + + if ( llc_coloring_enabled ) + ma = virt_to_maddr(virt_to_reloc_virt(va)); + else + ma = va + phys_offset; return mfn_to_xen_entry(maddr_to_mfn(ma), MT_NORMAL); } @@ -216,11 +226,55 @@ static void xen_pt_enforce_wnx(void) flush_xen_tlb_local(); } +#ifdef CONFIG_LLC_COLORING +static void __init create_llc_coloring_mappings(paddr_t xen_paddr) +{ + lpae_t pte; + unsigned int i; + mfn_t mfn = maddr_to_mfn(xen_paddr); + + for_each_xen_colored_mfn( mfn, i ) + { + pte = mfn_to_xen_entry(mfn, MT_NORMAL); + pte.pt.table = 1; /* level 3 mappings always have this bit set */ + xen_colored_temp[i] = pte; + } + + pte = mfn_to_xen_entry(virt_to_mfn(xen_colored_temp), MT_NORMAL); + pte.pt.table = 1; + write_pte(&boot_second[second_table_offset(BOOT_RELOC_VIRT_START)], pte); +} + +void __init remove_llc_coloring_mappings(void) +{ + int rc; + + /* destroy the _PAGE_BLOCK mapping */ + rc = modify_xen_mappings(BOOT_RELOC_VIRT_START, + BOOT_RELOC_VIRT_START + SZ_2M, + _PAGE_BLOCK); + BUG_ON(rc); +} +#else +static void __init create_llc_coloring_mappings(paddr_t xen_paddr) {} +void __init remove_llc_coloring_mappings(void) {} +#endif /* CONFIG_LLC_COLORING */ + /* - * Boot-time pagetable setup. + * Boot-time pagetable setup with coloring support * Changes here may need matching changes in head.S + * + * The coloring support consists of: + * - Create a temporary colored mapping that conforms to Xen color selection. + * - pte_of_xenaddr takes care of translating the virtual addresses to the + * new colored physical space and the returns the pte, so that the page table + * initialization can remain the same. + * - Copy Xen to the new colored physical space by exploiting the temporary + * mapping. + * - Update TTBR0_EL2 with the new root page table address. */ -void __init setup_pagetables(unsigned long boot_phys_offset) + +void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr) { uint64_t ttbr; lpae_t pte, *p; @@ -228,6 +282,9 @@ void __init setup_pagetables(unsigned long boot_phys_offset) phys_offset = boot_phys_offset; + if ( llc_coloring_enabled ) + create_llc_coloring_mappings(xen_paddr); + arch_setup_page_tables(); #ifdef CONFIG_ARM_64 @@ -281,10 +338,13 @@ void __init setup_pagetables(unsigned long boot_phys_offset) pte.pt.table = 1; xen_second[second_table_offset(FIXMAP_ADDR(0))] = pte; + if ( llc_coloring_enabled ) + ttbr = virt_to_maddr(virt_to_reloc_virt(xen_pgtable)); + else #ifdef CONFIG_ARM_64 - ttbr = (uintptr_t) xen_pgtable + phys_offset; + ttbr = (uintptr_t) xen_pgtable + phys_offset; #else - ttbr = (uintptr_t) cpu0_pgtable + phys_offset; + ttbr = (uintptr_t) cpu0_pgtable + phys_offset; #endif switch_ttbr(ttbr); @@ -294,6 +354,18 @@ void __init setup_pagetables(unsigned long boot_phys_offset) #ifdef CONFIG_ARM_32 per_cpu(xen_pgtable, 0) = cpu0_pgtable; #endif + + /* + * Keep original Xen memory mapped because secondary CPUs still point to it + * and a few variables needs to be accessed by the master CPU in order to + * let them boot. This mapping will also replace the one created at the + * beginning of setup_pagetables. + */ + if ( llc_coloring_enabled ) + map_pages_to_xen(BOOT_RELOC_VIRT_START, + maddr_to_mfn(XEN_VIRT_START + phys_offset), + SZ_2M >> PAGE_SHIFT, PAGE_HYPERVISOR_RW | _PAGE_BLOCK); + } void *__init arch_vmap_virt_end(void) diff --git a/xen/arch/arm/mmu/smpboot.c b/xen/arch/arm/mmu/smpboot.c index b6fc0aae07..a69183ec88 100644 --- a/xen/arch/arm/mmu/smpboot.c +++ b/xen/arch/arm/mmu/smpboot.c @@ -6,6 +6,7 @@ */ #include <xen/domain_page.h> +#include <xen/llc-coloring.h> #include <asm/setup.h> @@ -71,14 +72,20 @@ static void clear_boot_pagetables(void) #ifdef CONFIG_ARM_64 int prepare_secondary_mm(int cpu) { + uint64_t *init_ttbr_addr = &init_ttbr; + clear_boot_pagetables(); + if ( llc_coloring_enabled ) + init_ttbr_addr = (uint64_t *)virt_to_reloc_virt(&init_ttbr); + /* * Set init_ttbr for this CPU coming up. All CPUs share a single setof * pagetables, but rewrite it each time for consistency with 32 bit. */ - init_ttbr = virt_to_maddr(xen_pgtable); - clean_dcache(init_ttbr); + *init_ttbr_addr = virt_to_maddr(xen_pgtable); + clean_dcache(*init_ttbr_addr); + return 0; } #else diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c index 695d2fa1f1..23e298c477 100644 --- a/xen/arch/arm/psci.c +++ b/xen/arch/arm/psci.c @@ -11,6 +11,7 @@ #include <xen/types.h> #include <xen/init.h> +#include <xen/llc-coloring.h> #include <xen/mm.h> #include <xen/smp.h> #include <asm/cpufeature.h> @@ -39,9 +40,13 @@ static uint32_t psci_cpu_on_nr; int call_psci_cpu_on(int cpu) { struct arm_smccc_res res; + vaddr_t init_secondary_addr = (vaddr_t)init_secondary; - arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary), - &res); + if ( llc_coloring_enabled ) + init_secondary_addr = virt_to_reloc_virt(init_secondary); + + arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), + __pa(init_secondary_addr), &res); return PSCI_RET(res); } diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 4c16b566db..ebbbf39477 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -545,6 +545,100 @@ static void * __init relocate_fdt(paddr_t dtb_paddr, size_t dtb_size) return fdt; } +#ifdef CONFIG_LLC_COLORING +/* + * Returns the end address of the highest region in the range s..e + * with required size and alignment that does not conflict with the + * modules from first_mod to nr_modules. + * + * For non-recursive callers first_mod should normally be 0 (all + * modules and Xen itself) or 1 (all modules but not Xen). + */ +static paddr_t __init consider_modules(paddr_t s, paddr_t e, + uint32_t size, paddr_t align, + int first_mod) +{ + const struct bootmodules *mi = &bootinfo.modules; + int i; + int nr; + + s = (s+align-1) & ~(align-1); + e = e & ~(align-1); + + if ( s > e || e - s < size ) + return 0; + + /* First check the boot modules */ + for ( i = first_mod; i < mi->nr_mods; i++ ) + { + paddr_t mod_s = mi->module[i].start; + paddr_t mod_e = mod_s + mi->module[i].size; + + if ( s < mod_e && mod_s < e ) + { + mod_e = consider_modules(mod_e, e, size, align, i+1); + if ( mod_e ) + return mod_e; + + return consider_modules(s, mod_s, size, align, i+1); + } + } + + /* Now check any fdt reserved areas. */ + + nr = fdt_num_mem_rsv(device_tree_flattened); + + for ( ; i < mi->nr_mods + nr; i++ ) + { + paddr_t mod_s, mod_e; + + if ( fdt_get_mem_rsv_paddr(device_tree_flattened, + i - mi->nr_mods, + &mod_s, &mod_e ) < 0 ) + /* If we can't read it, pretend it doesn't exist... */ + continue; + + /* fdt_get_mem_rsv_paddr returns length */ + mod_e += mod_s; + + if ( s < mod_e && mod_s < e ) + { + mod_e = consider_modules(mod_e, e, size, align, i+1); + if ( mod_e ) + return mod_e; + + return consider_modules(s, mod_s, size, align, i+1); + } + } + + /* + * i is the current bootmodule we are evaluating, across all + * possible kinds of bootmodules. + * + * When retrieving the corresponding reserved-memory addresses, we + * need to index the bootinfo.reserved_mem bank starting from 0, and + * only counting the reserved-memory modules. Hence, we need to use + * i - nr. + */ + nr += mi->nr_mods; + for ( ; i - nr < bootinfo.reserved_mem.nr_banks; i++ ) + { + paddr_t r_s = bootinfo.reserved_mem.bank[i - nr].start; + paddr_t r_e = r_s + bootinfo.reserved_mem.bank[i - nr].size; + + if ( s < r_e && r_s < e ) + { + r_e = consider_modules(r_e, e, size, align, i + 1); + if ( r_e ) + return r_e; + + return consider_modules(s, r_s, size, align, i + 1); + } + } + return e; +} +#endif + /* * Return the end of the non-module region starting at s. In other * words return s the start of the next modules after s. @@ -579,6 +673,62 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end) return lowest; } +#ifdef CONFIG_LLC_COLORING +/** + * get_xen_paddr - get physical address to relocate Xen to + * + * Xen is relocated to as near to the top of RAM as possible and + * aligned to a XEN_PADDR_ALIGN boundary. + */ +static paddr_t __init get_xen_paddr(uint32_t xen_size) +{ + struct meminfo *mi = &bootinfo.mem; + paddr_t min_size; + paddr_t paddr = 0; + int i; + + min_size = (xen_size + (XEN_PADDR_ALIGN-1)) & ~(XEN_PADDR_ALIGN-1); + + /* Find the highest bank with enough space. */ + for ( i = 0; i < mi->nr_banks; i++ ) + { + const struct membank *bank = &mi->bank[i]; + paddr_t s, e; + + if ( bank->size >= min_size ) + { + e = consider_modules(bank->start, bank->start + bank->size, + min_size, XEN_PADDR_ALIGN, 0); + if ( !e ) + continue; + +#ifdef CONFIG_ARM_32 + /* Xen must be under 4GB */ + if ( e > 0x100000000ULL ) + e = 0x100000000ULL; + if ( e < bank->start ) + continue; +#endif + + s = e - min_size; + + if ( s > paddr ) + paddr = s; + } + } + + if ( !paddr ) + panic("Not enough memory to relocate Xen\n"); + + printk("Placing Xen at 0x%"PRIpaddr"-0x%"PRIpaddr"\n", + paddr, paddr + min_size); + + return paddr; +} +#else +static paddr_t __init get_xen_paddr(uint32_t xen_size) { return 0; } +#endif + void __init init_pdx(void) { paddr_t bank_start, bank_size, bank_end; @@ -724,8 +874,6 @@ void asmlinkage __init start_xen(unsigned long boot_phys_offset, /* Initialize traps early allow us to get backtrace when an error occurred */ init_traps(); - setup_pagetables(boot_phys_offset); - smp_clear_cpu_maps(); device_tree_flattened = early_fdt_map(fdt_paddr); @@ -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); } + setup_pagetables(boot_phys_offset, xen_bootmodule->start); + device_tree_flattened = early_fdt_map(fdt_paddr); + setup_mm(); /* Parse the ACPI tables for possible boot-time configuration */ @@ -867,6 +1020,14 @@ void asmlinkage __init start_xen(unsigned long boot_phys_offset, setup_virt_paging(); + /* + * The removal is done earlier than discard_initial_modules beacuse the + * livepatch init uses a virtual address equal to BOOT_RELOC_VIRT_START. + * Remove LLC coloring mappings to expose a clear state to the livepatch + * module. + */ + if ( llc_coloring_enabled ) + remove_llc_coloring_mappings(); do_initcalls(); /* diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c index 7110bc11fc..7ed7357d58 100644 --- a/xen/arch/arm/smpboot.c +++ b/xen/arch/arm/smpboot.c @@ -14,6 +14,7 @@ #include <xen/domain_page.h> #include <xen/errno.h> #include <xen/init.h> +#include <xen/llc-coloring.h> #include <xen/mm.h> #include <xen/param.h> #include <xen/sched.h> @@ -444,6 +445,7 @@ int __cpu_up(unsigned int cpu) { int rc; s_time_t deadline; + unsigned long *smp_up_cpu_addr = &smp_up_cpu; printk("Bringing up CPU%d\n", cpu); @@ -459,9 +461,12 @@ int __cpu_up(unsigned int cpu) /* Tell the remote CPU what its logical CPU ID is. */ init_data.cpuid = cpu; + if ( llc_coloring_enabled ) + smp_up_cpu_addr = (unsigned long *)virt_to_reloc_virt(&smp_up_cpu); + /* Open the gate for this CPU */ - smp_up_cpu = cpu_logical_map(cpu); - clean_dcache(smp_up_cpu); + *smp_up_cpu_addr = cpu_logical_map(cpu); + clean_dcache(*smp_up_cpu_addr); rc = arch_cpu_up(cpu);