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

[Mirela Simonovic <mirela.simonovic@aggios.com>]

Hi Julien,

On Wed, Apr 18, 2018 at 11:48 AM, Julien Grall <julien.grall@arm.com> wrote:
>
>
> On 17/04/18 16:22, Mirela Simonovic wrote:
>>
>> Hi Julien,
>>
>> 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.
>>>
>>
>> Use case you're trying to cover is hotplugging a CPU after the boot is
>> done in bit.LITTLE system, and that CPU wasn't initially brought
>> online (on boot). Right?
>
>
> That's right. It is how I understood your commit title.
>
>>
>>> 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.
>>>
>>
>> Could you please point me to the location in sources where this is
>> done on boot? I mean checking compliance with chosen VTCR value and
>> parking CPU if it doesn't fit.
>
>
> At the moment all CPUs are brought up during Xen boot. So what we do is find
> the small PA range that will accommodate all the onlined CPUs (see
> setup_virt_paging).
>
> So there are no parking required at the moment. However, if you start
> bringing-up new CPUs after boot. Then you have to check they will be able to
> handle the values chosen at boot.
>
> As you pointed out, this might not be necessary for the suspend/resume case.
> So I am not asking you to implement that. However, please make sure the
> commit message clearly spell out that you bringing up secondary CPU after
> Xen has boot only covers the suspend/resume case.
>

I'll fix the commit title, thanks.

> Cheers,
>
> --
> Julien Grall

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