Re: [PATCH v5 06/11] mac_oldworld: Rename ppc_heathrow_reset to ppc_heathrow_cpu_reset

[BALATON Zoltan]

On Fri, 26 Jun 2020, Mark Cave-Ayland wrote:
> On 16/06/2020 14:47, BALATON Zoltan wrote:
>
> > This function resets a CPU not the whole machine so reflect that in
> > its name.
> >
> > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >  hw/ppc/mac_oldworld.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> > index 4200008851..f97f241e0c 100644
> > --- a/hw/ppc/mac_oldworld.c
> > +++ b/hw/ppc/mac_oldworld.c
> > @@ -73,7 +73,7 @@ static uint64_t translate_kernel_address(void *opaque, 
> > uint64_t addr)
> >      return (addr & 0x0fffffff) + KERNEL_LOAD_ADDR;
> >  }
> >
> > -static void ppc_heathrow_reset(void *opaque)
> > +static void ppc_heathrow_cpu_reset(void *opaque)
> >  {
> >      PowerPCCPU *cpu = opaque;
> >
> > @@ -112,7 +112,7 @@ static void ppc_heathrow_init(MachineState *machine)
> >
> >          /* Set time-base frequency to 16.6 Mhz */
> >          cpu_ppc_tb_init(env,  TBFREQ);
> > -        qemu_register_reset(ppc_heathrow_reset, cpu);
> > +        qemu_register_reset(ppc_heathrow_cpu_reset, cpu);
> >      }
> >
> >      /* allocate RAM */
>
> As per my previous comment on your earlier version, I don't agree with this - 
> the
> reset is being registered at board level, it just so happens that as it's only
> touching the CPU due to the opaque being passed in.
>
> I'd be inclined to pass in a suitable HeathrowMachineState object containing a
> reference to the CPU instead.

It's not a board level reset func but a CPU level one. See where it's 
registered here:

https://git.qemu.org/?p=qemu.git;a=blob;f=hw/ppc/mac_oldworld.c;h=f8c204ead73843098084bf5213ac4046d7d843c4;hb=HEAD#l111

One for each CPU and as there could be more than one CPU, this won't work 
with a single reference in HeathrowMachineState. We could reset CPUs in a 
board level reset func (added by next patch) but I don't know how to 
access CPU objects from MachineState (it did not look trivial when I've 
looked) so I just left it as it is for later clean up separate from this 
series. I've just renamed it to avoid confusion with board level reset 
func which is usually named *_reset but I could call that 
ppc_heathrow_board_reset and then we don't need this patch but I think 
this is cleaner.

I don't even know why we need a reset func to reset the CPU, I'd expect 
that to be the default behaviour of any board reset without needing to 
register a func to do it.

Regards,
BALATON Zoltan