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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> 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?
If it is only for Arm32, do you need me adding the print for Arm64 as well in 
this patch?

> 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.

>>   *
>> - * Clobbers x0 - x4
>> + * Clobbers x0 - x6
> 
> Below, you only seem to introduce x5. So shouldn't this be: "Clobbers x0 - 
> x5"?

Hmmm yes you are correct, I blindly copied the code from [2]. Sorry for the 
mess, I will
correct it in v9.

> Cheers,
> 
> [1] 
> https://lore.kernel.org/all/20230911135942.791206-2-ayan.kumar.halder@xxxxxxx/

[2] 
https://lore.kernel.org/xen-devel/4d7a9849-8990-8ddd-3531-93f4e2e262b1@xxxxxxx/

Kind regards,
Henry

> 
> -- 
> Julien Grall