[v5] Mac Old World ROM experiment

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

Posted by BALATON Zoltan 5 years, 3 months ago

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 */
-- 
2.21.3

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

Posted by Mark Cave-Ayland 5 years, 3 months ago

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.


ATB,

Mark.

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

Posted by BALATON Zoltan 5 years, 3 months ago

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