Re: [Xen-devel] [PATCH 6/7] xen/arm: Setup virtual paging for secondary CPUs in non-boot scenario

[Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>]

On Tue, Apr 17, 2018 at 4:11 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
>
>
> On 17/04/18 13:54, Mirela Simonovic wrote:
>>
>> Hi Julien,
>
>
> Hi,
>
>>
>> On Wed, Apr 11, 2018 at 5:11 PM, Julien Grall <julien.grall@xxxxxxx>
>> wrote:
>>>
>>> Hi,
>>>
>>> On 11/04/18 14:19, Mirela Simonovic wrote:
>>>>
>>>>
>>>> In existing code the paging for secondary CPUs is setup only in boot
>>>> flow.
>>>> The setup is triggered from start_xen function 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
>>>> should be self-contained - it should fully initialize a secondary CPU,
>>>> because the cpu_up is used not only to bring a secondary CPU online on
>>>> boot, but also to hotplug a CPU during the system resume.
>>>> 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
>>>> will be setup in non-boot scenarios, while the setup in boot scenario
>>>> remains unchanged.
>>>
>>>
>>>
>>> I am afraid that this is not correct. You can't assume that value chosen
>>> for
>>> VTCR by Xen at boot will fit this new CPU. So you have to check it is
>>> fine
>>> or park the CPU if there are any issue.
>>>
>>
>> This is not a new CPU. This CPU already went through its boot sequence
>> and it reached the resume point because it does fit the value chosen
>> for VTCR by Xen.
>> If it wouldn't fit the chosen value for VTCR it would be parked so it
>> wouldn't participate in suspend/resume. Please let me know if I
>> misunderstood your comment.
>
>
> This is not a new CPU for your use case. However your commit message
> speak about "non-boot" CPU bring-up. So for me this is more than
> suspend/resume, it is about bringing-up CPU at any time.
>
> As those CPUs can't participate to the decision (it is too late), you
> need to make sure the VTCR will fit on that CPU.
>
>>
>> AFAIU the value chosen by Xen for VTCR config has to be common for all
>> online CPUs. Since this value is also used in the resume path I
>> suggest to make global (static in the p2m.c) the 'val' variable which
>> is currently local in setup_virt_paging() and passed as argument to
>> setup_virt_paging_one(). Then setup_virt_paging_one() would not
>> receive an argument.
>> I need to access this value on resume, so I would call
>> setup_virt_paging_one() without argument from start_secondary() if the
>> system state is not boot.
>> This seems to me a bit cleaner compared to what I submitted in this
>> patch, but fundamentally the functionality is the same.
>
>
> You don't need to introduce a static variable it. I believe you can
> re-create it based on the information we already have in global
> variables. So what I would do is moving the creation of vtcr value in
> that function.

Using this approach each CPU will need to recalculate the value which
is already known prior to executing the function.
I believe this is sub-optimal and contrary to existing implementation
where only one CPU performs the calculation.
Is there any benefit of recalculating the value?
Is there any disadvantage of remembering the value into a static variable?

Please let me know, I just need to understand whether there is any
particular reason why you want to repeat the calculation. If no
particular reasons, saving the calculated value is definitely a better
approach.

Thanks,
Mirela

>
> Cheers,
>
> --
> Julien Grall
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium. Thank you.

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