1. Patchew
  2. QEMU
  3. [PATCH v3 00/11] hw: Strengthen SysBus & QBus API
  4. View patch

[PATCH v3 11/11] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices

Philippe Mathieu-Daudé posted 11 patches 1 year, 12 months ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, John Snow <jsnow@redhat.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, "Hervé Poussineau" <hpoussin@reactos.org>, Yoshinori Sato <ysato@users.sourceforge.jp>, Magnus Damm <magnus.damm@gmail.com>, Fabien Chouteau <chouteau@adacore.com>, Frederic Konrad <konrad.frederic@yahoo.fr>, Artyom Tarasenko <atar4qemu@gmail.com>
There is a newer version of this series
[PATCH v3 11/11] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices
Posted by Philippe Mathieu-Daudé 1 year, 12 months ago 
Inline cpu_create() in order to call
qdev_init_gpio_in_named_with_opaque()
before the CPU is realized.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sparc64/sparc64.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c
index 72f0849f50..3091cde586 100644
--- a/hw/sparc64/sparc64.c
+++ b/hw/sparc64/sparc64.c
@@ -24,6 +24,7 @@
 
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "cpu.h"
 #include "hw/boards.h"
 #include "hw/sparc/sparc64.h"
@@ -271,9 +272,10 @@ SPARCCPU *sparc64_cpu_devinit(const char *cpu_type, uint64_t prom_addr)
     uint32_t  stick_frequency = 100 * 1000000;
     uint32_t hstick_frequency = 100 * 1000000;
 
-    cpu = SPARC_CPU(cpu_create(cpu_type));
+    cpu = SPARC_CPU(object_new(cpu_type));
     qdev_init_gpio_in_named(DEVICE(cpu), sparc64_cpu_set_ivec_irq,
                             "ivec-irq", IVEC_MAX);
+    qdev_realize(DEVICE(cpu), NULL, &error_fatal);
     env = &cpu->env;
 
     env->tick = cpu_timer_create("tick", cpu, tick_irq,
-- 
2.41.0
Re: [PATCH v3 11/11] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices
Posted by Mark Cave-Ayland 1 year, 12 months ago 
On 08/02/2024 18:12, Philippe Mathieu-Daudé wrote:

> Inline cpu_create() in order to call
> qdev_init_gpio_in_named_with_opaque()
> before the CPU is realized.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/sparc64/sparc64.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c
> index 72f0849f50..3091cde586 100644
> --- a/hw/sparc64/sparc64.c
> +++ b/hw/sparc64/sparc64.c
> @@ -24,6 +24,7 @@
>   
>   
>   #include "qemu/osdep.h"
> +#include "qapi/error.h"
>   #include "cpu.h"
>   #include "hw/boards.h"
>   #include "hw/sparc/sparc64.h"
> @@ -271,9 +272,10 @@ SPARCCPU *sparc64_cpu_devinit(const char *cpu_type, uint64_t prom_addr)
>       uint32_t  stick_frequency = 100 * 1000000;
>       uint32_t hstick_frequency = 100 * 1000000;
>   
> -    cpu = SPARC_CPU(cpu_create(cpu_type));
> +    cpu = SPARC_CPU(object_new(cpu_type));
>       qdev_init_gpio_in_named(DEVICE(cpu), sparc64_cpu_set_ivec_irq,
>                               "ivec-irq", IVEC_MAX);
> +    qdev_realize(DEVICE(cpu), NULL, &error_fatal);
>       env = &cpu->env;
>   
>       env->tick = cpu_timer_create("tick", cpu, tick_irq,

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.
Re: [PATCH v3 11/11] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices
Posted by Peter Maydell 1 year, 12 months ago 
On Thu, 8 Feb 2024 at 18:14, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Inline cpu_create() in order to call
> qdev_init_gpio_in_named_with_opaque()
> before the CPU is realized.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/sparc64/sparc64.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c
> index 72f0849f50..3091cde586 100644
> --- a/hw/sparc64/sparc64.c
> +++ b/hw/sparc64/sparc64.c
> @@ -24,6 +24,7 @@
>
>
>  #include "qemu/osdep.h"
> +#include "qapi/error.h"
>  #include "cpu.h"
>  #include "hw/boards.h"
>  #include "hw/sparc/sparc64.h"
> @@ -271,9 +272,10 @@ SPARCCPU *sparc64_cpu_devinit(const char *cpu_type, uint64_t prom_addr)
>      uint32_t  stick_frequency = 100 * 1000000;
>      uint32_t hstick_frequency = 100 * 1000000;
>
> -    cpu = SPARC_CPU(cpu_create(cpu_type));
> +    cpu = SPARC_CPU(object_new(cpu_type));
>      qdev_init_gpio_in_named(DEVICE(cpu), sparc64_cpu_set_ivec_irq,
>                              "ivec-irq", IVEC_MAX);
> +    qdev_realize(DEVICE(cpu), NULL, &error_fatal);
>      env = &cpu->env;
>
>      env->tick = cpu_timer_create("tick", cpu, tick_irq,
> --
> 2.41.0

For the purposes of letting us enforce the "init GPIOs
before realize, not after" rule,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

but it looks like this code is adding a GPIO to a
device from code that's not actually part of the
implementation of the device. Ideally most of the code in
this file should be rolled into the CPU itself in target/sparc.

thanks
-- PMM
Re: [PATCH v3 11/11] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices
Posted by Mark Cave-Ayland 1 year, 12 months ago 
On 09/02/2024 11:34, Peter Maydell wrote:

> On Thu, 8 Feb 2024 at 18:14, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Inline cpu_create() in order to call
>> qdev_init_gpio_in_named_with_opaque()
>> before the CPU is realized.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/sparc64/sparc64.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c
>> index 72f0849f50..3091cde586 100644
>> --- a/hw/sparc64/sparc64.c
>> +++ b/hw/sparc64/sparc64.c
>> @@ -24,6 +24,7 @@
>>
>>
>>   #include "qemu/osdep.h"
>> +#include "qapi/error.h"
>>   #include "cpu.h"
>>   #include "hw/boards.h"
>>   #include "hw/sparc/sparc64.h"
>> @@ -271,9 +272,10 @@ SPARCCPU *sparc64_cpu_devinit(const char *cpu_type, uint64_t prom_addr)
>>       uint32_t  stick_frequency = 100 * 1000000;
>>       uint32_t hstick_frequency = 100 * 1000000;
>>
>> -    cpu = SPARC_CPU(cpu_create(cpu_type));
>> +    cpu = SPARC_CPU(object_new(cpu_type));
>>       qdev_init_gpio_in_named(DEVICE(cpu), sparc64_cpu_set_ivec_irq,
>>                               "ivec-irq", IVEC_MAX);
>> +    qdev_realize(DEVICE(cpu), NULL, &error_fatal);
>>       env = &cpu->env;
>>
>>       env->tick = cpu_timer_create("tick", cpu, tick_irq,
>> --
>> 2.41.0
> 
> For the purposes of letting us enforce the "init GPIOs
> before realize, not after" rule,
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> but it looks like this code is adding a GPIO to a
> device from code that's not actually part of the
> implementation of the device. Ideally most of the code in
> this file should be rolled into the CPU itself in target/sparc.

I suspect the reason the code is arranged like this is because IVECs aren't part of 
the core SPARC 64-bit architecture specification, although they happen to be 
implemented by the CPUs used by QEMU. Perhaps this would be better be handled on a 
CPU model basis, but I agree this shouldn't be a blocker for this patch.


ATB,

Mark.
Re: [PATCH v3 11/11] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices
Posted by Philippe Mathieu-Daudé 1 year, 11 months ago 
On 9/2/24 23:01, Mark Cave-Ayland wrote:
> On 09/02/2024 11:34, Peter Maydell wrote:
> 
>> On Thu, 8 Feb 2024 at 18:14, Philippe Mathieu-Daudé 
>> <philmd@linaro.org> wrote:
>>>
>>> Inline cpu_create() in order to call
>>> qdev_init_gpio_in_named_with_opaque()
>>> before the CPU is realized.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/sparc64/sparc64.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c
>>> index 72f0849f50..3091cde586 100644
>>> --- a/hw/sparc64/sparc64.c
>>> +++ b/hw/sparc64/sparc64.c
>>> @@ -24,6 +24,7 @@
>>>
>>>
>>>   #include "qemu/osdep.h"
>>> +#include "qapi/error.h"
>>>   #include "cpu.h"
>>>   #include "hw/boards.h"
>>>   #include "hw/sparc/sparc64.h"
>>> @@ -271,9 +272,10 @@ SPARCCPU *sparc64_cpu_devinit(const char 
>>> *cpu_type, uint64_t prom_addr)
>>>       uint32_t  stick_frequency = 100 * 1000000;
>>>       uint32_t hstick_frequency = 100 * 1000000;
>>>
>>> -    cpu = SPARC_CPU(cpu_create(cpu_type));
>>> +    cpu = SPARC_CPU(object_new(cpu_type));
>>>       qdev_init_gpio_in_named(DEVICE(cpu), sparc64_cpu_set_ivec_irq,
>>>                               "ivec-irq", IVEC_MAX);
>>> +    qdev_realize(DEVICE(cpu), NULL, &error_fatal);
>>>       env = &cpu->env;
>>>
>>>       env->tick = cpu_timer_create("tick", cpu, tick_irq,
>>> -- 
>>> 2.41.0
>>
>> For the purposes of letting us enforce the "init GPIOs
>> before realize, not after" rule,
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> but it looks like this code is adding a GPIO to a
>> device from code that's not actually part of the
>> implementation of the device. Ideally most of the code in
>> this file should be rolled into the CPU itself in target/sparc.
> 
> I suspect the reason the code is arranged like this is because IVECs 
> aren't part of the core SPARC 64-bit architecture specification, 
> although they happen to be implemented by the CPUs used by QEMU. Perhaps 
> this would be better be handled on a CPU model basis, but I agree this 
> shouldn't be a blocker for this patch.

Suggestion recorded as https://gitlab.com/qemu-project/qemu/-/issues/2163.

Thanks both!

Phil.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.