Re: [PATCH 14/16] xen/arm: Resume memory management on Xen resume

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi, @Julien Grall

On Wed, Mar 12, 2025 at 1:04 AM Julien Grall <julien@xxxxxxx> wrote:
>
> Hi,
>
> On 05/03/2025 09:11, Mykola Kvach 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).
> >
> > 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.
> >
> > Signed-off-by: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
> > Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xxxxxxxxxx>
> > Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > ---
> > Changes in V3:
> > - updated commit message
> > - instead of using create_page_tables, enable_mmu, and 
> > mmu_init_secondary_cpu,
> >    the existing function enable_secondary_cpu_mm is now used
> > - prepare_secondary_mm (previously init_secondary_pagetables in the previous
> >    patch series) is now called at the end of start_xen instead of
> >    setup_pagetables. Calling it in the previous location caused a crash
> > - add early printk init during resume
> >
> > Changes in V2:
> > - moved hyp_resume to head.S to place it near the rest of the start code
> > - simplified the code in hyp_resume by using existing functions such as
> >    check_cpu_mode, cpu_init, create_page_tables, and enable_mmu
> > ---
> >   xen/arch/arm/arm64/head.S | 23 +++++++++++++++++++++++
> >   xen/arch/arm/setup.c      |  8 ++++++++
> >   2 files changed, 31 insertions(+)
> >
> > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > index 3522c497c5..fab2812e54 100644
> > --- a/xen/arch/arm/arm64/head.S
> > +++ b/xen/arch/arm/arm64/head.S
> > @@ -564,6 +564,29 @@ END(efi_xen_start)
> >   #ifdef CONFIG_SYSTEM_SUSPEND
> >
> >   FUNC(hyp_resume)
> > +        msr   DAIFSet, 0xf           /* Disable all interrupts */
>
> Surely we should be here with interrupts disabled. No?

You are right, I overlooked this and left the code unchanged from a
previous patch series.
According to the Power State Coordination Interface (DEN0022F.b 1.3):
```
6.4.3.3 Exceptions
The appropriate asynchronous exception masks must be set when starting
up the core at the Exception
level from which the call was made, or at ELns. Typically, this means
that for the Exception level where
execution is restarting:

If starting in AArch64 state, the SPSR_ELx.{D,A,I,F} bits must be set
to {1, 1, 1, 1}. ELx is the
Exception level being returned from.
```

The ARM Trusted Firmware code enforces this correctly here:
https://elixir.bootlin.com/arm-trusted-firmware/v2.13.0/source/lib/psci/psci_common.c#L869

So, the code should indeed expect DAIF bits to be set on resume. I
will update the patch accordingly.

>
> > +
> > +        tlbi  alle2
> > +        dsb   sy                     /* Ensure completion of TLB flush */
>
> This doesn't exist when booting Xen and I am not sure why we would need
> it for resume as we already nuke the TLbs in enable_mmu. Can you clarify?

You're absolutely right — that line is a leftover from earlier changes
when the memory handling logic was being reorganized.
It's no longer necessary because enable_mmu already handles TLB
invalidation, including the TLBI and DSB instructions.
I'll remove it in the next version. Thanks for catching this!

>
> > +        isb
> > +
> > +        ldr   x0, =start
> > +        adr   x19, start             /* x19 := paddr (start) */
> > +        sub   x20, x19, x0           /* x20 := phys-offset */
>
> Looking at the code, I wonder if it is still necessary to set x19 and
> x20 for booting secondary CPUs and resume. There doesn't seem to be any
> use of the registers.

x20 is still required during resume. It's used inside
enable_secondary_cpu_mm via the load_paddr macro.
So although x19 may no longer be necessary in this context, x20 is
still used and needs to be set beforehand.

>
> > +
> > +        /* Initialize the UART if earlyprintk has been enabled. */
> > +#ifdef CONFIG_EARLY_PRINTK
> > +        bl    init_uart
> > +#endif
> > +        PRINT_ID("- Xen resuming -\r\n")
> > +
> > +        bl    check_cpu_mode
> > +        bl    cpu_init
> > +
> > +        ldr   lr, =mmu_resumed
> > +        b     enable_secondary_cpu_mm
> > +
> > +mmu_resumed:
> >           b .
> >   END(hyp_resume)
> >
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index ffcae900d7..3a89ac436b 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -508,6 +508,14 @@ void asmlinkage __init start_xen(unsigned long 
> > fdt_paddr)
> >       for_each_domain( d )
> >           domain_unpause_by_systemcontroller(d);
> >
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +    /*
> > +     * It is called to initialize init_ttbr.
> > +     * Without this call, Xen gets stuck after resuming.
>
> This is not a very descriptive comment. But, you seem to make the
> assumption that prepare_secondary_mm() can be called on the boot CPU.
> This is not always the case. For instance on arm32, it will allocate
> memory and overwrite the per-cpu page-tables pointer (not great). This
> will also soon be the case for arm64.
>
> Furthermore, this call reminded me that the secondary CPU page-tables
> are not freed when turning off a CPU. So they will leak. Not yet a
> problem for arm64 though.
>
> So overall, I think we need a separate function that will be prepare
> init_ttbr for a given CPU (not allocate any memory). This will then need
> to be called from the suspend helper.

Thank you for the detailed explanation.

I will rework this part to avoid calling prepare_secondary_mm() on
the boot CPU. As suggested, I plan to introduce a dedicated helper
function that will only initialize init_ttbr without allocating
memory and call it  from the suspend helper.

>
> > +     */
>  > +    prepare_secondary_mm(0);> +#endif
> > +
> >       /* Switch on to the dynamically allocated stack for the idle vcpu
> >        * since the static one we're running on is about to be freed. */
> >       memcpy(idle_vcpu[0]->arch.cpu_info, get_cpu_info(),
>
> Cheers,
>
> --
> Julien Grall
>

Thank you for reviewing this patch series.

Best regards,
Mykola