diff mbox series

[02/23] hw/ppc/e500: Reduce scope of env pointer

Message ID 20240923093016.66437-3-shentey@gmail.com
State New
Headers show
Series E500 Cleanup | expand

Commit Message

Bernhard Beschow Sept. 23, 2024, 9:29 a.m. UTC 
The env pointer isn't used outside the for loop, so move it inside. After that,
the firstenv pointer is never read, so remove it.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ppc/e500.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

Comments

BALATON Zoltan Sept. 23, 2024, 10:04 a.m. UTC | #1 
On Mon, 23 Sep 2024, Bernhard Beschow wrote:
> The env pointer isn't used outside the for loop, so move it inside. After that,
> the firstenv pointer is never read, so remove it.

It's probably the other way arouns, you remove firstenv (which is the 
bigger part of this patch) then it's clear env is not needed outside of 
the loop any more so can be moved there. The purpose of this seems to be 
to preserve the env of the first CPU but as it's unused yet maybe it can 
be removed for now and readded later when needed.

Regards,
BALATON Zoltan

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/ppc/e500.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 75b051009f..f68779a1ea 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -899,7 +899,6 @@ void ppce500_init(MachineState *machine)
>     const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(machine);
>     MachineClass *mc = MACHINE_CLASS(pmc);
>     PCIBus *pci_bus;
> -    CPUPPCState *env = NULL;
>     uint64_t loadaddr;
>     hwaddr kernel_base = -1LL;
>     int kernel_size = 0;
> @@ -921,7 +920,6 @@ void ppce500_init(MachineState *machine)
>     IrqLines *irqs;
>     DeviceState *dev, *mpicdev;
>     DriveInfo *dinfo;
> -    CPUPPCState *firstenv = NULL;
>     MemoryRegion *ccsr_addr_space;
>     SysBusDevice *s;
>     PPCE500CCSRState *ccsr;
> @@ -930,6 +928,7 @@ void ppce500_init(MachineState *machine)
>     irqs = g_new0(IrqLines, smp_cpus);
>     for (i = 0; i < smp_cpus; i++) {
>         PowerPCCPU *cpu;
> +        CPUPPCState *env;
>         CPUState *cs;
>
>         cpu = POWERPC_CPU(object_new(machine->cpu_type));
> @@ -950,10 +949,6 @@ void ppce500_init(MachineState *machine)
>                                  &error_abort);
>         qdev_realize_and_unref(DEVICE(cs), NULL, &error_fatal);
>
> -        if (!firstenv) {
> -            firstenv = env;
> -        }
> -
>         irqs[i].irq[OPENPIC_OUTPUT_INT] =
>             qdev_get_gpio_in(DEVICE(cpu), PPCE500_INPUT_INT);
>         irqs[i].irq[OPENPIC_OUTPUT_CINT] =
> @@ -974,8 +969,6 @@ void ppce500_init(MachineState *machine)
>         }
>     }
>
> -    env = firstenv;
> -
>     if (!QEMU_IS_ALIGNED(machine->ram_size, RAM_SIZES_ALIGN)) {
>         error_report("RAM size must be multiple of %" PRIu64, RAM_SIZES_ALIGN);
>         exit(EXIT_FAILURE);
>
Cédric Le Goater Sept. 25, 2024, 3:37 p.m. UTC | #2 
On 9/23/24 11:29, Bernhard Beschow wrote:
> The env pointer isn't used outside the for loop, so move it inside. After that,
> the firstenv pointer is never read, so remove it.

Just wondering, have you considered introducing an PowerPCCPU array
under the machine state ?

This would be an intermediate step towards the introduction of an SoC
model (in the long term)

Thanks,

C.



> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/ppc/e500.c | 9 +--------
>   1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 75b051009f..f68779a1ea 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -899,7 +899,6 @@ void ppce500_init(MachineState *machine)
>       const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(machine);
>       MachineClass *mc = MACHINE_CLASS(pmc);
>       PCIBus *pci_bus;
> -    CPUPPCState *env = NULL;
>       uint64_t loadaddr;
>       hwaddr kernel_base = -1LL;
>       int kernel_size = 0;
> @@ -921,7 +920,6 @@ void ppce500_init(MachineState *machine)
>       IrqLines *irqs;
>       DeviceState *dev, *mpicdev;
>       DriveInfo *dinfo;
> -    CPUPPCState *firstenv = NULL;
>       MemoryRegion *ccsr_addr_space;
>       SysBusDevice *s;
>       PPCE500CCSRState *ccsr;
> @@ -930,6 +928,7 @@ void ppce500_init(MachineState *machine)
>       irqs = g_new0(IrqLines, smp_cpus);
>       for (i = 0; i < smp_cpus; i++) {
>           PowerPCCPU *cpu;
> +        CPUPPCState *env;
>           CPUState *cs;
>   
>           cpu = POWERPC_CPU(object_new(machine->cpu_type));
> @@ -950,10 +949,6 @@ void ppce500_init(MachineState *machine)
>                                    &error_abort);
>           qdev_realize_and_unref(DEVICE(cs), NULL, &error_fatal);
>   
> -        if (!firstenv) {
> -            firstenv = env;
> -        }
> -
>           irqs[i].irq[OPENPIC_OUTPUT_INT] =
>               qdev_get_gpio_in(DEVICE(cpu), PPCE500_INPUT_INT);
>           irqs[i].irq[OPENPIC_OUTPUT_CINT] =
> @@ -974,8 +969,6 @@ void ppce500_init(MachineState *machine)
>           }
>       }
>   
> -    env = firstenv;
> -
>       if (!QEMU_IS_ALIGNED(machine->ram_size, RAM_SIZES_ALIGN)) {
>           error_report("RAM size must be multiple of %" PRIu64, RAM_SIZES_ALIGN);
>           exit(EXIT_FAILURE);
Bernhard Beschow Sept. 25, 2024, 7:02 p.m. UTC | #3 
Am 25. September 2024 15:37:22 UTC schrieb "Cédric Le Goater" <clg@redhat.com>:
>On 9/23/24 11:29, Bernhard Beschow wrote:
>> The env pointer isn't used outside the for loop, so move it inside. After that,
>> the firstenv pointer is never read, so remove it.
>
>Just wondering, have you considered introducing an PowerPCCPU array
>under the machine state ?
>
>This would be an intermediate step towards the introduction of an SoC
>model (in the long term)

Well, there seem to be many members in the QorIQ family with incompatible offsets. So I experimented with dtb-driven machine creation instead to sidestep the whole problem. Once this series is merged I plan to submit an RFC for that.

Best regards,
Bernhard

>
>Thanks,
>
>C.
>
>
>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/ppc/e500.c | 9 +--------
>>   1 file changed, 1 insertion(+), 8 deletions(-)
>> 
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 75b051009f..f68779a1ea 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -899,7 +899,6 @@ void ppce500_init(MachineState *machine)
>>       const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(machine);
>>       MachineClass *mc = MACHINE_CLASS(pmc);
>>       PCIBus *pci_bus;
>> -    CPUPPCState *env = NULL;
>>       uint64_t loadaddr;
>>       hwaddr kernel_base = -1LL;
>>       int kernel_size = 0;
>> @@ -921,7 +920,6 @@ void ppce500_init(MachineState *machine)
>>       IrqLines *irqs;
>>       DeviceState *dev, *mpicdev;
>>       DriveInfo *dinfo;
>> -    CPUPPCState *firstenv = NULL;
>>       MemoryRegion *ccsr_addr_space;
>>       SysBusDevice *s;
>>       PPCE500CCSRState *ccsr;
>> @@ -930,6 +928,7 @@ void ppce500_init(MachineState *machine)
>>       irqs = g_new0(IrqLines, smp_cpus);
>>       for (i = 0; i < smp_cpus; i++) {
>>           PowerPCCPU *cpu;
>> +        CPUPPCState *env;
>>           CPUState *cs;
>>             cpu = POWERPC_CPU(object_new(machine->cpu_type));
>> @@ -950,10 +949,6 @@ void ppce500_init(MachineState *machine)
>>                                    &error_abort);
>>           qdev_realize_and_unref(DEVICE(cs), NULL, &error_fatal);
>>   -        if (!firstenv) {
>> -            firstenv = env;
>> -        }
>> -
>>           irqs[i].irq[OPENPIC_OUTPUT_INT] =
>>               qdev_get_gpio_in(DEVICE(cpu), PPCE500_INPUT_INT);
>>           irqs[i].irq[OPENPIC_OUTPUT_CINT] =
>> @@ -974,8 +969,6 @@ void ppce500_init(MachineState *machine)
>>           }
>>       }
>>   -    env = firstenv;
>> -
>>       if (!QEMU_IS_ALIGNED(machine->ram_size, RAM_SIZES_ALIGN)) {
>>           error_report("RAM size must be multiple of %" PRIu64, RAM_SIZES_ALIGN);
>>           exit(EXIT_FAILURE);
>
Bernhard Beschow Sept. 25, 2024, 7:09 p.m. UTC | #4 
Am 23. September 2024 10:04:48 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Mon, 23 Sep 2024, Bernhard Beschow wrote:
>> The env pointer isn't used outside the for loop, so move it inside. After that,
>> the firstenv pointer is never read, so remove it.
>
>It's probably the other way arouns, you remove firstenv (which is the bigger part of this patch) then it's clear env is not needed outside of the loop any more so can be moved there. The purpose of this seems to be to preserve the env of the first CPU but as it's unused yet maybe it can be removed for now and readded later when needed.

I'll fix the commit message.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/ppc/e500.c | 9 +--------
>> 1 file changed, 1 insertion(+), 8 deletions(-)
>> 
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 75b051009f..f68779a1ea 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -899,7 +899,6 @@ void ppce500_init(MachineState *machine)
>>     const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(machine);
>>     MachineClass *mc = MACHINE_CLASS(pmc);
>>     PCIBus *pci_bus;
>> -    CPUPPCState *env = NULL;
>>     uint64_t loadaddr;
>>     hwaddr kernel_base = -1LL;
>>     int kernel_size = 0;
>> @@ -921,7 +920,6 @@ void ppce500_init(MachineState *machine)
>>     IrqLines *irqs;
>>     DeviceState *dev, *mpicdev;
>>     DriveInfo *dinfo;
>> -    CPUPPCState *firstenv = NULL;
>>     MemoryRegion *ccsr_addr_space;
>>     SysBusDevice *s;
>>     PPCE500CCSRState *ccsr;
>> @@ -930,6 +928,7 @@ void ppce500_init(MachineState *machine)
>>     irqs = g_new0(IrqLines, smp_cpus);
>>     for (i = 0; i < smp_cpus; i++) {
>>         PowerPCCPU *cpu;
>> +        CPUPPCState *env;
>>         CPUState *cs;
>> 
>>         cpu = POWERPC_CPU(object_new(machine->cpu_type));
>> @@ -950,10 +949,6 @@ void ppce500_init(MachineState *machine)
>>                                  &error_abort);
>>         qdev_realize_and_unref(DEVICE(cs), NULL, &error_fatal);
>> 
>> -        if (!firstenv) {
>> -            firstenv = env;
>> -        }
>> -
>>         irqs[i].irq[OPENPIC_OUTPUT_INT] =
>>             qdev_get_gpio_in(DEVICE(cpu), PPCE500_INPUT_INT);
>>         irqs[i].irq[OPENPIC_OUTPUT_CINT] =
>> @@ -974,8 +969,6 @@ void ppce500_init(MachineState *machine)
>>         }
>>     }
>> 
>> -    env = firstenv;
>> -
>>     if (!QEMU_IS_ALIGNED(machine->ram_size, RAM_SIZES_ALIGN)) {
>>         error_report("RAM size must be multiple of %" PRIu64, RAM_SIZES_ALIGN);
>>         exit(EXIT_FAILURE);
>>
diff mbox series

Patch

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 75b051009f..f68779a1ea 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -899,7 +899,6 @@  void ppce500_init(MachineState *machine)
     const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(machine);
     MachineClass *mc = MACHINE_CLASS(pmc);
     PCIBus *pci_bus;
-    CPUPPCState *env = NULL;
     uint64_t loadaddr;
     hwaddr kernel_base = -1LL;
     int kernel_size = 0;
@@ -921,7 +920,6 @@  void ppce500_init(MachineState *machine)
     IrqLines *irqs;
     DeviceState *dev, *mpicdev;
     DriveInfo *dinfo;
-    CPUPPCState *firstenv = NULL;
     MemoryRegion *ccsr_addr_space;
     SysBusDevice *s;
     PPCE500CCSRState *ccsr;
@@ -930,6 +928,7 @@  void ppce500_init(MachineState *machine)
     irqs = g_new0(IrqLines, smp_cpus);
     for (i = 0; i < smp_cpus; i++) {
         PowerPCCPU *cpu;
+        CPUPPCState *env;
         CPUState *cs;
 
         cpu = POWERPC_CPU(object_new(machine->cpu_type));
@@ -950,10 +949,6 @@  void ppce500_init(MachineState *machine)
                                  &error_abort);
         qdev_realize_and_unref(DEVICE(cs), NULL, &error_fatal);
 
-        if (!firstenv) {
-            firstenv = env;
-        }
-
         irqs[i].irq[OPENPIC_OUTPUT_INT] =
             qdev_get_gpio_in(DEVICE(cpu), PPCE500_INPUT_INT);
         irqs[i].irq[OPENPIC_OUTPUT_CINT] =
@@ -974,8 +969,6 @@  void ppce500_init(MachineState *machine)
         }
     }
 
-    env = firstenv;
-
     if (!QEMU_IS_ALIGNED(machine->ram_size, RAM_SIZES_ALIGN)) {
         error_report("RAM size must be multiple of %" PRIu64, RAM_SIZES_ALIGN);
         exit(EXIT_FAILURE);