1. Patchew
  2. QEMU
  3. [PATCH 00/23] E500 Cleanup
  4. View patch

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

Bernhard Beschow posted 23 patches 4 weeks, 1 day ago
There is a newer version of this series
[PATCH 02/23] hw/ppc/e500: Reduce scope of env pointer
Posted by Bernhard Beschow 4 weeks, 1 day ago 
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(-)

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);
-- 
2.46.1
Re: [PATCH 02/23] hw/ppc/e500: Reduce scope of env pointer
Posted by Cédric Le Goater 3 weeks, 6 days ago 
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);
Re: [PATCH 02/23] hw/ppc/e500: Reduce scope of env pointer
Posted by Bernhard Beschow 3 weeks, 6 days ago 

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);
>
Re: [PATCH 02/23] hw/ppc/e500: Reduce scope of env pointer
Posted by BALATON Zoltan 4 weeks, 1 day ago 
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);
>
Re: [PATCH 02/23] hw/ppc/e500: Reduce scope of env pointer
Posted by Bernhard Beschow 3 weeks, 6 days ago 

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);
>>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.