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 <hpous...@reactos.org>
>>>
>>> v1:
>>> * Coding style fixes.
>>>
>>> Signed-off-by: Hervé Poussineau <hpous...@reactos.org>
>>> Cc: Alexander Graf <ag...@suse.de>
>>> Signed-off-by: Andreas Färber <andreas.faer...@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
