| Message ID | 1409930126-28449-5-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 5 September 2014 16:15, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > When booting with -bios or -pflash rather than -kernel, we need to make sure > reset handlers are registered. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > hw/arm/boot.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c > index 8f5649a250fd..0cfc11d42962 100644 > --- a/hw/arm/boot.c > +++ b/hw/arm/boot.c > @@ -473,6 +473,10 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) > } > } > > + for (; cs; cs = CPU_NEXT(cs)) { > + qemu_register_reset(do_cpu_reset, ARM_CPU(cs)); > + } > + > /* If no kernel specified, do nothing; we will start from address 0 > * (typically a boot ROM image) in the same way as hardware. > */ Andreas, with QOM CPUs who is supposed to be responsible for causing them to be reset when qemu_system_reset() happens? At the moment for ARM it doesn't happen at all unless you happened to pass -kernel. This patch makes boot.c register the reset handlers, but that seems a bit odd to me (among other things, it won't work for v7M). thanks -- PMM
On 9 September 2014 11:14, Peter Maydell <peter.maydell@linaro.org> wrote: > On 5 September 2014 16:15, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> When booting with -bios or -pflash rather than -kernel, we need to make sure >> reset handlers are registered. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> hw/arm/boot.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/hw/arm/boot.c b/hw/arm/boot.c >> index 8f5649a250fd..0cfc11d42962 100644 >> --- a/hw/arm/boot.c >> +++ b/hw/arm/boot.c >> @@ -473,6 +473,10 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) >> } >> } >> >> + for (; cs; cs = CPU_NEXT(cs)) { >> + qemu_register_reset(do_cpu_reset, ARM_CPU(cs)); >> + } >> + >> /* If no kernel specified, do nothing; we will start from address 0 >> * (typically a boot ROM image) in the same way as hardware. >> */ > > Andreas, with QOM CPUs who is supposed to be responsible > for causing them to be reset when qemu_system_reset() > happens? At the moment for ARM it doesn't happen at all > unless you happened to pass -kernel. This patch makes > boot.c register the reset handlers, but that seems > a bit odd to me (among other things, it won't work > for v7M). > Has there been any discussion on this topic in the mean time? Cheers, Ard.
Am 17.09.2014 um 17:50 schrieb Ard Biesheuvel: > On 9 September 2014 11:14, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 5 September 2014 16:15, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>> When booting with -bios or -pflash rather than -kernel, we need to make sure >>> reset handlers are registered. >>> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> --- >>> hw/arm/boot.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c >>> index 8f5649a250fd..0cfc11d42962 100644 >>> --- a/hw/arm/boot.c >>> +++ b/hw/arm/boot.c >>> @@ -473,6 +473,10 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) >>> } >>> } >>> >>> + for (; cs; cs = CPU_NEXT(cs)) { >>> + qemu_register_reset(do_cpu_reset, ARM_CPU(cs)); >>> + } >>> + >>> /* If no kernel specified, do nothing; we will start from address 0 >>> * (typically a boot ROM image) in the same way as hardware. >>> */ >> >> Andreas, with QOM CPUs who is supposed to be responsible >> for causing them to be reset when qemu_system_reset() >> happens? At the moment for ARM it doesn't happen at all >> unless you happened to pass -kernel. This patch makes >> boot.c register the reset handlers, but that seems >> a bit odd to me (among other things, it won't work >> for v7M). >> > > Has there been any discussion on this topic in the mean time? No, thanks for the ping. IIRC each machine is responsible for registering a reset hook that calls - in most cases - cpu_reset(). The thing to look out for here is, does any machine already register a reset hook and would reset twice with this patch? What's the issue with v7-M? Regards, Andreas
On 17 September 2014 08:55, Andreas Färber <afaerber@suse.de> wrote: > IIRC each machine is responsible for registering a reset hook that calls > - in most cases - cpu_reset(). > > The thing to look out for here is, does any machine already register a > reset hook and would reset twice with this patch? Probably not -- any such double-reset would already be happening if the user passed -kernel. So in that sense this patch won't break things, but I wasn't sure if it's the right direction to go -- should we be fixing all the board and/or SoC models to do the CPU reset instead? QOM devices get reset when their bus gets reset, right? (so everything on a bus gets reset eventually as part of the process that starts when the top level sysbus gets reset). > What's the issue with v7-M? Probably also broken, but IIRC we don't actually wire up the register that's supposed to trigger a system reset... -- PMM
Am 17.09.2014 um 18:17 schrieb Peter Maydell: > On 17 September 2014 08:55, Andreas Färber <afaerber@suse.de> wrote: >> IIRC each machine is responsible for registering a reset hook that calls >> - in most cases - cpu_reset(). >> >> The thing to look out for here is, does any machine already register a >> reset hook and would reset twice with this patch? > > Probably not -- any such double-reset would already be happening > if the user passed -kernel. > > So in that sense this patch won't break things, but I wasn't > sure if it's the right direction to go -- should we be fixing > all the board and/or SoC models to do the CPU reset instead? > > QOM devices get reset when their bus gets reset, right? > (so everything on a bus gets reset eventually as part of > the process that starts when the top level sysbus gets reset). We avoided that by not using DeviceClass::reset but CPUClass::reset. It's a question of assuring appropriate reset ordering between CPU and devices. PowerPC needed a special reset order via hook in (what is now) MachineClass. So while I agree that CPU reset registration is not ideal and needs changing, I am not convinced that we can generally make the change and hope for the best. I wouldn't mind an incremental transition though, with arm taking the first step - still leaves the question of exact direction. If you look at x86, you will find that despite my protest against this inconsistency, the reset hook registration was moved into CPU code but none of the other targets changed alongside. Andreas
On 17 September 2014 09:40, Andreas Färber <afaerber@suse.de> wrote: > We avoided that by not using DeviceClass::reset but CPUClass::reset. > It's a question of assuring appropriate reset ordering between CPU and > devices. PowerPC needed a special reset order via hook in (what is now) > MachineClass. > So while I agree that CPU reset registration is not ideal and needs > changing, I am not convinced that we can generally make the change and > hope for the best. I wouldn't mind an incremental transition though, > with arm taking the first step - still leaves the question of exact > direction. If you look at x86, you will find that despite my protest > against this inconsistency, the reset hook registration was moved into > CPU code but none of the other targets changed alongside. I don't object to taking a pragmatic approach in the ARM code (eg this patch). I just wanted to know if you had a preferred direction we should be taking instead (which as you say we kind of have to do in an incremental way). It sounds like you don't have anything concrete in mind so maybe we should just apply this patch. In general I suspect there are a lot of unresolved issues in our handling of reset -- it's a complicated area which we attempt to address in an over-simplistic way at the moment :-( But there's a pile of other problems on the ARM QEMU todo list so as long as a change like this isn't actively working against a transition you're working towards then it will solve the immediate bug. thanks -- PMM
Am 17.09.2014 um 18:47 schrieb Peter Maydell: > On 17 September 2014 09:40, Andreas Färber <afaerber@suse.de> wrote: >> We avoided that by not using DeviceClass::reset but CPUClass::reset. >> It's a question of assuring appropriate reset ordering between CPU and >> devices. PowerPC needed a special reset order via hook in (what is now) >> MachineClass. > >> So while I agree that CPU reset registration is not ideal and needs >> changing, I am not convinced that we can generally make the change and >> hope for the best. I wouldn't mind an incremental transition though, >> with arm taking the first step - still leaves the question of exact >> direction. If you look at x86, you will find that despite my protest >> against this inconsistency, the reset hook registration was moved into >> CPU code but none of the other targets changed alongside. > > I don't object to taking a pragmatic approach in the ARM code > (eg this patch). I just wanted to know if you had a preferred > direction we should be taking instead (which as you say we > kind of have to do in an incremental way). It sounds like you > don't have anything concrete in mind so maybe we should just > apply this patch. Ack. One other concern I have with this patch is the loop assuming that all following CPUs will be of type ARMCPU, but I suspect there will be other code making the same assumption - in that case Reviewed-by. > In general I suspect there are a lot of unresolved issues in > our handling of reset -- it's a complicated area which we > attempt to address in an over-simplistic way at the moment :-( Yes, having test cases for all machines would help refactor these things... Andreas
diff --git a/hw/arm/boot.c b/hw/arm/boot.c index 8f5649a250fd..0cfc11d42962 100644 --- a/hw/arm/boot.c +++ b/hw/arm/boot.c @@ -473,6 +473,10 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) } } + for (; cs; cs = CPU_NEXT(cs)) { + qemu_register_reset(do_cpu_reset, ARM_CPU(cs)); + } + /* If no kernel specified, do nothing; we will start from address 0 * (typically a boot ROM image) in the same way as hardware. */
When booting with -bios or -pflash rather than -kernel, we need to make sure reset handlers are registered. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- hw/arm/boot.c | 4 ++++ 1 file changed, 4 insertions(+)