From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:48910) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ql42S-0005m4-NC for qemu-devel@nongnu.org; Sun, 24 Jul 2011 15:08:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ql42R-0006lB-78 for qemu-devel@nongnu.org; Sun, 24 Jul 2011 15:08:32 -0400 Received: from smtp5-g21.free.fr ([212.27.42.5]:52548) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ql42Q-0006kX-GV for qemu-devel@nongnu.org; Sun, 24 Jul 2011 15:08:31 -0400 Message-ID: <4E2C6DA0.6070001@reactos.org> Date: Sun, 24 Jul 2011 21:08:16 +0200 From: =?ISO-8859-1?Q?Herv=E9_Poussineau?= MIME-Version: 1.0 References: <1292961678-1581-1-git-send-email-andreas.faerber@web.de> <316C34FA-D0A1-4155-B9EA-1361F70F1A4C@suse.de> In-Reply-To: <316C34FA-D0A1-4155-B9EA-1361F70F1A4C@suse.de> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC] ppc: qdev-ify CPU creation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: =?ISO-8859-1?Q?Andreas_F=E4rber?= , =?ISO-8859-1?Q?Herv=E9_Poussineau?= , qemu-devel@nongnu.org Alexander Graf a =E9crit : > On 21.12.2010, at 21:01, Andreas F=E4rber wrote: > > =20 >> From: Herv=E9 Poussineau >> >> v1: >> * Coding style fixes. >> >> Signed-off-by: Herv=E9 Poussineau >> Cc: Alexander Graf >> Signed-off-by: Andreas F=E4rber >> --- >> >> 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_6x= x()? >> >> 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, uint1= 6_t NVRAM_size, >> >> return 0; >> } >> + >> +DeviceState *cpu_ppc_create_simple(const char *cpu_model) >> +{ >> + DeviceState *dev; >> + >> + dev =3D 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; >> =20 > > I'm not sure we really want CPUs on the sysbus. They belong to their ow= n 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 callback= s would be possible. > > =20 CPUs don't need a bus with specific capabilities, so I used the most=20 simple existing one, ie SysBus. >> + char *model; >> + CPUPPCState state; >> +} CPUPPC; >> + >> +static void cpu_device_irq_request(void *opaque, int pin, int level) >> +{ >> + CPUPPC* cpu =3D opaque; >> + CPUPPCState* env =3D &cpu->state; >> + ppc6xx_set_irq(env, pin, level); >> +} >> + >> +static int cpu_device_init(SysBusDevice *dev) >> +{ >> + CPUPPC* cpu =3D FROM_SYSBUS(CPUPPC, dev); >> + CPUPPCState* env =3D &cpu->state; >> + >> + if (cpu_ppc_init_inplace(env, cpu->model) < 0) { >> + return -1; >> + } >> + >> + if (env->flags & POWERPC_FLAG_RTC_CLK) { >> =20 > > Where does this flag suddenly come from? Is this related to qdev'ificat= ion? > > =20 It's not related to qdev'ification. It is already set in CPU definitions=20 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); >> =20 > > Usually we have a TB frequency of 400Mhz in our board/devtrees hardcode= d 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 - jus= t move that into the PREP code. > > =20 This code has been extracted from ppc_prep.c ; a qdev property is also=20 fine. >> + } >> + >> + qdev_init_gpio_in(&dev->qdev, cpu_device_irq_request, PPC6xx_INPU= T_NB); >> + return 0; >> +} >> + >> +static void cpu_device_reset(DeviceState *d) >> +{ >> + CPUPPC *s =3D FROM_SYSBUS(CPUPPC, sysbus_from_qdev(d)); >> + cpu_reset(&s->state); >> +} >> + >> +static SysBusDeviceInfo cpu_device_info =3D { >> + .qdev.name =3D "cpu-ppc", >> + .qdev.size =3D sizeof(CPUPPC), >> + .qdev.reset =3D cpu_device_reset, >> + .init =3D cpu_device_init, >> + .qdev.props =3D (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 =3D 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 =3D cpu_ppc_find_by_name(cpu_model); >> - if (!def) >> - return NULL; >> + if (!def) { >> + return -1; >> + } >> >> - env =3D qemu_mallocz(sizeof(CPUPPCState)); >> cpu_exec_init(env); >> ppc_translate_init(); >> env->cpu_model_str =3D 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 =3D qemu_mallocz(sizeof(CPUPPCState)); >> + if (cpu_ppc_init_inplace(env, cpu_model) < 0) { >> =20 > > Why would we need this function again if the CPUs are qdev'ified? > =20 This function is not added ; it is already an existing one (see 25 lines=20 before). I kept it to not put in the same patch the CPU qdev'ification=20 and the change of all the callers. Indeed, a second patch may be created to change all callers to use=20 cpu_ppc_create_simple() and to remove this function. > Overall, I really like the idea of moving CPUs to qdev though. Makes th= ings a lot more structured. > =20 Thanks Herv=E9