Re: [Xen-devel] [PATCH 1/7] xen/arm: mm: Consolidate setting SCTLR_EL2.WXN in a single place

[Julien Grall <Julien.Grall@xxxxxxx>]

Hi,

On 25/04/2019 19:00, Andrii Anisov wrote:
> 
> 
> On 17.04.19 20:58, Julien Grall wrote:
>> The logic to set SCTLR_EL2.WXN is the same for the boot CPU and
>> non-boot CPU. So introduce a function to set the bit and clear TBLs.
> s/TBL/TLB/
> 
>>
>> This new function will help us to document and update the logic in a
>> single place.
>>
>> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> 
>> ---
>>   xen/arch/arm/mm.c | 22 +++++++++++++++-------
>>   1 file changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 01ae2cccc0..93ad118183 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -601,6 +601,19 @@ void __init remove_early_mappings(void)
>>       flush_xen_data_tlb_range_va(BOOT_FDT_VIRT_START, 
>> BOOT_FDT_SLOT_SIZE);
>>   }
>> +/*
>> + * After boot, Xen page-tables should not contain mapping that are both
>> + * Writable and eXecutables.
>> + *
>> + * This should be called on each CPU to enforce the policy.
>> + */
>> +static void xen_pt_enforce_wnx(void)
> Could it be inline?

Why can't we let the compiler deciding for us? The more that inline is 
pretty broken. See:

https://www.kernel.org/doc/local/inline.html

> 
>> +{
>> +    WRITE_SYSREG32(READ_SYSREG32(SCTLR_EL2) | SCTLR_WXN, SCTLR_EL2);
>> +    /* Flush everything after setting WXN bit. */
>> +    flush_xen_text_tlb_local();
>> +}
>> +
>>   extern void switch_ttbr(uint64_t ttbr);
>>   /* Clear a translation table and clean & invalidate the cache */
>> @@ -702,10 +715,7 @@ void __init setup_pagetables(unsigned long 
>> boot_phys_offset)
>>       clear_table(boot_second);
>>       clear_table(boot_third);
>> -    /* From now on, no mapping may be both writable and executable. */
>> -    WRITE_SYSREG32(READ_SYSREG32(SCTLR_EL2) | SCTLR_WXN, SCTLR_EL2);
>> -    /* Flush everything after setting WXN bit. */
>> -    flush_xen_text_tlb_local();
>> +    xen_pt_enforce_wnx();
>>   #ifdef CONFIG_ARM_32
>>       per_cpu(xen_pgtable, 0) = cpu0_pgtable;
>> @@ -777,9 +787,7 @@ int init_secondary_pagetables(int cpu)
>>   /* MMU setup for secondary CPUS (which already have paging enabled) */
>>   void mmu_init_secondary_cpu(void)
>>   {
>> -    /* From now on, no mapping may be both writable and executable. */
>> -    WRITE_SYSREG32(READ_SYSREG32(SCTLR_EL2) | SCTLR_WXN, SCTLR_EL2);
>> -    flush_xen_text_tlb_local();
>> +    xen_pt_enforce_wnx();
>>   }
>>   #ifdef CONFIG_ARM_32
>>
> 
> With minor notes,
> 
> Reviewed-by: Andrii Anisov <andrii_anisov@xxxxxxxx>

Thank you!

Cheers,

-- 
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel