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 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.

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