| Message ID | c4716207f7f269b8adf7ed1b1928558cfc1d3bc8.1665137247.git.mykyta_poturai@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/19] xen/arm: Implement PSCI system suspend | expand |
Hi, On 07/10/2022 11:32, Mykyta Poturai wrote: > From: Mirela Simonovic <mirela.simonovic@aggios.com> > > The MMU needs to be enabled in the resume flow before the context > can be restored (we need to be able to access the context data by > virtual address in order to restore it). The configuration of system > registers prior to branching to the routine that sets up the page > tables is copied from xen/arch/arm/arm64/head.S. > After the MMU is enabled, the content of TTBR0_EL2 is changed to > point to init_ttbr (page tables used at runtime). This is not Arm Arm compliant. Please look at the series [1] to see how you can safely switch the MMU on and use the page tables. > > At boot the init_ttbr variable is updated when a secondary CPU is > hotplugged. In the scenario where there is only one physical CPU in > the system, the init_ttbr would not be initialized for the use in > resume flow. To get the variable initialized in all scenarios in this > patch we add that the boot CPU updates init_ttbr after it sets the > page tables for runtime. > > After the memory management is resumed, the SCTLR_WXN in SCTLR_EL2 > has to be set in order to configure that a mapping cannot be both > writable and executable (this was configured prior to suspend). > This is done using an existing function (mmu_init_secondary_cpu). > > Update: moved hyp_resume to head.S to place it near the rest of the > start code > > Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com> > Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xilinx.com> > Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com> > --- > xen/arch/arm/arm64/entry.S | 2 ++ > xen/arch/arm/arm64/head.S | 30 ++++++++++++++++++++++++++++++ > xen/arch/arm/mm.c | 1 + > xen/arch/arm/suspend.c | 6 ++++++ > xen/include/asm-arm/processor.h | 22 ++++++++++++++++++++++ > 5 files changed, 61 insertions(+) > > diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S > index fc3811ad0a..f49f1daf46 100644 > --- a/xen/arch/arm/arm64/entry.S > +++ b/xen/arch/arm/arm64/entry.S > @@ -1,4 +1,6 @@ > #include <asm/current.h> > +#include <asm/macros.h> > +#include <asm/page.h> > #include <asm/regs.h> > #include <asm/alternative.h> > #include <asm/smccc.h> > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 8857955699..82d83214dc 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -958,6 +958,36 @@ ENTRY(efi_xen_start) > ENDPROC(efi_xen_start) > > ENTRY(hyp_resume) > + msr DAIFSet, 0xf /* Disable all interrupts */ > + > + tlbi alle2 > + dsb sy /* Ensure completion of TLB flush */ > + isb Please explain what this TLB is for. > + > + ldr x0, =start > + adr x19, start /* x19 := paddr (start) */ > + sub x20, x19, x0 /* x20 := phys-offset */ > + > + mov x22, #0 /* x22 := is_secondary_cpu */ x22 is not hold the is_secondary_cpu anymore. > + > + bl check_cpu_mode > + bl cpu_init > + bl create_page_tables > + bl enable_mmu > + > + ldr x0, =mmu_resumed /* Explicit vaddr, not RIP-relative */ > + br x0 /* Get a proper vaddr into PC */ > + > +mmu_resumed: > + ldr x4, =init_ttbr /* VA of TTBR0_EL2 stashed by CPU 0 */ > + ldr x4, [x4] /* Actual value */ > + dsb sy > + msr TTBR0_EL2, x4 > + dsb sy > + isb > + tlbi alle2 > + dsb sy /* Ensure completion of TLB flush */ > + isb > b . > > /* > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index eea926d823..29cdaff3bf 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -708,6 +708,7 @@ void __init setup_pagetables(unsigned long boot_phys_offset) > switch_ttbr(ttbr); > > xen_pt_enforce_wnx(); > + init_secondary_pagetables(0); This function is used to prepare the page tables for the secondary CPUs. This may allocate memory. So this is incorrect to call for CPU0. In this case, I think it would be better if the code to suspend sets init_ttbr and clear the boot pages tables. This could be done by split init_secondary_pagetables() in two: 1) Allocate memory for the page tables 2) Clear the boot page tables and the init_ttbr > > #ifdef CONFIG_ARM_32 > per_cpu(xen_pgtable, 0) = cpu0_pgtable; > diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c > index a0258befc9..aa5ee4714b 100644 > --- a/xen/arch/arm/suspend.c > +++ b/xen/arch/arm/suspend.c > @@ -167,6 +167,12 @@ static long system_suspend(void *data) > > system_state = SYS_STATE_resume; > > + /* > + * SCTLR_WXN needs to be set to configure that a mapping cannot be both > + * writable and executable. This is done by mmu_init_secondary_cpu. > + */ Let's avoid to describe what a function does in the caller. This can be easily rot. > + mmu_init_secondary_cpu(); I dislike the idea of using this function here. It is meant to be used by secondary CPUs, not CPU0. If you want to use it here, then it should be renamed to reflect how the function is used. > + > gic_resume(); > > resume_irqs: > diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h > index 8ab2940f68..ecf97f1ab4 100644 > --- a/xen/include/asm-arm/processor.h > +++ b/xen/include/asm-arm/processor.h > @@ -133,6 +133,28 @@ > #define TTBCR_PD1 (_AC(1,U)<<5) > > /* SCTLR System Control Register. */ > +/* HSCTLR is a subset of this. */ > +#define SCTLR_TE (_AC(1,U)<<30) > +#define SCTLR_AFE (_AC(1,U)<<29) > +#define SCTLR_TRE (_AC(1,U)<<28) > +#define SCTLR_NMFI (_AC(1,U)<<27) > +#define SCTLR_EE (_AC(1,U)<<25) > +#define SCTLR_VE (_AC(1,U)<<24) > +#define SCTLR_U (_AC(1,U)<<22) > +#define SCTLR_FI (_AC(1,U)<<21) > +#define SCTLR_WXN (_AC(1,U)<<19) > +#define SCTLR_HA (_AC(1,U)<<17) > +#define SCTLR_RR (_AC(1,U)<<14) > +#define SCTLR_V (_AC(1,U)<<13) > +#define SCTLR_I (_AC(1,U)<<12) > +#define SCTLR_Z (_AC(1,U)<<11) > +#define SCTLR_SW (_AC(1,U)<<10) > +#define SCTLR_B (_AC(1,U)<<7) > +#define SCTLR_C (_AC(1,U)<<2) > +#define SCTLR_A (_AC(1,U)<<1) > +#define SCTLR_M (_AC(1,U)<<0) > + > +#define HSCTLR_BASE _AC(0x30c51878,U) I don't see any use of SCTLR_* and HSCTLR_* here. What why do you need to define them? > > /* Bits specific to SCTLR_EL1 for Arm32 */ > Cheers, [2] https://lore.kernel.org/all/20221022150422.17707-1-julien@xen.org/
diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S index fc3811ad0a..f49f1daf46 100644 --- a/xen/arch/arm/arm64/entry.S +++ b/xen/arch/arm/arm64/entry.S @@ -1,4 +1,6 @@ #include <asm/current.h> +#include <asm/macros.h> +#include <asm/page.h> #include <asm/regs.h> #include <asm/alternative.h> #include <asm/smccc.h> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index 8857955699..82d83214dc 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -958,6 +958,36 @@ ENTRY(efi_xen_start) ENDPROC(efi_xen_start) ENTRY(hyp_resume) + msr DAIFSet, 0xf /* Disable all interrupts */ + + tlbi alle2 + dsb sy /* Ensure completion of TLB flush */ + isb + + ldr x0, =start + adr x19, start /* x19 := paddr (start) */ + sub x20, x19, x0 /* x20 := phys-offset */ + + mov x22, #0 /* x22 := is_secondary_cpu */ + + bl check_cpu_mode + bl cpu_init + bl create_page_tables + bl enable_mmu + + ldr x0, =mmu_resumed /* Explicit vaddr, not RIP-relative */ + br x0 /* Get a proper vaddr into PC */ + +mmu_resumed: + ldr x4, =init_ttbr /* VA of TTBR0_EL2 stashed by CPU 0 */ + ldr x4, [x4] /* Actual value */ + dsb sy + msr TTBR0_EL2, x4 + dsb sy + isb + tlbi alle2 + dsb sy /* Ensure completion of TLB flush */ + isb b . /* diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index eea926d823..29cdaff3bf 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -708,6 +708,7 @@ void __init setup_pagetables(unsigned long boot_phys_offset) switch_ttbr(ttbr); xen_pt_enforce_wnx(); + init_secondary_pagetables(0); #ifdef CONFIG_ARM_32 per_cpu(xen_pgtable, 0) = cpu0_pgtable; diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c index a0258befc9..aa5ee4714b 100644 --- a/xen/arch/arm/suspend.c +++ b/xen/arch/arm/suspend.c @@ -167,6 +167,12 @@ static long system_suspend(void *data) system_state = SYS_STATE_resume; + /* + * SCTLR_WXN needs to be set to configure that a mapping cannot be both + * writable and executable. This is done by mmu_init_secondary_cpu. + */ + mmu_init_secondary_cpu(); + gic_resume(); resume_irqs: diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h index 8ab2940f68..ecf97f1ab4 100644 --- a/xen/include/asm-arm/processor.h +++ b/xen/include/asm-arm/processor.h @@ -133,6 +133,28 @@ #define TTBCR_PD1 (_AC(1,U)<<5) /* SCTLR System Control Register. */ +/* HSCTLR is a subset of this. */ +#define SCTLR_TE (_AC(1,U)<<30) +#define SCTLR_AFE (_AC(1,U)<<29) +#define SCTLR_TRE (_AC(1,U)<<28) +#define SCTLR_NMFI (_AC(1,U)<<27) +#define SCTLR_EE (_AC(1,U)<<25) +#define SCTLR_VE (_AC(1,U)<<24) +#define SCTLR_U (_AC(1,U)<<22) +#define SCTLR_FI (_AC(1,U)<<21) +#define SCTLR_WXN (_AC(1,U)<<19) +#define SCTLR_HA (_AC(1,U)<<17) +#define SCTLR_RR (_AC(1,U)<<14) +#define SCTLR_V (_AC(1,U)<<13) +#define SCTLR_I (_AC(1,U)<<12) +#define SCTLR_Z (_AC(1,U)<<11) +#define SCTLR_SW (_AC(1,U)<<10) +#define SCTLR_B (_AC(1,U)<<7) +#define SCTLR_C (_AC(1,U)<<2) +#define SCTLR_A (_AC(1,U)<<1) +#define SCTLR_M (_AC(1,U)<<0) + +#define HSCTLR_BASE _AC(0x30c51878,U) /* Bits specific to SCTLR_EL1 for Arm32 */