Re: [PATCH v8 3/8] xen/arm: Fold mmu_init_secondary_cpu() to head.S

[Julien Grall <julien@xxxxxxx>]

On 01/11/2023 01:58, Henry Wang wrote:
> Hi Julien,
Hi,

> > On Nov 1, 2023, at 02:29, Julien Grall <julien@xxxxxxx> wrote:
> >
> > Hi Henry,
> >
> > +Ayan
> >
> > On 23/10/2023 03:13, Henry Wang wrote:
> > > Currently mmu_init_secondary_cpu() only enforces the page table
> > > should not contain mapping that are both Writable and eXecutables
> > > after boot. To ease the arch/arm/mm.c split work, fold this function
> > > to head.S.
> > > For arm32, introduce an assembly macro pt_enforce_wxn. The macro is
> > > called before secondary CPUs jumping into the C world.
> > > For arm64, set the SCTLR_Axx_ELx_WXN flag right when the MMU is
> > > enabled. This would avoid the extra TLB flush and SCTLR dance.
> > For a random reader, it is not clear why you can't set WnX early for arm32 as 
> > well. I think it would helpful to explain the difference. I.e. at the point the 
> > MMU is enabled, the page-tables may still contain mapping which are writable 
> > and executable.
> Sounds good, I will add the suggested sentence.
>
> > >   .endm
> > >   +/*
> > > + * Enforce Xen page-tables do not contain mapping that are both
> > > + * Writable and eXecutables.
> > > + *
> > > + * This should be called on each secondary CPU.
> > > + */
> > > +.macro pt_enforce_wxn tmp
> > > +        mrc   CP32(\tmp, HSCTLR)
> > > +        orr   \tmp, \tmp, #SCTLR_Axx_ELx_WXN
> > > +        dsb
> > > +        mcr   CP32(\tmp, HSCTLR)
> > > +        /*
> > > +         * The TLBs may cache SCTLR_EL2.WXN. So ensure it is synchronized
> > > +         * before flushing the TLBs.
> > > +         */
> > > +        isb
> > > +        flush_xen_tlb_local \tmp
> > > +.endm
> > > +
> > >   /*
> > >    * Common register usage in this file:
> > >    *   r0  -
> > > @@ -254,6 +273,7 @@ secondary_switched:
> > >           /* Use a virtual address to access the UART. */
> > >           mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
> > >   #endif
> > > +        pt_enforce_wxn r0
> >  From recent discussion on IRC, Ayan reminded me this patch [1]. Ideally, I 
> > would want to print a message just before to indicate that the bit is set. But 
> > I understand that this would need to be droppped in Ayan rework as we don't yet 
> > support early printk in enable_mmu().
> >
> > While debugging an MMU issue on Arm32, I wrote a patch to sprinkle prints in 
> > the enable_mmu() code. I will clean-up the patch and send it.
> Just to make sure, your patch is for both Arm32 and Arm64, is my understanding 
> correct?
No it is only for arm32.

> If it is only for Arm32, do you need me adding the print for Arm64 as well in 
> this patch?
No need. For arm64, we will enable WnX at the same time as the MMU. So 
we are already covered by the other prints.
> > I will add a print at that point. Meanwhile, I would move the call a few lines 
> > above? This will allow Ayan to drop [1].
> Yeah I will include Ayan’s change in this patch and add his sign-off.
>
> > >           PRINT("- Ready -\r\n")
> > >           /* Jump to C world */
> > >           mov_w r2, start_secondary
> > > diff --git a/xen/arch/arm/arm64/mmu/head.S b/xen/arch/arm/arm64/mmu/head.S
> > > index 88075ef083..df06cefbbe 100644
> > > --- a/xen/arch/arm/arm64/mmu/head.S
> > > +++ b/xen/arch/arm/arm64/mmu/head.S
> > > @@ -264,10 +264,11 @@ ENDPROC(create_page_tables)
> > >    * Inputs:
> > >    *   x0 : Physical address of the page tables.
> > The inputs list should be updated to mention what x1 means.
> I will use “x1: Extra flags of the SCTLR.” if this looks good to you.
I am fine with that.

Cheers,


--
Julien Grall