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

[Julien Grall <julien.grall@xxxxxxx>]

On 17/04/18 16:22, Mirela Simonovic wrote:
> Hi Julien,
>
> 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.
> 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.
Cheers,

--
Julien Grall

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