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

[Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>]

Hi Julien,

Thanks for the feedback.

On Mon, Apr 23, 2018 at 1:28 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hi Mirela,
>
>
> On 20/04/18 13:25, Mirela Simonovic wrote:
>>
>> In existing code the 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 if the current system state is not boot. This way, the paging
>> for secondary CPUs will be setup in non-boot scenarios as well, while the
>> 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, these in 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)
>> ---
>>   xen/arch/arm/p2m.c        | 13 ++++++++++++-
>>   xen/arch/arm/smpboot.c    |  3 +++
>>   xen/include/asm-arm/p2m.h |  3 +++
>>   3 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index d43c3aa896..9bb62c13cd 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1451,13 +1451,21 @@ err:
>>       return page;
>>   }
>>   -static void __init setup_virt_paging_one(void *data)
>> +static void setup_virt_paging_one(void *data)
>>   {
>>       unsigned long val = (unsigned long)data;
>>       WRITE_SYSREG32(val, VTCR_EL2);
>>       isb();
>>   }
>>   +/* VTCR value to be configured by all CPUs. Set only once by the boot
>> CPU */
>> +static unsigned long __read_mostly vtcr_value;
>
>
> VTCR is a register, so the type should be represented in term of bits (i.e
> uint*_t).

I followed the type used in setup_virt_paging() and it's unsigned
long. However, the spec says the VTCR_EL2 is 32-bit register, meaning
that in the current implementation the type is not correct.
If I want the type to be correct in this patch Xen will not compile.
Are you suggesting to fix the type in existing implementation?

>
>> +
>> +void setup_virt_paging_secondary(void)
>> +{
>> +    setup_virt_paging_one((void *)vtcr_value);
>
>
> That's fairly ugly. Is there any way to rework the interface? For instance,
> because you have a static variable which contain the VTCR, you could just
> use the variable in setup_virt_paging one.
>

If the argument provided to setup_virt_paging_one() is NULL within the
setup_virt_paging_one() I configure saved static vtcr_value? If that
is what you meant it was submitted in previous version of this patch
:)
Are you suggesting to revert the change to v1?

>> +}
>> +
>>   void __init setup_virt_paging(void)
>>   {
>>       /* Setup Stage 2 address translation */
>> @@ -1540,6 +1548,9 @@ void __init setup_virt_paging(void)
>>       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);
>> +
>> +    /* Save configured value (to be used later for secondary CPUs
>> hotplug) */
>> +    vtcr_value = val;
>>   }
>>     /*
>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>> index 38b665a6d2..abc642804f 100644
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> @@ -358,6 +358,9 @@ void start_secondary(unsigned long boot_phys_offset,
>>       local_irq_enable();
>>       local_abort_enable();
>>   +    if ( system_state != SYS_STATE_boot )
>> +        setup_virt_paging_secondary();
>
> I think this code needs some documentation. So people understand why you
> only call setup_virt_paging_secondary() after boot. But is there any reason
> to not use a notifier (see notify_cpu_starting)? This would avoid yet
> another export.

It works using notifiers, but I wouldn't say it's worth the overhead.
Implementation using notifiers requires 1 additional data structure
(notifier_block) and 2 functions to be implemented (an __init function
to register a notifier and another one for callback).
Moreover, such a callback would be called for each CPU event, which is
3 times when the CPU is hot-unplugged and nothing needs to be done.

I've already implemented notifier-based approach so I have no problem
submitting it. However, I'm not sure what you want to trade. Please
lets clarify.

>
>> +
>>       check_local_cpu_errata();
>>         printk(XENLOG_DEBUG "CPU %u booted.\n", smp_processor_id());
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index 8823707c17..85b66a1196 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -153,6 +153,9 @@ void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
>>   /* Second stage paging setup, to be called on all CPUs */
>>   void setup_virt_paging(void);
>>   +/* To be called by secondary CPU on hotplug */
>> +void setup_virt_paging_secondary(void);
>> +
>>   /* Init the datastructures for later use by the p2m code */
>>   int p2m_init(struct domain *d);
>>
>
>
> Cheers,
>
> --
> Julien Grall

Thanks,
Mirela

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