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

[Julien Grall <julien.grall@arm.com>]

On 18/04/18 11:45, Mirela Simonovic wrote:
> On Tue, Apr 17, 2018 at 4:11 PM, Julien Grall <julien.grall@arm.com> 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@arm.com>
>>> 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.

The implementation usually evolves with the requirements. In the 
existing implementation, the value could be calculated on a single pCPU 
and then scratched afterwards.

> Is there any benefit of recalculating the value? > Is there any disadvantage of remembering the value into a static 
variable?

I know that it is only a 32-bit value, but I would rather avoid 
spreading static variable when a value can be recompute in a few steps 
with what we have.

Stefano do you have any opinions?

Cheers,

-- 
Julien Grall

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