On Thu, 12 Apr 2018 19:29:28 +0100
Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 12 April 2018 at 17:40, Igor Mammedov <imamm...@redhat.com> wrote:
> > if arm_load_kernel() were passed non first_cpu, QEMU would end up
> > with partially set do_cpu_reset() callback leaving some CPUs without it.
> >
> > Make sure that do_cpu_reset() is registered for all CPUs by enumerating
> > CPUs from first_cpu.
> >
> > Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> > ---
> > hw/arm/boot.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > index 2f464ca..2591fee 100644
> > --- a/hw/arm/boot.c
> > +++ b/hw/arm/boot.c
> > @@ -1188,7 +1188,7 @@ void arm_load_kernel(ARMCPU *cpu, struct
> > arm_boot_info *info)
> > * actually loading a kernel, the handler is also responsible for
> > * arranging that we start it correctly.
> > */
> > - for (cs = CPU(cpu); cs; cs = CPU_NEXT(cs)) {
> > + for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
> > qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
> > }
> > }
>
> Definitely a bug fix, so:
> Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
>
> I think though that in at least some cases we'll still mishandle
> being passed anything other than first_cpu as the CPU pointer,
> because in do_cpu_reset() we do some checks for "do this if
> cs == first_cpu", on the assumption that first_cpu is the
> primary CPU that we're booting. We should instead I suppose
> be checking against the CPU pointer we're given as the
> arm_load_kernel() argument (which I think do_cpu_reset() can
> get at via info->load_kernel_notifier.cpu).
Agreed,
only that load_kernel_notifier is being removed in 4/4,
but nothing prevents us from adding pointer to arm_boot_info directly.
> We should probably analyse which boards actually pass anything
> other than first_cpu -- I suspect it will end up just being
> the xilinx board which has both A and R profile cores...
Probably,
I've managed to stop myself digging deeper into reset handling for now,
and deal with firmware blobs regeneration first.
I still might have to look back at do_cpu_reset() later
if it proves to be in a way of cpu hotplug but not just yet.
> thanks
> -- PMM
