| Message ID | 20221216114853.8227-21-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove the directmap | expand |
Hi Julien, > -----Original Message----- > Subject: [PATCH 20/22] xen/arm64: mm: Use per-pCPU page-tables > > From: Julien Grall <jgrall@amazon.com> > > At the moment, on Arm64, every pCPU are sharing the same page-tables. Nit: s/every pCPU are/ every pCPU is/ > > /* > diff --git a/xen/arch/arm/include/asm/domain_page.h > b/xen/arch/arm/include/asm/domain_page.h > new file mode 100644 > index 000000000000..e9f52685e2ec > --- /dev/null > +++ b/xen/arch/arm/include/asm/domain_page.h > @@ -0,0 +1,13 @@ > +#ifndef __ASM_ARM_DOMAIN_PAGE_H__ > +#define __ASM_ARM_DOMAIN_PAGE_H__ > + > +#ifdef CONFIG_ARCH_MAP_DOMAIN_PAGE > +bool init_domheap_mappings(unsigned int cpu); I wonder if we can make this function "__init" as IIRC this function is only used at Xen boot time, but since the original init_domheap_mappings() is not "__init" anyway so this is not a strong argument. > +#else > +static inline bool init_domheap_mappings(unsigned int cpu) (and also here) Either you agree with above "__init" comment or not: Reviewed-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry
Hi Henry, On 06/01/2023 14:54, Henry Wang wrote: >> -----Original Message----- >> Subject: [PATCH 20/22] xen/arm64: mm: Use per-pCPU page-tables >> >> From: Julien Grall <jgrall@amazon.com> >> >> At the moment, on Arm64, every pCPU are sharing the same page-tables. > > Nit: s/every pCPU are/ every pCPU is/ I will fix it. > >> >> /* >> diff --git a/xen/arch/arm/include/asm/domain_page.h >> b/xen/arch/arm/include/asm/domain_page.h >> new file mode 100644 >> index 000000000000..e9f52685e2ec >> --- /dev/null >> +++ b/xen/arch/arm/include/asm/domain_page.h >> @@ -0,0 +1,13 @@ >> +#ifndef __ASM_ARM_DOMAIN_PAGE_H__ >> +#define __ASM_ARM_DOMAIN_PAGE_H__ >> + >> +#ifdef CONFIG_ARCH_MAP_DOMAIN_PAGE >> +bool init_domheap_mappings(unsigned int cpu); > > I wonder if we can make this function "__init" as IIRC this function is only > used at Xen boot time, but since the original init_domheap_mappings() > is not "__init" anyway so this is not a strong argument. While this is not yet supported on Xen on Arm, CPUs can be onlined/offlined at runtime. So you want to keep init_domheap_mappings() around. We could consider to provide a new attribute that will be match __init if hotplug is supported otherwirse it would be a NOP. But I don't think this is related to this series (most of the function used for bringup are not in __init). >> +static inline bool init_domheap_mappings(unsigned int cpu) > > (and also here) > > Either you agree with above "__init" comment or not: > Reviewed-by: Henry Wang <Henry.Wang@arm.com> Thanks! Cheers,
Hi Julien, > -----Original Message----- > Subject: Re: [PATCH 20/22] xen/arm64: mm: Use per-pCPU page-tables > > Hi Henry, > > >> Subject: [PATCH 20/22] xen/arm64: mm: Use per-pCPU page-tables > >> > >> From: Julien Grall <jgrall@amazon.com> > >> > >> At the moment, on Arm64, every pCPU are sharing the same page-tables. > > > > Nit: s/every pCPU are/ every pCPU is/ > > I will fix it. Thank you. > > >> +bool init_domheap_mappings(unsigned int cpu); > > > > I wonder if we can make this function "__init" as IIRC this function is only > > used at Xen boot time, but since the original init_domheap_mappings() > > is not "__init" anyway so this is not a strong argument. > > While this is not yet supported on Xen on Arm, CPUs can be > onlined/offlined at runtime. So you want to keep init_domheap_mappings() > around. This is a very good point. I agree the pCPU online/offline is affected by the "__init" so leaving the function without the "__init" like what we are doing now is a good idea. > > We could consider to provide a new attribute that will be match __init > if hotplug is supported otherwirse it would be a NOP. But I don't think > this is related to this series (most of the function used for bringup > are not in __init). Agreed. > > >> +static inline bool init_domheap_mappings(unsigned int cpu) > > > > (and also here) > > > > Either you agree with above "__init" comment or not: > > Reviewed-by: Henry Wang <Henry.Wang@arm.com> > > Thanks! No problem. To avoid confusion, my reviewed-by tag still holds. Kind regards, Henry > > Cheers, > > -- > Julien Grall
On Fri, 16 Dec 2022, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > At the moment, on Arm64, every pCPU are sharing the same page-tables. > > In a follow-up patch, we will allow the possibility to remove the > direct map and therefore it will be necessary to have a mapcache. > > While we have plenty of spare virtual address space to have > to reserve part for each pCPU, it means that temporary mappings > (e.g. guest memory) could be accessible by every pCPU. > > In order to increase our security posture, it would be better if > those mappings are only accessible by the pCPU doing the temporary > mapping. > > In addition to that, a per-pCPU page-tables opens the way to have > per-domain mapping area. > > Arm32 is already using per-pCPU page-tables so most of the code > can be re-used. Arm64 doesn't yet have support for the mapcache, > so a stub is provided (moved to its own header asm/domain_page.h). > > Take the opportunity to fix a typo in a comment that is modified. > > Signed-off-by: Julien Grall <jgrall@amazon.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/arch/arm/domain_page.c | 2 ++ > xen/arch/arm/include/asm/arm32/mm.h | 8 ----- > xen/arch/arm/include/asm/domain_page.h | 13 ++++++++ > xen/arch/arm/include/asm/mm.h | 5 +++ > xen/arch/arm/mm.c | 42 +++++++------------------- > xen/arch/arm/setup.c | 1 + > 6 files changed, 32 insertions(+), 39 deletions(-) > create mode 100644 xen/arch/arm/include/asm/domain_page.h > > diff --git a/xen/arch/arm/domain_page.c b/xen/arch/arm/domain_page.c > index b7c02c919064..4540b3c5f24c 100644 > --- a/xen/arch/arm/domain_page.c > +++ b/xen/arch/arm/domain_page.c > @@ -3,6 +3,8 @@ > #include <xen/pmap.h> > #include <xen/vmap.h> > > +#include <asm/domain_page.h> > + > /* Override macros from asm/page.h to make them work with mfn_t */ > #undef virt_to_mfn > #define virt_to_mfn(va) _mfn(__virt_to_mfn(va)) > diff --git a/xen/arch/arm/include/asm/arm32/mm.h b/xen/arch/arm/include/asm/arm32/mm.h > index 8bfc906e7178..6b039d9ceaa2 100644 > --- a/xen/arch/arm/include/asm/arm32/mm.h > +++ b/xen/arch/arm/include/asm/arm32/mm.h > @@ -1,12 +1,6 @@ > #ifndef __ARM_ARM32_MM_H__ > #define __ARM_ARM32_MM_H__ > > -#include <xen/percpu.h> > - > -#include <asm/lpae.h> > - > -DECLARE_PER_CPU(lpae_t *, xen_pgtable); > - > /* > * Only a limited amount of RAM, called xenheap, is always mapped on ARM32. > * For convenience always return false. > @@ -16,8 +10,6 @@ static inline bool arch_mfns_in_directmap(unsigned long mfn, unsigned long nr) > return false; > } > > -bool init_domheap_mappings(unsigned int cpu); > - > #endif /* __ARM_ARM32_MM_H__ */ > > /* > diff --git a/xen/arch/arm/include/asm/domain_page.h b/xen/arch/arm/include/asm/domain_page.h > new file mode 100644 > index 000000000000..e9f52685e2ec > --- /dev/null > +++ b/xen/arch/arm/include/asm/domain_page.h > @@ -0,0 +1,13 @@ > +#ifndef __ASM_ARM_DOMAIN_PAGE_H__ > +#define __ASM_ARM_DOMAIN_PAGE_H__ > + > +#ifdef CONFIG_ARCH_MAP_DOMAIN_PAGE > +bool init_domheap_mappings(unsigned int cpu); > +#else > +static inline bool init_domheap_mappings(unsigned int cpu) > +{ > + return true; > +} > +#endif > + > +#endif /* __ASM_ARM_DOMAIN_PAGE_H__ */ > diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h > index 2366928d71aa..7a2c775f9562 100644 > --- a/xen/arch/arm/include/asm/mm.h > +++ b/xen/arch/arm/include/asm/mm.h > @@ -2,6 +2,9 @@ > #define __ARCH_ARM_MM__ > > #include <xen/kernel.h> > +#include <xen/percpu.h> > + > +#include <asm/lpae.h> > #include <asm/page.h> > #include <public/xen.h> > #include <xen/pdx.h> > @@ -14,6 +17,8 @@ > # error "unknown ARM variant" > #endif > > +DECLARE_PER_CPU(lpae_t *, xen_pgtable); > + > /* Align Xen to a 2 MiB boundary. */ > #define XEN_PADDR_ALIGN (1 << 21) > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index 4e208f7d20c8..2af751af9003 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -24,6 +24,7 @@ > > #include <xsm/xsm.h> > > +#include <asm/domain_page.h> > #include <asm/fixmap.h> > #include <asm/setup.h> > > @@ -90,20 +91,19 @@ DEFINE_BOOT_PAGE_TABLE(boot_third); > * xen_second, xen_fixmap and xen_xenmap are always shared between all > * PCPUs. > */ > +/* Per-CPU pagetable pages */ > +/* xen_pgtable == root of the trie (zeroeth level on 64-bit, first on 32-bit) */ > +DEFINE_PER_CPU(lpae_t *, xen_pgtable); > + > +/* Root of the trie for cpu0, other CPU's PTs are dynamically allocated */ > +static DEFINE_PAGE_TABLE(cpu0_pgtable); > +#define THIS_CPU_PGTABLE this_cpu(xen_pgtable) > > #ifdef CONFIG_ARM_64 > #define HYP_PT_ROOT_LEVEL 0 > -static DEFINE_PAGE_TABLE(xen_pgtable); > static DEFINE_PAGE_TABLE(xen_first); > -#define THIS_CPU_PGTABLE xen_pgtable > #else > #define HYP_PT_ROOT_LEVEL 1 > -/* Per-CPU pagetable pages */ > -/* xen_pgtable == root of the trie (zeroeth level on 64-bit, first on 32-bit) */ > -DEFINE_PER_CPU(lpae_t *, xen_pgtable); > -#define THIS_CPU_PGTABLE this_cpu(xen_pgtable) > -/* Root of the trie for cpu0, other CPU's PTs are dynamically allocated */ > -static DEFINE_PAGE_TABLE(cpu0_pgtable); > #endif > > /* Common pagetable leaves */ > @@ -481,14 +481,13 @@ void __init setup_pagetables(unsigned long boot_phys_offset) > > phys_offset = boot_phys_offset; > > + p = cpu0_pgtable; > + > #ifdef CONFIG_ARM_64 > - p = (void *) xen_pgtable; > p[0] = pte_of_xenaddr((uintptr_t)xen_first); > p[0].pt.table = 1; > p[0].pt.xn = 0; > p = (void *) xen_first; > -#else > - p = (void *) cpu0_pgtable; > #endif > > /* Map xen second level page-table */ > @@ -527,19 +526,13 @@ void __init setup_pagetables(unsigned long boot_phys_offset) > pte.pt.table = 1; > xen_second[second_table_offset(FIXMAP_ADDR(0))] = pte; > > -#ifdef CONFIG_ARM_64 > - ttbr = (uintptr_t) xen_pgtable + phys_offset; > -#else > ttbr = (uintptr_t) cpu0_pgtable + phys_offset; > -#endif > > switch_ttbr(ttbr); > > xen_pt_enforce_wnx(); > > -#ifdef CONFIG_ARM_32 > per_cpu(xen_pgtable, 0) = cpu0_pgtable; > -#endif > } > > static void clear_boot_pagetables(void) > @@ -557,18 +550,6 @@ static void clear_boot_pagetables(void) > clear_table(boot_third); > } > > -#ifdef CONFIG_ARM_64 > -int init_secondary_pagetables(int cpu) > -{ > - clear_boot_pagetables(); > - > - /* 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 = (uintptr_t) xen_pgtable + phys_offset; > - clean_dcache(init_ttbr); > - return 0; > -} > -#else > int init_secondary_pagetables(int cpu) > { > lpae_t *root = alloc_xenheap_page(); > @@ -599,7 +580,6 @@ int init_secondary_pagetables(int cpu) > > return 0; > } > -#endif > > /* MMU setup for secondary CPUS (which already have paging enabled) */ > void mmu_init_secondary_cpu(void) > @@ -1089,7 +1069,7 @@ static int xen_pt_update(unsigned long virt, > unsigned long left = nr_mfns; > > /* > - * For arm32, page-tables are different on each CPUs. Yet, they share > + * Page-tables are different on each CPU. Yet, they share > * some common mappings. It is assumed that only common mappings > * will be modified with this function. > * > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 2311726f5ddd..88d9d90fb5ad 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -39,6 +39,7 @@ > #include <asm/gic.h> > #include <asm/cpuerrata.h> > #include <asm/cpufeature.h> > +#include <asm/domain_page.h> > #include <asm/platform.h> > #include <asm/procinfo.h> > #include <asm/setup.h> > -- > 2.38.1 >
diff --git a/xen/arch/arm/domain_page.c b/xen/arch/arm/domain_page.c index b7c02c919064..4540b3c5f24c 100644 --- a/xen/arch/arm/domain_page.c +++ b/xen/arch/arm/domain_page.c @@ -3,6 +3,8 @@ #include <xen/pmap.h> #include <xen/vmap.h> +#include <asm/domain_page.h> + /* Override macros from asm/page.h to make them work with mfn_t */ #undef virt_to_mfn #define virt_to_mfn(va) _mfn(__virt_to_mfn(va)) diff --git a/xen/arch/arm/include/asm/arm32/mm.h b/xen/arch/arm/include/asm/arm32/mm.h index 8bfc906e7178..6b039d9ceaa2 100644 --- a/xen/arch/arm/include/asm/arm32/mm.h +++ b/xen/arch/arm/include/asm/arm32/mm.h @@ -1,12 +1,6 @@ #ifndef __ARM_ARM32_MM_H__ #define __ARM_ARM32_MM_H__ -#include <xen/percpu.h> - -#include <asm/lpae.h> - -DECLARE_PER_CPU(lpae_t *, xen_pgtable); - /* * Only a limited amount of RAM, called xenheap, is always mapped on ARM32. * For convenience always return false. @@ -16,8 +10,6 @@ static inline bool arch_mfns_in_directmap(unsigned long mfn, unsigned long nr) return false; } -bool init_domheap_mappings(unsigned int cpu); - #endif /* __ARM_ARM32_MM_H__ */ /* diff --git a/xen/arch/arm/include/asm/domain_page.h b/xen/arch/arm/include/asm/domain_page.h new file mode 100644 index 000000000000..e9f52685e2ec --- /dev/null +++ b/xen/arch/arm/include/asm/domain_page.h @@ -0,0 +1,13 @@ +#ifndef __ASM_ARM_DOMAIN_PAGE_H__ +#define __ASM_ARM_DOMAIN_PAGE_H__ + +#ifdef CONFIG_ARCH_MAP_DOMAIN_PAGE +bool init_domheap_mappings(unsigned int cpu); +#else +static inline bool init_domheap_mappings(unsigned int cpu) +{ + return true; +} +#endif + +#endif /* __ASM_ARM_DOMAIN_PAGE_H__ */ diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h index 2366928d71aa..7a2c775f9562 100644 --- a/xen/arch/arm/include/asm/mm.h +++ b/xen/arch/arm/include/asm/mm.h @@ -2,6 +2,9 @@ #define __ARCH_ARM_MM__ #include <xen/kernel.h> +#include <xen/percpu.h> + +#include <asm/lpae.h> #include <asm/page.h> #include <public/xen.h> #include <xen/pdx.h> @@ -14,6 +17,8 @@ # error "unknown ARM variant" #endif +DECLARE_PER_CPU(lpae_t *, xen_pgtable); + /* Align Xen to a 2 MiB boundary. */ #define XEN_PADDR_ALIGN (1 << 21) diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 4e208f7d20c8..2af751af9003 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -24,6 +24,7 @@ #include <xsm/xsm.h> +#include <asm/domain_page.h> #include <asm/fixmap.h> #include <asm/setup.h> @@ -90,20 +91,19 @@ DEFINE_BOOT_PAGE_TABLE(boot_third); * xen_second, xen_fixmap and xen_xenmap are always shared between all * PCPUs. */ +/* Per-CPU pagetable pages */ +/* xen_pgtable == root of the trie (zeroeth level on 64-bit, first on 32-bit) */ +DEFINE_PER_CPU(lpae_t *, xen_pgtable); + +/* Root of the trie for cpu0, other CPU's PTs are dynamically allocated */ +static DEFINE_PAGE_TABLE(cpu0_pgtable); +#define THIS_CPU_PGTABLE this_cpu(xen_pgtable) #ifdef CONFIG_ARM_64 #define HYP_PT_ROOT_LEVEL 0 -static DEFINE_PAGE_TABLE(xen_pgtable); static DEFINE_PAGE_TABLE(xen_first); -#define THIS_CPU_PGTABLE xen_pgtable #else #define HYP_PT_ROOT_LEVEL 1 -/* Per-CPU pagetable pages */ -/* xen_pgtable == root of the trie (zeroeth level on 64-bit, first on 32-bit) */ -DEFINE_PER_CPU(lpae_t *, xen_pgtable); -#define THIS_CPU_PGTABLE this_cpu(xen_pgtable) -/* Root of the trie for cpu0, other CPU's PTs are dynamically allocated */ -static DEFINE_PAGE_TABLE(cpu0_pgtable); #endif /* Common pagetable leaves */ @@ -481,14 +481,13 @@ void __init setup_pagetables(unsigned long boot_phys_offset) phys_offset = boot_phys_offset; + p = cpu0_pgtable; + #ifdef CONFIG_ARM_64 - p = (void *) xen_pgtable; p[0] = pte_of_xenaddr((uintptr_t)xen_first); p[0].pt.table = 1; p[0].pt.xn = 0; p = (void *) xen_first; -#else - p = (void *) cpu0_pgtable; #endif /* Map xen second level page-table */ @@ -527,19 +526,13 @@ void __init setup_pagetables(unsigned long boot_phys_offset) pte.pt.table = 1; xen_second[second_table_offset(FIXMAP_ADDR(0))] = pte; -#ifdef CONFIG_ARM_64 - ttbr = (uintptr_t) xen_pgtable + phys_offset; -#else ttbr = (uintptr_t) cpu0_pgtable + phys_offset; -#endif switch_ttbr(ttbr); xen_pt_enforce_wnx(); -#ifdef CONFIG_ARM_32 per_cpu(xen_pgtable, 0) = cpu0_pgtable; -#endif } static void clear_boot_pagetables(void) @@ -557,18 +550,6 @@ static void clear_boot_pagetables(void) clear_table(boot_third); } -#ifdef CONFIG_ARM_64 -int init_secondary_pagetables(int cpu) -{ - clear_boot_pagetables(); - - /* 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 = (uintptr_t) xen_pgtable + phys_offset; - clean_dcache(init_ttbr); - return 0; -} -#else int init_secondary_pagetables(int cpu) { lpae_t *root = alloc_xenheap_page(); @@ -599,7 +580,6 @@ int init_secondary_pagetables(int cpu) return 0; } -#endif /* MMU setup for secondary CPUS (which already have paging enabled) */ void mmu_init_secondary_cpu(void) @@ -1089,7 +1069,7 @@ static int xen_pt_update(unsigned long virt, unsigned long left = nr_mfns; /* - * For arm32, page-tables are different on each CPUs. Yet, they share + * Page-tables are different on each CPU. Yet, they share * some common mappings. It is assumed that only common mappings * will be modified with this function. * diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 2311726f5ddd..88d9d90fb5ad 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -39,6 +39,7 @@ #include <asm/gic.h> #include <asm/cpuerrata.h> #include <asm/cpufeature.h> +#include <asm/domain_page.h> #include <asm/platform.h> #include <asm/procinfo.h> #include <asm/setup.h>