From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37372) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WwU0p-0005G4-0J for qemu-devel@nongnu.org; Mon, 16 Jun 2014 06:19:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WwU0i-0002pn-OQ for qemu-devel@nongnu.org; Mon, 16 Jun 2014 06:19:38 -0400 Received: from cantor2.suse.de ([195.135.220.15]:35195 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WwU0i-0002pN-Fn for qemu-devel@nongnu.org; Mon, 16 Jun 2014 06:19:32 -0400 Message-ID: <539EC4B1.5090709@suse.de> Date: Mon, 16 Jun 2014 12:19:29 +0200 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite , Alistair Francis Cc: Peter Maydell , "qemu-devel@nongnu.org Developers" Am 16.06.2014 06:43, schrieb Peter Crosthwaite: > On Tue, Jun 10, 2014 at 11:32 AM, Alistair Francis > wrote: >> This patch adds the Cortex-A9 ARM CPU to the A9MPCore. It >> first does a check to make sure no other CPUs exist and if >> they do the Cortex-A9 won't be added. This is implemented to >> maintain compatibility and can be removed once all machines >> have been updated >> >> This patch also allows the midr and reset-property to be set >> >> Signed-off-by: Alistair Francis >> --- >> There comments in the code explaining the reason that the CPU >> is initiated in the realize function. This is because it relies >> on the num_cpu property, which isn't yet set in the initfn >> Is this an acceptable compromise? No, this is not OK as is. The CPUs need to be initialized before realizing them through the parent, otherwise the CPUs can't have any additional properties set by user/management. >> >> hw/cpu/a9mpcore.c | 43 ++++++++++++++++++++++++++++++++++++= +++++++ >> include/hw/cpu/a9mpcore.h | 4 ++++ >> 2 files changed, 47 insertions(+), 0 deletions(-) >> >> diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c >> index c09358c..1159044 100644 >> --- a/hw/cpu/a9mpcore.c >> +++ b/hw/cpu/a9mpcore.c >> @@ -21,6 +21,12 @@ static void a9mp_priv_initfn(Object *obj) >> { >> A9MPPrivState *s =3D A9MPCORE_PRIV(obj); >> >> + /* Ideally would init the CPUs here, but the num_cpu property has= not been >> + * set yet. So that only works if assuming a single CPU >> + * object_initialize(&s->cpu, sizeof(s->cpu), "cortex-a9-" TYPE_A= RM_CPU); >> + * object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL); >> + */ >> + >=20 > So you could add an integer property listener to init them earlier (or > even do dynamic extending/freeing or the allocated CPUs). I'm not sure > exactly what we are really supposed to do though, when the number of > child object depends on a prop like this? Andreas? PMM had added or looked into a property creating an array of properties. However a more fundamental issue that PMM was unsure about is whether the CPUs should be child<> of MPCore as done here or a sibling of the MPCore container. >> memory_region_init(&s->container, obj, "a9mp-priv-container", 0x2= 000); >> sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->container); >> >> @@ -50,6 +56,40 @@ static void a9mp_priv_realize(DeviceState *dev, Err= or **errp) >> Error *err =3D NULL; >> int i; >> >> + /* Just a temporary measure to not break machines that init the C= PU >> + * seperatly */ >=20 > "separately" >=20 >> + if (!first_cpu) { >> + s->cpu =3D g_malloc(sizeof(ARMCPU) * s->num_cpu); >=20 > g_new should be use to allocate arrays. Whether malloc or new, more important is to 0-initialize the memory before running object_initialize() on it! :) >=20 >> + for (i =3D 0; i < s->num_cpu; i++) { >> + object_initialize((s->cpu + i), sizeof(*(s->cpu + i)), >=20 > &s->cpu[i] is more common and easier to read. >=20 > sizeof(*s->cpu) is fine. >=20 >> + "cortex-a9-" TYPE_ARM_CPU); >=20 > Use cpu_class_by_name logic like in some of the boards, rather than > the string concatenation. The specifics of the concatenation system is > (supposed to be) private to target-arm code. +1 >=20 >> + >> + if (s->midr) { >> + object_property_set_int(OBJECT((s->cpu + i)), s->midr= , >> + "midr", &err); >> + if (err) { >> + error_propagate(errp, err); >> + exit(1); >> + } >> + } >> + if (s->reset_cbar) { >> + object_property_set_int(OBJECT((s->cpu + i)), s->rese= t_cbar, >> + "reset-cbar", &err); >> + if (err) { >> + error_propagate(errp, err); >> + exit(1); >> + } >> + } >> + object_property_set_bool(OBJECT((s->cpu + i)), true, >> + "realized", &err); >> + if (err) { >> + error_propagate(errp, err); >> + return; >> + } >> + } >> + g_free(s->cpu); >=20 > Why free the just-initialized CPUs? >=20 >> + } >> + >> scudev =3D DEVICE(&s->scu); >> qdev_prop_set_uint32(scudev, "num-cpu", s->num_cpu); >> object_property_set_bool(OBJECT(&s->scu), true, "realized", &err)= ; >> @@ -152,6 +192,9 @@ static Property a9mp_priv_properties[] =3D { >> * Other boards may differ and should set this property appropria= tely. >> */ >> DEFINE_PROP_UINT32("num-irq", A9MPPrivState, num_irq, 96), >> + /* Properties for the A9 CPU */ >> + DEFINE_PROP_UINT32("midr", A9MPPrivState, midr, 0), >> + DEFINE_PROP_UINT64("reset-cbar", A9MPPrivState, reset_cbar, 0), >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> diff --git a/include/hw/cpu/a9mpcore.h b/include/hw/cpu/a9mpcore.h >> index 5d67ca2..8e395a4 100644 >> --- a/include/hw/cpu/a9mpcore.h >> +++ b/include/hw/cpu/a9mpcore.h >> @@ -29,6 +29,10 @@ typedef struct A9MPPrivState { >> MemoryRegion container; >> uint32_t num_irq; >> >> + ARMCPU *cpu; >> + uint32_t midr; >=20 > I'd preface this as "cpu_midr". >=20 >> + uint64_t reset_cbar; >=20 > MPCores refer to this as PERIPHBASE in their documentation. Why have this as separate state at all? Can't those properties be passed through to the CPUs once created earlier? When the CPUs are not available, setter can return an Error. Regards, Andreas >> + >> A9SCUState scu; >> GICState gic; >> A9GTimerState gtimer; >> -- >> 1.7.1 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg