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

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- Attachment #1: Type: text/plain, Size: 2517 bytes --]

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