Re: [PATCH 13/19] xen/arm: Resume memory management on Xen resume

[Julien Grall <julien@xxxxxxx>]

Hi,

On 07/10/2022 11:32, Mykyta Poturai wrote:
> From: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xxxxxxxxxx>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
> ---
>   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@xxxxxxx/

--
Julien Grall