Re: [Xen-devel] [PATCH v3 05/10] xen/arm: Setup virtual paging for non-boot CPUs on hotplug/resume

[Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>]

Hi Julien,


On Mon, Apr 30, 2018 at 4:47 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hi Mirela,
>
>
> On 27/04/18 18:12, Mirela Simonovic wrote:
>>
>> In existing code the virtual paging for non-boot CPUs is setup only on
>> boot.
>> The setup is triggered from start_xen() after all CPUs are brought online.
>> In other words, the initialization of VTCR_EL2 register is done out of the
>> cpu_up/start_secondary() control flow. However, the cpu_up flow is also
>> used
>> to hotplug non-boot CPUs on resume from suspend to RAM state, in which
>> case
>> the virtual paging will not be configured.
>>
>> With this patch the setting of paging is triggered from start_secondary()
>> function using cpu starting notifier (notify_cpu_starting() call). The
>> notifier is registered in p2m.c using init call. This has to be done with
>> init call rather than presmp_init because the registered callback depends
>> on vtcr configuration value which is setup after the presmp init calls
>> are executed (do_presmp_initcalls() called from start_xen()). Init calls
>> are executed after initial virtual paging is set up for all CPUs on boot.
>> This ensures that no callback can fire until the vtcr value is calculated
>> by Xen and virtual paging is set up initially for all CPUs. Also, this way
>> the virtual paging setup in boot scenario remains unchanged.
>>
>> It is assumed here that after the system completed the boot, CPUs that
>> execute start_secondary() were booted as well when the Xen itself was
>> booted. According to this assumption non-boot CPUs will always be
>> compliant
>> with the VTCR_EL2 value that was selected by Xen on boot.
>> Currently, there is no mechanism to trigger hotplugging of a CPU. This
>> will be added with the suspend to RAM support for ARM, where the hotplug
>> of non-boot CPUs will be triggered via enable_nonboot_cpus() call.
>>
>> Signed-off-by: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
>>
>> ---
>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> CC: Julien Grall <julien.grall@xxxxxxx>
>> ---
>> Changes in v2:
>> -Fix commit message
>> -Save configured VTCR_EL2 value into static variable that will be used
>>   by non-boot CPUs on hotplug
>> -Add setup_virt_paging_secondary() and invoke it from start_secondary()
>>   if that CPU has to setup virtual paging (if the system state is not
>> boot)
>>
>> Changes in v3:
>> -Fix commit message
>> -Remove setup_virt_paging_secondary() and use notifier to setup virtual
>>   paging for non-boot CPU on hotplug.
>> -In setup_virt_paging() use vtcr static variable instead of local val
>> -In setup_virt_paging_one() use vtcr static variable instead of provided
>>   argument
>> ---
>>   xen/arch/arm/p2m.c | 82
>> +++++++++++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 62 insertions(+), 20 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index d43c3aa896..98a1fe6de9 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -8,6 +8,8 @@
>>   #include <xen/iocap.h>
>>   #include <xen/mem_access.h>
>>   #include <xen/xmalloc.h>
>> +#include <xen/notifier.h>
>> +#include <xen/cpu.h>
>
>
> Please add them alphabetically.
>
>
>>   #include <public/vm_event.h>
>>   #include <asm/flushtlb.h>
>>   #include <asm/event.h>
>> @@ -1451,24 +1453,17 @@ err:
>>       return page;
>>   }
>>   -static void __init setup_virt_paging_one(void *data)
>> +/* VTCR value to be configured by all CPUs. Set only once by the boot CPU
>> */
>> +static uint64_t __read_mostly vtcr;
>> +
>> +static void setup_virt_paging_one(void *data)
>>   {
>> -    unsigned long val = (unsigned long)data;
>> -    WRITE_SYSREG32(val, VTCR_EL2);
>> +    WRITE_SYSREG32(vtcr, VTCR_EL2);
>>       isb();
>>   }
>>     void __init setup_virt_paging(void)
>>   {
>> -    /* Setup Stage 2 address translation */
>> -    unsigned long val =
>> VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
>> -
>> -#ifdef CONFIG_ARM_32
>> -    printk("P2M: 40-bit IPA\n");
>> -    p2m_ipa_bits = 40;
>> -    val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
>> -    val |= VTCR_SL0(0x1); /* P2M starts at first level */
>> -#else /* CONFIG_ARM_64 */
>>       const struct {
>>           unsigned int pabits; /* Physical Address Size */
>>           unsigned int t0sz;   /* Desired T0SZ, minimum in comment */
>> @@ -1491,6 +1486,16 @@ void __init setup_virt_paging(void)
>>       unsigned int pa_range = 0x10; /* Larger than any possible value */
>>       bool vmid_8_bit = false;
>
>
> That's not going to build on arm32 because vmid_8_bit & co are not used.
> While I will not ask you to test the changes on 32-bit board, I would at
> least want each change to be build test it on impacted architecture.
>
> In that particular case, you can just move the initialization of vtcr at
> after the endif because there is nothing that required that to be setup very
> early.
>
>
>>   +    /* Setup Stage 2 address translation */
>> +    vtcr = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
>> +
>> +#ifdef CONFIG_ARM_32
>> +    printk("P2M: 40-bit IPA\n");
>> +    p2m_ipa_bits = 40;
>> +    vtcr |= VTCR_T0SZ(0x18); /* 40 bit IPA */
>> +    vtcr |= VTCR_SL0(0x1); /* P2M starts at first level */
>> +#else /* CONFIG_ARM_64 */
>> +
>>       for_each_online_cpu ( cpu )
>>       {
>>           const struct cpuinfo_arm *info = &cpu_data[cpu];
>> @@ -1513,14 +1518,14 @@ void __init setup_virt_paging(void)
>>       if ( pa_range >= ARRAY_SIZE(pa_range_info) ||
>> !pa_range_info[pa_range].pabits )
>>           panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n",
>> pa_range);
>>   -    val |= VTCR_PS(pa_range);
>> -    val |= VTCR_TG0_4K;
>> +    vtcr |= VTCR_PS(pa_range);
>> +    vtcr |= VTCR_TG0_4K;
>>         /* Set the VS bit only if 16 bit VMID is supported. */
>>       if ( MAX_VMID == MAX_VMID_16_BIT )
>> -        val |= VTCR_VS;
>> -    val |= VTCR_SL0(pa_range_info[pa_range].sl0);
>> -    val |= VTCR_T0SZ(pa_range_info[pa_range].t0sz);
>> +        vtcr |= VTCR_VS;
>> +    vtcr |= VTCR_SL0(pa_range_info[pa_range].sl0);
>> +    vtcr |= VTCR_T0SZ(pa_range_info[pa_range].t0sz);
>>         p2m_root_order = pa_range_info[pa_range].root_order;
>>       p2m_root_level = 2 - pa_range_info[pa_range].sl0;
>> @@ -1532,16 +1537,53 @@ void __init setup_virt_paging(void)
>>              ( MAX_VMID == MAX_VMID_16_BIT ) ? 16 : 8);
>>   #endif
>>       printk("P2M: %d levels with order-%d root, VTCR 0x%lx\n",
>> -           4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, val);
>> +           4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, vtcr);
>>         p2m_vmid_allocator_init();
>>         /* It is not allowed to concatenate a level zero root */
>>       BUG_ON( P2M_ROOT_LEVEL == 0 && P2M_ROOT_ORDER > 0 );
>> -    setup_virt_paging_one((void *)val);
>> -    smp_call_function(setup_virt_paging_one, (void *)val, 1);
>> +    setup_virt_paging_one(NULL);
>> +    smp_call_function(setup_virt_paging_one, NULL, 1);
>> +}
>> +
>> +static int cpu_virt_paging_callback(
>> +    struct notifier_block *nfb, unsigned long action, void *hcpu)
>
>
> The indentation looks wrong.
>

Editor indented this for me and it looks the same as in other places
where a notifier is defined. I did
grep -r "struct notifier_block \*nfb,"
to check. It looks weird but seems correct

>> +{
>> +    switch ( action )
>> +    {
>> +    case CPU_STARTING:
>> +        ASSERT(system_state != SYS_STATE_boot);
>
>
> I was about to complain about this but then saw you add the notifiers after.
> That's quite clever and the comment is really helpful :).
>
>> +        setup_virt_paging_one(NULL);
>> +        break;
>> +    default:
>> +        break;
>> +    }
>> +
>> +    return NOTIFY_DONE;
>>   }
>>   +static struct notifier_block cpu_virt_paging_nfb = {
>> +    .notifier_call = cpu_virt_paging_callback,
>> +    .priority = 100 /* highest priority */
>> +};
>> +
>> +static int __init cpu_virt_paging_init(void)
>> +{
>> +    register_cpu_notifier(&cpu_virt_paging_nfb);
>> +    return 0;
>> +}
>
>
> NIT: Missing newline.
>
>> +/*
>> + * Initialization of the notifier has to be done at init rather than
>> presmp_init
>> + * phase because: the registered notifier is used to setup virtual paging
>> for
>> + * non-boot CPUs after the initial virtual paging for all CPUs is already
>> setup,
>> + * i.e. when a non-boot CPU is hotplugged after the system has booted. In
>> other
>> + * words, the notifier should be registered after the virtual paging is
>> + * initially setup (setup_virt_paging() is called from start_xen()). This
>> is
>> + * required because vtcr config value has to be set before a notifier can
>> fire.
>> + */
>> +__initcall(cpu_virt_paging_init);
>> +
>>   /*
>>    * Local variables:
>>    * mode: C
>>
>
> Cheers,
>
> --
> Julien Grall

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