| Message ID | 1523551221-11612-4-git-send-email-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12 April 2018 at 17:40, Igor Mammedov <imammedo@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 <imammedo@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.maydell@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). 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... thanks -- PMM
On Thu, 12 Apr 2018 19:29:28 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On 12 April 2018 at 17:40, Igor Mammedov <imammedo@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 <imammedo@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.maydell@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
On 12 April 2018 at 19:29, Peter Maydell <peter.maydell@linaro.org> wrote: > On 12 April 2018 at 17:40, Igor Mammedov <imammedo@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 <imammedo@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.maydell@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). > > 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... I did the check, and in fact all of our boards always pass the first CPU as the boot CPU. Xilinx comes the closest, in that the SoC object has a boot-cpu property to pick a boot CPU, but since none of our boards set that it defaults to the first CPU. Since this patch isn't very related to the rest of the series I've just applied it to target-arm.next ready for 2.13 (with a note in the commit message that in practice it isn't a behaviour change). Let me know if that's awkward for you and you'd prefer to keep it in this series, in which case I'll drop it. thanks -- PMM
On Mon, 16 Apr 2018 18:17:51 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On 12 April 2018 at 19:29, Peter Maydell <peter.maydell@linaro.org> wrote: > > On 12 April 2018 at 17:40, Igor Mammedov <imammedo@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 <imammedo@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.maydell@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). > > > > 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... > > I did the check, and in fact all of our boards always pass the > first CPU as the boot CPU. Xilinx comes the closest, in that > the SoC object has a boot-cpu property to pick a boot CPU, but > since none of our boards set that it defaults to the first CPU. > > Since this patch isn't very related to the rest of the series > I've just applied it to target-arm.next ready for 2.13 (with > a note in the commit message that in practice it isn't a behaviour > change). Let me know if that's awkward for you and you'd prefer > to keep it in this series, in which case I'll drop it. That's fine, I'll just rebase v2 on top of it. > > thanks > -- PMM
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)); } }
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 <imammedo@redhat.com> --- hw/arm/boot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)