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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 18 Apr 2018, Julien Grall wrote:
> On 18/04/18 11:45, Mirela Simonovic wrote:
> > 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.
> 
> 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?

Either way it is of very little consequence. I would let Mirela do as
she prefer.

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