[Qemu-devel] [RFC] ppc: qdev-ify CPU creation

[Qemu-devel] [RFC] ppc: qdev-ify CPU creation

[Andreas Färber]

From: Hervé Poussineau <hpoussin@reactos.org>

v1:
* Coding style fixes.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 
 Hello Alex,
 
 Seeing the discussions about Leon3, is this the way to go for ppc? Is ppc.[hc] right?
 
 The unconditional use of 6xx looks suspicious to me, no?
 Should we rename cpu_device_irq_request() to cpu_device_irq_request_6xx()?
 
 Regards,
 Andreas
 
 hw/ppc.c            |   75 +++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/ppc.h            |    2 +
 target-ppc/cpu.h    |    1 +
 target-ppc/helper.c |   21 +++++++++++---
 4 files changed, 94 insertions(+), 5 deletions(-)

diff --git a/hw/ppc.c b/hw/ppc.c
index 968aec1..0927326 100644
--- a/hw/ppc.c
+++ b/hw/ppc.c
@@ -30,6 +30,8 @@
 #include "loader.h"
 #include "kvm.h"
 #include "kvm_ppc.h"
+#include "hw/qdev.h"
+#include "hw/sysbus.h"
 
 //#define PPC_DEBUG_IRQ
 //#define PPC_DEBUG_TB
@@ -1286,3 +1288,76 @@ int PPC_NVRAM_set_params (nvram_t *nvram, uint16_t NVRAM_size,
 
     return 0;
 }
+
+DeviceState *cpu_ppc_create_simple(const char *cpu_model)
+{
+    DeviceState *dev;
+
+    dev = qdev_create(NULL, "cpu-ppc");
+    if (!dev) {
+        return NULL;
+    }
+    qdev_prop_set_string(dev, "model", qemu_strdup(cpu_model));
+    if (qdev_init(dev) < 0) {
+        return NULL;
+    }
+    return dev;
+}
+
+typedef struct CPUPPC {
+    SysBusDevice busdev;
+    char *model;
+    CPUPPCState state;
+} CPUPPC;
+
+static void cpu_device_irq_request(void *opaque, int pin, int level)
+{
+    CPUPPC* cpu = opaque;
+    CPUPPCState* env = &cpu->state;
+    ppc6xx_set_irq(env, pin, level);
+}
+
+static int cpu_device_init(SysBusDevice *dev)
+{
+    CPUPPC* cpu = FROM_SYSBUS(CPUPPC, dev);
+    CPUPPCState* env = &cpu->state;
+
+    if (cpu_ppc_init_inplace(env, cpu->model) < 0) {
+        return -1;
+    }
+
+    if (env->flags & POWERPC_FLAG_RTC_CLK) {
+        /* POWER / PowerPC 601 RTC clock frequency is 7.8125 MHz */
+        cpu_ppc_tb_init(env, 7812500UL);
+    } else {
+        /* Set time-base frequency to 100 Mhz */
+        cpu_ppc_tb_init(env, 100UL * 1000UL * 1000UL);
+    }
+
+    qdev_init_gpio_in(&dev->qdev, cpu_device_irq_request, PPC6xx_INPUT_NB);
+    return 0;
+}
+
+static void cpu_device_reset(DeviceState *d)
+{
+    CPUPPC *s = FROM_SYSBUS(CPUPPC, sysbus_from_qdev(d));
+    cpu_reset(&s->state);
+}
+
+static SysBusDeviceInfo cpu_device_info = {
+    .qdev.name = "cpu-ppc",
+    .qdev.size = sizeof(CPUPPC),
+    .qdev.reset = cpu_device_reset,
+    .init = cpu_device_init,
+    .qdev.props = (Property[]) {
+        DEFINE_PROP_STRING("model", CPUPPC, model),
+        DEFINE_PROP_END_OF_LIST(),
+    },
+};
+
+static void ppc_register_devices(void)
+{
+    sysbus_register_withprop(&cpu_device_info);
+}
+
+device_init(ppc_register_devices)
diff --git a/hw/ppc.h b/hw/ppc.h
index 34f54cf..ae8dd97 100644
--- a/hw/ppc.h
+++ b/hw/ppc.h
@@ -37,6 +37,8 @@ void ppce500_irq_init (CPUState *env);
 void ppc6xx_irq_init (CPUState *env);
 void ppc970_irq_init (CPUState *env);
 
+DeviceState *cpu_ppc_create_simple(const char *cpu_model);
+
 /* PPC machines for OpenBIOS */
 enum {
     ARCH_PREP = 0,
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index deb8d7c..0f56d45 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -721,6 +721,7 @@ struct mmu_ctx_t {
 
 /*****************************************************************************/
 CPUPPCState *cpu_ppc_init (const char *cpu_model);
+int cpu_ppc_init_inplace(CPUPPCState *env, const char *cpu_model);
 void ppc_translate_init(void);
 int cpu_ppc_exec (CPUPPCState *s);
 void cpu_ppc_close (CPUPPCState *s);
diff --git a/target-ppc/helper.c b/target-ppc/helper.c
index 4b49101..99af1f6 100644
--- a/target-ppc/helper.c
+++ b/target-ppc/helper.c
@@ -2794,22 +2794,33 @@ void cpu_reset(CPUPPCState *env)
     tlb_flush(env, 1);
 }
 
-CPUPPCState *cpu_ppc_init (const char *cpu_model)
+int cpu_ppc_init_inplace(CPUPPCState *env, const char *cpu_model)
 {
-    CPUPPCState *env;
     const ppc_def_t *def;
 
     def = cpu_ppc_find_by_name(cpu_model);
-    if (!def)
-        return NULL;
+    if (!def) {
+        return -1;
+    }
 
-    env = qemu_mallocz(sizeof(CPUPPCState));
     cpu_exec_init(env);
     ppc_translate_init();
     env->cpu_model_str = cpu_model;
     cpu_ppc_register_internal(env, def);
 
     qemu_init_vcpu(env);
+    return 0;
+}
+
+CPUPPCState *cpu_ppc_init(const char *cpu_model)
+{
+    CPUPPCState *env;
+
+    env = qemu_mallocz(sizeof(CPUPPCState));
+    if (cpu_ppc_init_inplace(env, cpu_model) < 0) {
+        qemu_free(env);
+        return NULL;
+    }
 
     return env;
 }
-- 
1.7.3.4

Re: [Qemu-devel] [RFC] ppc: qdev-ify CPU creation

[Andreas Färber]

Am 21.12.2010 um 21:01 schrieb Andreas Färber:

> From: Hervé Poussineau <hpoussin@reactos.org>
>
> v1:
> * Coding style fixes.
>
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> ---
>
> Hello Alex,
>
> Seeing the discussions about Leon3, is this the way to go for ppc?  
> Is ppc.[hc] right?
>
> The unconditional use of 6xx looks suspicious to me, no?
> Should we rename cpu_device_irq_request() to  
> cpu_device_irq_request_6xx()?
>
> Regards,
> Andreas

Ping? Any guidance on how to proceed with this?

Andreas

> hw/ppc.c            |   75 ++++++++++++++++++++++++++++++++++++++++++ 
> +++++++++
> hw/ppc.h            |    2 +
> target-ppc/cpu.h    |    1 +
> target-ppc/helper.c |   21 +++++++++++---
> 4 files changed, 94 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ppc.c b/hw/ppc.c
> index 968aec1..0927326 100644
> --- a/hw/ppc.c
> +++ b/hw/ppc.c
> @@ -30,6 +30,8 @@
> #include "loader.h"
> #include "kvm.h"
> #include "kvm_ppc.h"
> +#include "hw/qdev.h"
> +#include "hw/sysbus.h"
>
> //#define PPC_DEBUG_IRQ
> //#define PPC_DEBUG_TB
> @@ -1286,3 +1288,76 @@ int PPC_NVRAM_set_params (nvram_t *nvram,  
> uint16_t NVRAM_size,
>
>     return 0;
> }
> +
> +DeviceState *cpu_ppc_create_simple(const char *cpu_model)
> +{
> +    DeviceState *dev;
> +
> +    dev = qdev_create(NULL, "cpu-ppc");
> +    if (!dev) {
> +        return NULL;
> +    }
> +    qdev_prop_set_string(dev, "model", qemu_strdup(cpu_model));
> +    if (qdev_init(dev) < 0) {
> +        return NULL;
> +    }
> +    return dev;
> +}
> +
> +typedef struct CPUPPC {
> +    SysBusDevice busdev;
> +    char *model;
> +    CPUPPCState state;
> +} CPUPPC;
> +
> +static void cpu_device_irq_request(void *opaque, int pin, int level)
> +{
> +    CPUPPC* cpu = opaque;
> +    CPUPPCState* env = &cpu->state;
> +    ppc6xx_set_irq(env, pin, level);
> +}
> +
> +static int cpu_device_init(SysBusDevice *dev)
> +{
> +    CPUPPC* cpu = FROM_SYSBUS(CPUPPC, dev);
> +    CPUPPCState* env = &cpu->state;
> +
> +    if (cpu_ppc_init_inplace(env, cpu->model) < 0) {
> +        return -1;
> +    }
> +
> +    if (env->flags & POWERPC_FLAG_RTC_CLK) {
> +        /* POWER / PowerPC 601 RTC clock frequency is 7.8125 MHz */
> +        cpu_ppc_tb_init(env, 7812500UL);
> +    } else {
> +        /* Set time-base frequency to 100 Mhz */
> +        cpu_ppc_tb_init(env, 100UL * 1000UL * 1000UL);
> +    }
> +
> +    qdev_init_gpio_in(&dev->qdev, cpu_device_irq_request,  
> PPC6xx_INPUT_NB);
> +    return 0;
> +}
> +
> +static void cpu_device_reset(DeviceState *d)
> +{
> +    CPUPPC *s = FROM_SYSBUS(CPUPPC, sysbus_from_qdev(d));
> +    cpu_reset(&s->state);
> +}
> +
> +static SysBusDeviceInfo cpu_device_info = {
> +    .qdev.name = "cpu-ppc",
> +    .qdev.size = sizeof(CPUPPC),
> +    .qdev.reset = cpu_device_reset,
> +    .init = cpu_device_init,
> +    .qdev.props = (Property[]) {
> +        DEFINE_PROP_STRING("model", CPUPPC, model),
> +        DEFINE_PROP_END_OF_LIST(),
> +    },
> +};
> +
> +static void ppc_register_devices(void)
> +{
> +    sysbus_register_withprop(&cpu_device_info);
> +}
> +
> +device_init(ppc_register_devices)
> diff --git a/hw/ppc.h b/hw/ppc.h
> index 34f54cf..ae8dd97 100644
> --- a/hw/ppc.h
> +++ b/hw/ppc.h
> @@ -37,6 +37,8 @@ void ppce500_irq_init (CPUState *env);
> void ppc6xx_irq_init (CPUState *env);
> void ppc970_irq_init (CPUState *env);
>
> +DeviceState *cpu_ppc_create_simple(const char *cpu_model);
> +
> /* PPC machines for OpenBIOS */
> enum {
>     ARCH_PREP = 0,
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index deb8d7c..0f56d45 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -721,6 +721,7 @@ struct mmu_ctx_t {
>
> / 
> *****************************************************************************/
> CPUPPCState *cpu_ppc_init (const char *cpu_model);
> +int cpu_ppc_init_inplace(CPUPPCState *env, const char *cpu_model);
> void ppc_translate_init(void);
> int cpu_ppc_exec (CPUPPCState *s);
> void cpu_ppc_close (CPUPPCState *s);
> diff --git a/target-ppc/helper.c b/target-ppc/helper.c
> index 4b49101..99af1f6 100644
> --- a/target-ppc/helper.c
> +++ b/target-ppc/helper.c
> @@ -2794,22 +2794,33 @@ void cpu_reset(CPUPPCState *env)
>     tlb_flush(env, 1);
> }
>
> -CPUPPCState *cpu_ppc_init (const char *cpu_model)
> +int cpu_ppc_init_inplace(CPUPPCState *env, const char *cpu_model)
> {
> -    CPUPPCState *env;
>     const ppc_def_t *def;
>
>     def = cpu_ppc_find_by_name(cpu_model);
> -    if (!def)
> -        return NULL;
> +    if (!def) {
> +        return -1;
> +    }
>
> -    env = qemu_mallocz(sizeof(CPUPPCState));
>     cpu_exec_init(env);
>     ppc_translate_init();
>     env->cpu_model_str = cpu_model;
>     cpu_ppc_register_internal(env, def);
>
>     qemu_init_vcpu(env);
> +    return 0;
> +}
> +
> +CPUPPCState *cpu_ppc_init(const char *cpu_model)
> +{
> +    CPUPPCState *env;
> +
> +    env = qemu_mallocz(sizeof(CPUPPCState));
> +    if (cpu_ppc_init_inplace(env, cpu_model) < 0) {
> +        qemu_free(env);
> +        return NULL;
> +    }
>
>     return env;
> }
> -- 
> 1.7.3.4

Re: [Qemu-devel] [RFC] ppc: qdev-ify CPU creation

[Blue Swirl]

On Thu, Jun 9, 2011 at 12:13 AM, Andreas Färber <andreas.faerber@web.de> wrote:
> Am 21.12.2010 um 21:01 schrieb Andreas Färber:
>
>> From: Hervé Poussineau <hpoussin@reactos.org>
>>
>> v1:
>> * Coding style fixes.
>>
>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>> Cc: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>> ---
>>
>> Hello Alex,
>>
>> Seeing the discussions about Leon3, is this the way to go for ppc? Is
>> ppc.[hc] right?
>>
>> The unconditional use of 6xx looks suspicious to me, no?
>> Should we rename cpu_device_irq_request() to cpu_device_irq_request_6xx()?
>>
>> Regards,
>> Andreas
>
> Ping? Any guidance on how to proceed with this?

The patch looks OK, though the qdev method is not used.

Ideally after the patch, the devices and even the board level
shouldn't use CPUState but DeviceState.

Re: [Qemu-devel] [RFC] ppc: qdev-ify CPU creation

[Alexander Graf]

On 21.12.2010, at 21:01, Andreas Färber wrote:

> From: Hervé Poussineau <hpoussin@reactos.org>
> 
> v1:
> * Coding style fixes.
> 
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> ---
> 
> Hello Alex,
> 
> Seeing the discussions about Leon3, is this the way to go for ppc? Is ppc.[hc] right?
> 
> The unconditional use of 6xx looks suspicious to me, no?
> Should we rename cpu_device_irq_request() to cpu_device_irq_request_6xx()?
> 
> Regards,
> Andreas
> 
> hw/ppc.c            |   75 +++++++++++++++++++++++++++++++++++++++++++++++++++
> hw/ppc.h            |    2 +
> target-ppc/cpu.h    |    1 +
> target-ppc/helper.c |   21 +++++++++++---
> 4 files changed, 94 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc.c b/hw/ppc.c
> index 968aec1..0927326 100644
> --- a/hw/ppc.c
> +++ b/hw/ppc.c
> @@ -30,6 +30,8 @@
> #include "loader.h"
> #include "kvm.h"
> #include "kvm_ppc.h"
> +#include "hw/qdev.h"
> +#include "hw/sysbus.h"
> 
> //#define PPC_DEBUG_IRQ
> //#define PPC_DEBUG_TB
> @@ -1286,3 +1288,76 @@ int PPC_NVRAM_set_params (nvram_t *nvram, uint16_t NVRAM_size,
> 
>     return 0;
> }
> +
> +DeviceState *cpu_ppc_create_simple(const char *cpu_model)
> +{
> +    DeviceState *dev;
> +
> +    dev = qdev_create(NULL, "cpu-ppc");
> +    if (!dev) {
> +        return NULL;
> +    }
> +    qdev_prop_set_string(dev, "model", qemu_strdup(cpu_model));
> +    if (qdev_init(dev) < 0) {
> +        return NULL;
> +    }
> +    return dev;
> +}
> +
> +typedef struct CPUPPC {
> +    SysBusDevice busdev;

I'm not sure we really want CPUs on the sysbus. They belong to their own CPU bus. Basically, I think we should try to model our bus topology so that it reflects the bus topology in the device tree 1:1. Then generating a device tree from the bug information and some device specific callbacks would be possible.

> +    char *model;
> +    CPUPPCState state;
> +} CPUPPC;
> +
> +static void cpu_device_irq_request(void *opaque, int pin, int level)
> +{
> +    CPUPPC* cpu = opaque;
> +    CPUPPCState* env = &cpu->state;
> +    ppc6xx_set_irq(env, pin, level);
> +}
> +
> +static int cpu_device_init(SysBusDevice *dev)
> +{
> +    CPUPPC* cpu = FROM_SYSBUS(CPUPPC, dev);
> +    CPUPPCState* env = &cpu->state;
> +
> +    if (cpu_ppc_init_inplace(env, cpu->model) < 0) {
> +        return -1;
> +    }
> +
> +    if (env->flags & POWERPC_FLAG_RTC_CLK) {

Where does this flag suddenly come from? Is this related to qdev'ification?

> +        /* POWER / PowerPC 601 RTC clock frequency is 7.8125 MHz */
> +        cpu_ppc_tb_init(env, 7812500UL);
> +    } else {
> +        /* Set time-base frequency to 100 Mhz */
> +        cpu_ppc_tb_init(env, 100UL * 1000UL * 1000UL);

Usually we have a TB frequency of 400Mhz in our board/devtrees hardcoded in the TCG case. How about a qdev property that the creator could just modify to its needs? We won't need the special 601 flag then either - just move that into the PREP code.

> +    }
> +
> +    qdev_init_gpio_in(&dev->qdev, cpu_device_irq_request, PPC6xx_INPUT_NB);
> +    return 0;
> +}
> +
> +static void cpu_device_reset(DeviceState *d)
> +{
> +    CPUPPC *s = FROM_SYSBUS(CPUPPC, sysbus_from_qdev(d));
> +    cpu_reset(&s->state);
> +}
> +
> +static SysBusDeviceInfo cpu_device_info = {
> +    .qdev.name = "cpu-ppc",
> +    .qdev.size = sizeof(CPUPPC),
> +    .qdev.reset = cpu_device_reset,
> +    .init = cpu_device_init,
> +    .qdev.props = (Property[]) {
> +        DEFINE_PROP_STRING("model", CPUPPC, model),
> +        DEFINE_PROP_END_OF_LIST(),
> +    },
> +};
> +
> +static void ppc_register_devices(void)
> +{
> +    sysbus_register_withprop(&cpu_device_info);
> +}
> +
> +device_init(ppc_register_devices)
> diff --git a/hw/ppc.h b/hw/ppc.h
> index 34f54cf..ae8dd97 100644
> --- a/hw/ppc.h
> +++ b/hw/ppc.h
> @@ -37,6 +37,8 @@ void ppce500_irq_init (CPUState *env);
> void ppc6xx_irq_init (CPUState *env);
> void ppc970_irq_init (CPUState *env);
> 
> +DeviceState *cpu_ppc_create_simple(const char *cpu_model);
> +
> /* PPC machines for OpenBIOS */
> enum {
>     ARCH_PREP = 0,
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index deb8d7c..0f56d45 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -721,6 +721,7 @@ struct mmu_ctx_t {
> 
> /*****************************************************************************/
> CPUPPCState *cpu_ppc_init (const char *cpu_model);
> +int cpu_ppc_init_inplace(CPUPPCState *env, const char *cpu_model);
> void ppc_translate_init(void);
> int cpu_ppc_exec (CPUPPCState *s);
> void cpu_ppc_close (CPUPPCState *s);
> diff --git a/target-ppc/helper.c b/target-ppc/helper.c
> index 4b49101..99af1f6 100644
> --- a/target-ppc/helper.c
> +++ b/target-ppc/helper.c
> @@ -2794,22 +2794,33 @@ void cpu_reset(CPUPPCState *env)
>     tlb_flush(env, 1);
> }
> 
> -CPUPPCState *cpu_ppc_init (const char *cpu_model)
> +int cpu_ppc_init_inplace(CPUPPCState *env, const char *cpu_model)
> {
> -    CPUPPCState *env;
>     const ppc_def_t *def;
> 
>     def = cpu_ppc_find_by_name(cpu_model);
> -    if (!def)
> -        return NULL;
> +    if (!def) {
> +        return -1;
> +    }
> 
> -    env = qemu_mallocz(sizeof(CPUPPCState));
>     cpu_exec_init(env);
>     ppc_translate_init();
>     env->cpu_model_str = cpu_model;
>     cpu_ppc_register_internal(env, def);
> 
>     qemu_init_vcpu(env);
> +    return 0;
> +}
> +
> +CPUPPCState *cpu_ppc_init(const char *cpu_model)
> +{
> +    CPUPPCState *env;
> +
> +    env = qemu_mallocz(sizeof(CPUPPCState));
> +    if (cpu_ppc_init_inplace(env, cpu_model) < 0) {

Why would we need this function again if the CPUs are qdev'ified?


Overall, I really like the idea of moving CPUs to qdev though. Makes things a lot more structured.

Alex

Re: [Qemu-devel] [RFC] ppc: qdev-ify CPU creation

[Hervé Poussineau]

Alexander Graf a écrit :
> On 21.12.2010, at 21:01, Andreas Färber wrote:
>
>   
>> From: Hervé Poussineau <hpoussin@reactos.org>
>>
>> v1:
>> * Coding style fixes.
>>
>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>> Cc: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>> ---
>>
>> Hello Alex,
>>
>> Seeing the discussions about Leon3, is this the way to go for ppc? Is ppc.[hc] right?
>>
>> The unconditional use of 6xx looks suspicious to me, no?
>> Should we rename cpu_device_irq_request() to cpu_device_irq_request_6xx()?
>>
>> Regards,
>> Andreas
>>
>> hw/ppc.c            |   75 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> hw/ppc.h            |    2 +
>> target-ppc/cpu.h    |    1 +
>> target-ppc/helper.c |   21 +++++++++++---
>> 4 files changed, 94 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/ppc.c b/hw/ppc.c
>> index 968aec1..0927326 100644
>> --- a/hw/ppc.c
>> +++ b/hw/ppc.c
>> @@ -30,6 +30,8 @@
>> #include "loader.h"
>> #include "kvm.h"
>> #include "kvm_ppc.h"
>> +#include "hw/qdev.h"
>> +#include "hw/sysbus.h"
>>
>> //#define PPC_DEBUG_IRQ
>> //#define PPC_DEBUG_TB
>> @@ -1286,3 +1288,76 @@ int PPC_NVRAM_set_params (nvram_t *nvram, uint16_t NVRAM_size,
>>
>>     return 0;
>> }
>> +
>> +DeviceState *cpu_ppc_create_simple(const char *cpu_model)
>> +{
>> +    DeviceState *dev;
>> +
>> +    dev = qdev_create(NULL, "cpu-ppc");
>> +    if (!dev) {
>> +        return NULL;
>> +    }
>> +    qdev_prop_set_string(dev, "model", qemu_strdup(cpu_model));
>> +    if (qdev_init(dev) < 0) {
>> +        return NULL;
>> +    }
>> +    return dev;
>> +}
>> +
>> +typedef struct CPUPPC {
>> +    SysBusDevice busdev;
>>     
>
> I'm not sure we really want CPUs on the sysbus. They belong to their own CPU bus. Basically, I think we should try to model our bus topology so that it reflects the bus topology in the device tree 1:1. Then generating a device tree from the bug information and some device specific callbacks would be possible.
>
>   
CPUs don't need a bus with specific capabilities, so I used the most 
simple existing one, ie SysBus.
>> +    char *model;
>> +    CPUPPCState state;
>> +} CPUPPC;
>> +
>> +static void cpu_device_irq_request(void *opaque, int pin, int level)
>> +{
>> +    CPUPPC* cpu = opaque;
>> +    CPUPPCState* env = &cpu->state;
>> +    ppc6xx_set_irq(env, pin, level);
>> +}
>> +
>> +static int cpu_device_init(SysBusDevice *dev)
>> +{
>> +    CPUPPC* cpu = FROM_SYSBUS(CPUPPC, dev);
>> +    CPUPPCState* env = &cpu->state;
>> +
>> +    if (cpu_ppc_init_inplace(env, cpu->model) < 0) {
>> +        return -1;
>> +    }
>> +
>> +    if (env->flags & POWERPC_FLAG_RTC_CLK) {
>>     
>
> Where does this flag suddenly come from? Is this related to qdev'ification?
>
>   
It's not related to qdev'ification. It is already set in CPU definitions 
since a long time.

>> +        /* POWER / PowerPC 601 RTC clock frequency is 7.8125 MHz */
>> +        cpu_ppc_tb_init(env, 7812500UL);
>> +    } else {
>> +        /* Set time-base frequency to 100 Mhz */
>> +        cpu_ppc_tb_init(env, 100UL * 1000UL * 1000UL);
>>     
>
> Usually we have a TB frequency of 400Mhz in our board/devtrees hardcoded in the TCG case. How about a qdev property that the creator could just modify to its needs? We won't need the special 601 flag then either - just move that into the PREP code.
>
>   
This code has been extracted from ppc_prep.c ; a qdev property is also 
fine.
>> +    }
>> +
>> +    qdev_init_gpio_in(&dev->qdev, cpu_device_irq_request, PPC6xx_INPUT_NB);
>> +    return 0;
>> +}
>> +
>> +static void cpu_device_reset(DeviceState *d)
>> +{
>> +    CPUPPC *s = FROM_SYSBUS(CPUPPC, sysbus_from_qdev(d));
>> +    cpu_reset(&s->state);
>> +}
>> +
>> +static SysBusDeviceInfo cpu_device_info = {
>> +    .qdev.name = "cpu-ppc",
>> +    .qdev.size = sizeof(CPUPPC),
>> +    .qdev.reset = cpu_device_reset,
>> +    .init = cpu_device_init,
>> +    .qdev.props = (Property[]) {
>> +        DEFINE_PROP_STRING("model", CPUPPC, model),
>> +        DEFINE_PROP_END_OF_LIST(),
>> +    },
>> +};
>> +
>> +static void ppc_register_devices(void)
>> +{
>> +    sysbus_register_withprop(&cpu_device_info);
>> +}
>> +
>> +device_init(ppc_register_devices)
>> diff --git a/hw/ppc.h b/hw/ppc.h
>> index 34f54cf..ae8dd97 100644
>> --- a/hw/ppc.h
>> +++ b/hw/ppc.h
>> @@ -37,6 +37,8 @@ void ppce500_irq_init (CPUState *env);
>> void ppc6xx_irq_init (CPUState *env);
>> void ppc970_irq_init (CPUState *env);
>>
>> +DeviceState *cpu_ppc_create_simple(const char *cpu_model);
>> +
>> /* PPC machines for OpenBIOS */
>> enum {
>>     ARCH_PREP = 0,
>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>> index deb8d7c..0f56d45 100644
>> --- a/target-ppc/cpu.h
>> +++ b/target-ppc/cpu.h
>> @@ -721,6 +721,7 @@ struct mmu_ctx_t {
>>
>> /*****************************************************************************/
>> CPUPPCState *cpu_ppc_init (const char *cpu_model);
>> +int cpu_ppc_init_inplace(CPUPPCState *env, const char *cpu_model);
>> void ppc_translate_init(void);
>> int cpu_ppc_exec (CPUPPCState *s);
>> void cpu_ppc_close (CPUPPCState *s);
>> diff --git a/target-ppc/helper.c b/target-ppc/helper.c
>> index 4b49101..99af1f6 100644
>> --- a/target-ppc/helper.c
>> +++ b/target-ppc/helper.c
>> @@ -2794,22 +2794,33 @@ void cpu_reset(CPUPPCState *env)
>>     tlb_flush(env, 1);
>> }
>>
>> -CPUPPCState *cpu_ppc_init (const char *cpu_model)
>> +int cpu_ppc_init_inplace(CPUPPCState *env, const char *cpu_model)
>> {
>> -    CPUPPCState *env;
>>     const ppc_def_t *def;
>>
>>     def = cpu_ppc_find_by_name(cpu_model);
>> -    if (!def)
>> -        return NULL;
>> +    if (!def) {
>> +        return -1;
>> +    }
>>
>> -    env = qemu_mallocz(sizeof(CPUPPCState));
>>     cpu_exec_init(env);
>>     ppc_translate_init();
>>     env->cpu_model_str = cpu_model;
>>     cpu_ppc_register_internal(env, def);
>>
>>     qemu_init_vcpu(env);
>> +    return 0;
>> +}
>> +
>> +CPUPPCState *cpu_ppc_init(const char *cpu_model)
>> +{
>> +    CPUPPCState *env;
>> +
>> +    env = qemu_mallocz(sizeof(CPUPPCState));
>> +    if (cpu_ppc_init_inplace(env, cpu_model) < 0) {
>>     
>
> Why would we need this function again if the CPUs are qdev'ified?
>   
This function is not added ; it is already an existing one (see 25 lines 
before). I kept it to not put in the same patch the CPU qdev'ification 
and the change of all the callers.
Indeed, a second patch may be created to change all callers to use 
cpu_ppc_create_simple() and to remove this function.

> Overall, I really like the idea of moving CPUs to qdev though. Makes things a lot more structured.
>   
Thanks

Hervé

Re: [Qemu-devel] [RFC] ppc: qdev-ify CPU creation

[Alexander Graf]

On 24.07.2011, at 21:08, Hervé Poussineau wrote:

> Alexander Graf a écrit :
>> On 21.12.2010, at 21:01, Andreas Färber wrote:
>> 
>>  
>>> From: Hervé Poussineau <hpoussin@reactos.org>
>>> 
>>> v1:
>>> * Coding style fixes.
>>> 
>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>> Cc: Alexander Graf <agraf@suse.de>
>>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>>> ---
>>> 
>>> Hello Alex,
>>> 
>>> Seeing the discussions about Leon3, is this the way to go for ppc? Is ppc.[hc] right?
>>> 
>>> The unconditional use of 6xx looks suspicious to me, no?
>>> Should we rename cpu_device_irq_request() to cpu_device_irq_request_6xx()?
>>> 
>>> Regards,
>>> Andreas
>>> 
>>> hw/ppc.c            |   75 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>> hw/ppc.h            |    2 +
>>> target-ppc/cpu.h    |    1 +
>>> target-ppc/helper.c |   21 +++++++++++---
>>> 4 files changed, 94 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/hw/ppc.c b/hw/ppc.c
>>> index 968aec1..0927326 100644
>>> --- a/hw/ppc.c
>>> +++ b/hw/ppc.c
>>> @@ -30,6 +30,8 @@
>>> #include "loader.h"
>>> #include "kvm.h"
>>> #include "kvm_ppc.h"
>>> +#include "hw/qdev.h"
>>> +#include "hw/sysbus.h"
>>> 
>>> //#define PPC_DEBUG_IRQ
>>> //#define PPC_DEBUG_TB
>>> @@ -1286,3 +1288,76 @@ int PPC_NVRAM_set_params (nvram_t *nvram, uint16_t NVRAM_size,
>>> 
>>>    return 0;
>>> }
>>> +
>>> +DeviceState *cpu_ppc_create_simple(const char *cpu_model)
>>> +{
>>> +    DeviceState *dev;
>>> +
>>> +    dev = qdev_create(NULL, "cpu-ppc");
>>> +    if (!dev) {
>>> +        return NULL;
>>> +    }
>>> +    qdev_prop_set_string(dev, "model", qemu_strdup(cpu_model));
>>> +    if (qdev_init(dev) < 0) {
>>> +        return NULL;
>>> +    }
>>> +    return dev;
>>> +}
>>> +
>>> +typedef struct CPUPPC {
>>> +    SysBusDevice busdev;
>>>    
>> 
>> I'm not sure we really want CPUs on the sysbus. They belong to their own CPU bus. Basically, I think we should try to model our bus topology so that it reflects the bus topology in the device tree 1:1. Then generating a device tree from the bug information and some device specific callbacks would be possible.
>> 
>>  
> CPUs don't need a bus with specific capabilities, so I used the most simple existing one, ie SysBus.

Yeah, that's nice and simple from the implementation POV, but I have this funny idea that we might be able to generate a device tree from qemu's bus topology. And at that point we'll have to have a separate CPU bus.

>>> +    char *model;
>>> +    CPUPPCState state;
>>> +} CPUPPC;
>>> +
>>> +static void cpu_device_irq_request(void *opaque, int pin, int level)
>>> +{
>>> +    CPUPPC* cpu = opaque;
>>> +    CPUPPCState* env = &cpu->state;
>>> +    ppc6xx_set_irq(env, pin, level);
>>> +}
>>> +
>>> +static int cpu_device_init(SysBusDevice *dev)
>>> +{
>>> +    CPUPPC* cpu = FROM_SYSBUS(CPUPPC, dev);
>>> +    CPUPPCState* env = &cpu->state;
>>> +
>>> +    if (cpu_ppc_init_inplace(env, cpu->model) < 0) {
>>> +        return -1;
>>> +    }
>>> +
>>> +    if (env->flags & POWERPC_FLAG_RTC_CLK) {
>>>    
>> 
>> Where does this flag suddenly come from? Is this related to qdev'ification?
>> 
>>  
> It's not related to qdev'ification. It is already set in CPU definitions since a long time.

Hrm. So where is it usually read out then? Shouldn't that code go away?

> 
>>> +        /* POWER / PowerPC 601 RTC clock frequency is 7.8125 MHz */
>>> +        cpu_ppc_tb_init(env, 7812500UL);
>>> +    } else {
>>> +        /* Set time-base frequency to 100 Mhz */
>>> +        cpu_ppc_tb_init(env, 100UL * 1000UL * 1000UL);
>>>    
>> 
>> Usually we have a TB frequency of 400Mhz in our board/devtrees hardcoded in the TCG case. How about a qdev property that the creator could just modify to its needs? We won't need the special 601 flag then either - just move that into the PREP code.
>> 
>>  
> This code has been extracted from ppc_prep.c ; a qdev property is also fine.

Yes, please :)

>>> +    }
>>> +
>>> +    qdev_init_gpio_in(&dev->qdev, cpu_device_irq_request, PPC6xx_INPUT_NB);
>>> +    return 0;
>>> +}
>>> +
>>> +static void cpu_device_reset(DeviceState *d)
>>> +{
>>> +    CPUPPC *s = FROM_SYSBUS(CPUPPC, sysbus_from_qdev(d));
>>> +    cpu_reset(&s->state);
>>> +}
>>> +
>>> +static SysBusDeviceInfo cpu_device_info = {
>>> +    .qdev.name = "cpu-ppc",
>>> +    .qdev.size = sizeof(CPUPPC),
>>> +    .qdev.reset = cpu_device_reset,
>>> +    .init = cpu_device_init,
>>> +    .qdev.props = (Property[]) {
>>> +        DEFINE_PROP_STRING("model", CPUPPC, model),
>>> +        DEFINE_PROP_END_OF_LIST(),
>>> +    },
>>> +};
>>> +
>>> +static void ppc_register_devices(void)
>>> +{
>>> +    sysbus_register_withprop(&cpu_device_info);
>>> +}
>>> +
>>> +device_init(ppc_register_devices)
>>> diff --git a/hw/ppc.h b/hw/ppc.h
>>> index 34f54cf..ae8dd97 100644
>>> --- a/hw/ppc.h
>>> +++ b/hw/ppc.h
>>> @@ -37,6 +37,8 @@ void ppce500_irq_init (CPUState *env);
>>> void ppc6xx_irq_init (CPUState *env);
>>> void ppc970_irq_init (CPUState *env);
>>> 
>>> +DeviceState *cpu_ppc_create_simple(const char *cpu_model);
>>> +
>>> /* PPC machines for OpenBIOS */
>>> enum {
>>>    ARCH_PREP = 0,
>>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>>> index deb8d7c..0f56d45 100644
>>> --- a/target-ppc/cpu.h
>>> +++ b/target-ppc/cpu.h
>>> @@ -721,6 +721,7 @@ struct mmu_ctx_t {
>>> 
>>> /*****************************************************************************/
>>> CPUPPCState *cpu_ppc_init (const char *cpu_model);
>>> +int cpu_ppc_init_inplace(CPUPPCState *env, const char *cpu_model);
>>> void ppc_translate_init(void);
>>> int cpu_ppc_exec (CPUPPCState *s);
>>> void cpu_ppc_close (CPUPPCState *s);
>>> diff --git a/target-ppc/helper.c b/target-ppc/helper.c
>>> index 4b49101..99af1f6 100644
>>> --- a/target-ppc/helper.c
>>> +++ b/target-ppc/helper.c
>>> @@ -2794,22 +2794,33 @@ void cpu_reset(CPUPPCState *env)
>>>    tlb_flush(env, 1);
>>> }
>>> 
>>> -CPUPPCState *cpu_ppc_init (const char *cpu_model)
>>> +int cpu_ppc_init_inplace(CPUPPCState *env, const char *cpu_model)
>>> {
>>> -    CPUPPCState *env;
>>>    const ppc_def_t *def;
>>> 
>>>    def = cpu_ppc_find_by_name(cpu_model);
>>> -    if (!def)
>>> -        return NULL;
>>> +    if (!def) {
>>> +        return -1;
>>> +    }
>>> 
>>> -    env = qemu_mallocz(sizeof(CPUPPCState));
>>>    cpu_exec_init(env);
>>>    ppc_translate_init();
>>>    env->cpu_model_str = cpu_model;
>>>    cpu_ppc_register_internal(env, def);
>>> 
>>>    qemu_init_vcpu(env);
>>> +    return 0;
>>> +}
>>> +
>>> +CPUPPCState *cpu_ppc_init(const char *cpu_model)
>>> +{
>>> +    CPUPPCState *env;
>>> +
>>> +    env = qemu_mallocz(sizeof(CPUPPCState));
>>> +    if (cpu_ppc_init_inplace(env, cpu_model) < 0) {
>>>    
>> 
>> Why would we need this function again if the CPUs are qdev'ified?
>>  
> This function is not added ; it is already an existing one (see 25 lines before). I kept it to not put in the same patch the CPU qdev'ification and the change of all the callers.
> Indeed, a second patch may be created to change all callers to use cpu_ppc_create_simple() and to remove this function.

Ah, I see. Please write up a patch set that goes through the bits of fully converting it to Qdev :)


Alex

Re: [Qemu-devel] [RFC] ppc: qdev-ify CPU creation

[Andreas Färber]

Am 25.07.2011 um 12:09 schrieb Alexander Graf:

> On 24.07.2011, at 21:08, Hervé Poussineau wrote:
>
>> Alexander Graf a écrit :
>>> On 21.12.2010, at 21:01, Andreas Färber wrote:
>>>
>>>> From: Hervé Poussineau <hpoussin@reactos.org>
>>>>
>>>> v1:
>>>> * Coding style fixes.
>>>>
>>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>>> Cc: Alexander Graf <agraf@suse.de>
>>>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>>>> ---
>>>>
>>>> Hello Alex,
>>>>
>>>> Seeing the discussions about Leon3, is this the way to go for  
>>>> ppc? Is ppc.[hc] right?
>>>>
>>>> The unconditional use of 6xx looks suspicious to me, no?
>>>> Should we rename cpu_device_irq_request() to  
>>>> cpu_device_irq_request_6xx()?
>>>>
>>>> Regards,
>>>> Andreas

[snip]

>>>> +    if (env->flags & POWERPC_FLAG_RTC_CLK) {
>>>>
>>>
>>> Where does this flag suddenly come from? Is this related to  
>>> qdev'ification?
>>>
>>>
>> It's not related to qdev'ification. It is already set in CPU  
>> definitions since a long time.
>
> Hrm. So where is it usually read out then? Shouldn't that code go  
> away?

This is code moved 1:1 from PReP code to general ppc code, which I  
referred to above. I fear this is wrong.

>>>> +        /* POWER / PowerPC 601 RTC clock frequency is 7.8125 MHz  
>>>> */
>>>> +        cpu_ppc_tb_init(env, 7812500UL);
>>>> +    } else {
>>>> +        /* Set time-base frequency to 100 Mhz */
>>>> +        cpu_ppc_tb_init(env, 100UL * 1000UL * 1000UL);
>>>>
>>>
>>> Usually we have a TB frequency of 400Mhz in our board/devtrees  
>>> hardcoded in the TCG case. How about a qdev property that the  
>>> creator could just modify to its needs? We won't need the special  
>>> 601 flag then either - just move that into the PREP code.
>>>
>>>
>> This code has been extracted from ppc_prep.c ; a qdev property is  
>> also fine.
>
> Yes, please :)

>>>> +CPUPPCState *cpu_ppc_init(const char *cpu_model)
>>>> +{
>>>> +    CPUPPCState *env;
>>>> +
>>>> +    env = qemu_mallocz(sizeof(CPUPPCState));
>>>> +    if (cpu_ppc_init_inplace(env, cpu_model) < 0) {
>>>>
>>>
>>> Why would we need this function again if the CPUs are qdev'ified?
>>>
>> This function is not added ; it is already an existing one (see 25  
>> lines before). I kept it to not put in the same patch the CPU  
>> qdev'ification and the change of all the callers.
>> Indeed, a second patch may be created to change all callers to use  
>> cpu_ppc_create_simple() and to remove this function.
>
> Ah, I see. Please write up a patch set that goes through the bits of  
> fully converting it to Qdev :)

Could you please consider pulling my more recent patch of a PReP-local  
static function for shared CPU initialization into ppc-next? That way  
we can share code between PReP machines now and easily convert it in  
one central place when CPU qdev'ification has been completed.

Andreas

Re: [Qemu-devel] [RFC] ppc: qdev-ify CPU creation

[Alexander Graf]

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


On 25.07.2011, at 23:17, Andreas Färber wrote:

> Am 25.07.2011 um 12:09 schrieb Alexander Graf:
> 
>> On 24.07.2011, at 21:08, Hervé Poussineau wrote:
> 
>>>>> +CPUPPCState *cpu_ppc_init(const char *cpu_model)
>>>>> +{
>>>>> +    CPUPPCState *env;
>>>>> +
>>>>> +    env = qemu_mallocz(sizeof(CPUPPCState));
>>>>> +    if (cpu_ppc_init_inplace(env, cpu_model) < 0) {
>>>>> 
>>>> 
>>>> Why would we need this function again if the CPUs are qdev'ified?
>>>> 
>>> This function is not added ; it is already an existing one (see 25 lines before). I kept it to not put in the same patch the CPU qdev'ification and the change of all the callers.
>>> Indeed, a second patch may be created to change all callers to use cpu_ppc_create_simple() and to remove this function.
>> 
>> Ah, I see. Please write up a patch set that goes through the bits of fully converting it to Qdev :)
> 
> Could you please consider pulling my more recent patch of a PReP-local static function for shared CPU initialization into ppc-next? That way we can share code between PReP machines now and easily convert it in one central place when CPU qdev'ification has been completed.

Got a subject line for me? :)


Alex


[-- Attachment #2: Type: text/html, Size: 3205 bytes --]