| Message ID | a00d788d723bada207cc99e3e74a1deec80455f1.1402363608.git.alistair.francis@xilinx.com |
|---|---|
| State | New |
| Headers | show |
Ping On Tue, Jun 10, 2014 at 11:32 AM, Alistair Francis <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com> > --- > 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? > > 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 = 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_ARM_CPU); > + * object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL); > + */ > + > memory_region_init(&s->container, obj, "a9mp-priv-container", 0x2000); > sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->container); > > @@ -50,6 +56,40 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp) > Error *err = NULL; > int i; > > + /* Just a temporary measure to not break machines that init the CPU > + * seperatly */ > + if (!first_cpu) { > + s->cpu = g_malloc(sizeof(ARMCPU) * s->num_cpu); > + for (i = 0; i < s->num_cpu; i++) { > + object_initialize((s->cpu + i), sizeof(*(s->cpu + i)), > + "cortex-a9-" TYPE_ARM_CPU); > + > + 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->reset_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); > + } > + > scudev = 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[] = { > * Other boards may differ and should set this property appropriately. > */ > 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; > + uint64_t reset_cbar; > + > A9SCUState scu; > GICState gic; > A9GTimerState gtimer; > -- > 1.7.1 >
On Tue, Jun 10, 2014 at 11:32 AM, Alistair Francis <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com> > --- > 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? > > 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 = 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_ARM_CPU); > + * object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL); > + */ > + 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? > memory_region_init(&s->container, obj, "a9mp-priv-container", 0x2000); > sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->container); > > @@ -50,6 +56,40 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp) > Error *err = NULL; > int i; > > + /* Just a temporary measure to not break machines that init the CPU > + * seperatly */ "separately" > + if (!first_cpu) { > + s->cpu = g_malloc(sizeof(ARMCPU) * s->num_cpu); g_new should be use to allocate arrays. > + for (i = 0; i < s->num_cpu; i++) { > + object_initialize((s->cpu + i), sizeof(*(s->cpu + i)), &s->cpu[i] is more common and easier to read. sizeof(*s->cpu) is fine. > + "cortex-a9-" TYPE_ARM_CPU); 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. > + > + 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->reset_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); Why free the just-initialized CPUs? > + } > + > scudev = 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[] = { > * Other boards may differ and should set this property appropriately. > */ > 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; I'd preface this as "cpu_midr". > + uint64_t reset_cbar; MPCores refer to this as PERIPHBASE in their documentation. Regards, Peter > + > A9SCUState scu; > GICState gic; > A9GTimerState gtimer; > -- > 1.7.1 > >
On Mon, Jun 16, 2014 at 2:43 PM, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Tue, Jun 10, 2014 at 11:32 AM, Alistair Francis > <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com> >> --- >> 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? >> >> 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 = 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_ARM_CPU); >> + * object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL); >> + */ >> + > > 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? I'm open for ideas/opinions. The method used here seemed to be the easiest to implement (and actually the only reliable method that I could think of). > >> memory_region_init(&s->container, obj, "a9mp-priv-container", 0x2000); >> sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->container); >> >> @@ -50,6 +56,40 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp) >> Error *err = NULL; >> int i; >> >> + /* Just a temporary measure to not break machines that init the CPU >> + * seperatly */ > > "separately" > >> + if (!first_cpu) { >> + s->cpu = g_malloc(sizeof(ARMCPU) * s->num_cpu); > > g_new should be use to allocate arrays. > >> + for (i = 0; i < s->num_cpu; i++) { >> + object_initialize((s->cpu + i), sizeof(*(s->cpu + i)), > > &s->cpu[i] is more common and easier to read. > > sizeof(*s->cpu) is fine. > >> + "cortex-a9-" TYPE_ARM_CPU); > > 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. > >> + >> + 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->reset_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); > > Why free the just-initialized CPUs? I shouldn't have done that, I don't know how that slipped through > >> + } >> + >> scudev = 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[] = { >> * Other boards may differ and should set this property appropriately. >> */ >> 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; > > I'd preface this as "cpu_midr". > >> + uint64_t reset_cbar; > > MPCores refer to this as PERIPHBASE in their documentation. > > Regards, > Peter All comments were changed. I'll give it a few days and see if anyone else has any comments, otherwise I might release a patch following the same style as this RFC > >> + >> A9SCUState scu; >> GICState gic; >> A9GTimerState gtimer; >> -- >> 1.7.1 >> >> >
On 10 June 2014 02:32, Alistair Francis <alistair.francis@xilinx.com> wrote: > This patch adds the Cortex-A9 ARM CPU to the A9MPCore. I think this is in general the right way to go. > + /* Properties for the A9 CPU */ > + DEFINE_PROP_UINT32("midr", A9MPPrivState, midr, 0), > + DEFINE_PROP_UINT64("reset-cbar", A9MPPrivState, reset_cbar, 0), It seems a bit error-prone if every container object has to manually mirror the properties of the individual CPUs out like this; maybe there's a better way we could come up with? Paolo, weren't you looking at passthrough properties for the virtio devices? thanks -- PMM
On Mon, Jun 16, 2014 at 5:40 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 10 June 2014 02:32, Alistair Francis <alistair.francis@xilinx.com> wrote: >> This patch adds the Cortex-A9 ARM CPU to the A9MPCore. > > I think this is in general the right way to go. > >> + /* Properties for the A9 CPU */ >> + DEFINE_PROP_UINT32("midr", A9MPPrivState, midr, 0), >> + DEFINE_PROP_UINT64("reset-cbar", A9MPPrivState, reset_cbar, 0), > > It seems a bit error-prone if every container object has to manually > mirror the properties of the individual CPUs out like this; maybe > there's a better way we could come up with? > > Paolo, weren't you looking at passthrough properties for the > virtio devices? > Stefan added support for QDEV property aliasing. http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg06489.html The bigger issue though is how do you do an N:1 mapping. The container should only have 1 "midr" prop, but it should mirror to all contained CPUs. Should we add multiplicity to the aliasing feature? Regards, Peter > thanks > -- PMM >
Am 16.06.2014 06:43, schrieb Peter Crosthwaite: > On Tue, Jun 10, 2014 at 11:32 AM, Alistair Francis > <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com> >> --- >> 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 = 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_ARM_CPU); >> + * object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL); >> + */ >> + > > 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", 0x2000); >> sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->container); >> >> @@ -50,6 +56,40 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp) >> Error *err = NULL; >> int i; >> >> + /* Just a temporary measure to not break machines that init the CPU >> + * seperatly */ > > "separately" > >> + if (!first_cpu) { >> + s->cpu = g_malloc(sizeof(ARMCPU) * s->num_cpu); > > 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! :) > >> + for (i = 0; i < s->num_cpu; i++) { >> + object_initialize((s->cpu + i), sizeof(*(s->cpu + i)), > > &s->cpu[i] is more common and easier to read. > > sizeof(*s->cpu) is fine. > >> + "cortex-a9-" TYPE_ARM_CPU); > > 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 > >> + >> + 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->reset_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); > > Why free the just-initialized CPUs? > >> + } >> + >> scudev = 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[] = { >> * Other boards may differ and should set this property appropriately. >> */ >> 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; > > I'd preface this as "cpu_midr". > >> + uint64_t reset_cbar; > > 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
Am 16.06.2014 08:04, schrieb Alistair Francis: > On Mon, Jun 16, 2014 at 2:43 PM, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> On Tue, Jun 10, 2014 at 11:32 AM, Alistair Francis >> <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com> >>> --- >>> 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? >>> >>> 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 = 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_ARM_CPU); >>> + * object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL); >>> + */ >>> + >> >> 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? > > I'm open for ideas/opinions. The method used here seemed to be the easiest > to implement (and actually the only reliable method that I could think of). The point is that it needs to happen before realize, so that the object can be inspected and modified before it's too late, and allocations may fail so should be able to return an error to the caller. If there is nothing that suits your needs, you can either implement a new type of static qdev property or, easiest, implement a dynamic QOM property that takes actions once the number-of-CPUs value is set. An example using dynamic properties may be found around the sPAPR interrupt controllers. Cheers, Andreas
On Mon, Jun 16, 2014 at 8:19 PM, Andreas Färber <afaerber@suse.de> wrote: > Am 16.06.2014 06:43, schrieb Peter Crosthwaite: >> On Tue, Jun 10, 2014 at 11:32 AM, Alistair Francis >> <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com> >>> --- >>> 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 = 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_ARM_CPU); >>> + * object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL); >>> + */ >>> + >> >> 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. I guess it's only a small step to a property that init's an array of child objects. > 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. > I'll go with child. The CPU does not exist outside the MPCore. They are a heirachy, not-peers and the qom-composition should reflect that. >>> memory_region_init(&s->container, obj, "a9mp-priv-container", 0x2000); >>> sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->container); >>> >>> @@ -50,6 +56,40 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp) >>> Error *err = NULL; >>> int i; >>> >>> + /* Just a temporary measure to not break machines that init the CPU >>> + * seperatly */ >> >> "separately" >> >>> + if (!first_cpu) { >>> + s->cpu = g_malloc(sizeof(ARMCPU) * s->num_cpu); >> >> 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! :) > >> >>> + for (i = 0; i < s->num_cpu; i++) { >>> + object_initialize((s->cpu + i), sizeof(*(s->cpu + i)), >> >> &s->cpu[i] is more common and easier to read. >> >> sizeof(*s->cpu) is fine. >> >>> + "cortex-a9-" TYPE_ARM_CPU); >> >> 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 > >> >>> + >>> + 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->reset_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); >> >> Why free the just-initialized CPUs? >> >>> + } >>> + >>> scudev = 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[] = { >>> * Other boards may differ and should set this property appropriately. >>> */ >>> 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; >> >> I'd preface this as "cpu_midr". >> >>> + uint64_t reset_cbar; >> >> 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. > This should evaporate if the alias system is used somehow. No need for state or a setter fn. But either way, the alias should do the rename. That way both MPCore and ARMCPU match their documentation without one have to compromise to the others naming scheme. Regards, Peter > Regards, > Andreas > >>> + >>> A9SCUState scu; >>> GICState gic; >>> A9GTimerState gtimer; >>> -- >>> 1.7.1 > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg >
On 16 June 2014 11:19, Andreas Färber <afaerber@suse.de> wrote: > Am 16.06.2014 06:43, schrieb Peter Crosthwaite: >> 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. Was I? I can't currently see any reason you'd want them to be siblings of the container rather than children... I think property-listeners was the mechanism we talked about for when you need to do something before realize but it depends on some property, yes. >>> memory_region_init(&s->container, obj, "a9mp-priv-container", 0x2000); >>> sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->container); >>> >>> @@ -50,6 +56,40 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp) >>> Error *err = NULL; >>> int i; >>> >>> + /* Just a temporary measure to not break machines that init the CPU >>> + * seperatly */ >> >> "separately" >> >>> + if (!first_cpu) { >>> + s->cpu = g_malloc(sizeof(ARMCPU) * s->num_cpu); >> >> 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! :) > >> >>> + for (i = 0; i < s->num_cpu; i++) { >>> + object_initialize((s->cpu + i), sizeof(*(s->cpu + i)), >> >> &s->cpu[i] is more common and easier to read. >> >> sizeof(*s->cpu) is fine. >> >>> + "cortex-a9-" TYPE_ARM_CPU); >> >> 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 > >> >>> + >>> + 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->reset_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); >> >> Why free the just-initialized CPUs? >> >>> + } >>> + >>> scudev = 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[] = { >>> * Other boards may differ and should set this property appropriately. >>> */ >>> 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; >> >> I'd preface this as "cpu_midr". >> >>> + uint64_t reset_cbar; >> >> MPCores refer to this as PERIPHBASE in their documentation. Well, generally the external config signal is PERIPHBASE and the register it affects is CBAR. For QEMU I think we seem to be generally using CBAR in all contexts. thanks -- PMM
Am 16.06.2014 12:34, schrieb Peter Crosthwaite: > On Mon, Jun 16, 2014 at 8:19 PM, Andreas Färber <afaerber@suse.de> wrote: >> 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. >> > > I'll go with child. The CPU does not exist outside the MPCore. They > are a heirachy, not-peers and the qom-composition should reflect that. Well, for Cortex-A9 that may work. But Cortex-A15 (and Cortex-A5x if existant by now) should also be refactored alongside, as proof of concept - can you really create num_cpu cortex-a15 CPUs on the MPCore for a big.LITTLE configuration? I'd be really surprised if there were separate MPCore devices per cluster. That would then indicate that the homogeneity assumption among CPUs within an MPCore is wrong and we need to let its parent create the CPUs rather than an MPCore property. Besides, not all CPUs have an MPCore, Cortex-A8 and Cortex-A5 come to mind, so we should be aware that ARMCPU child<>s on the MPCore will lead to asymmetry between SoCs. But that shouldn't stop proper Cortex-A9/-A15 modeling, just like Quark and Baytrail SoCs will inevitably lead to modeling differences in the PC world. Regards, Andreas
On 16 June 2014 11:58, Andreas Färber <afaerber@suse.de> wrote: > Well, for Cortex-A9 that may work. But Cortex-A15 (and Cortex-A5x if > existant by now) should also be refactored alongside, as proof of > concept - can you really create num_cpu cortex-a15 CPUs on the MPCore > for a big.LITTLE configuration? I'd be really surprised if there were > separate MPCore devices per cluster. That would then indicate that the > homogeneity assumption among CPUs within an MPCore is wrong and we need > to let its parent create the CPUs rather than an MPCore property. Not sure what the relevance of big.LITTLE is here -- QEMU simply doesn't support heterogenous CPU configurations so we can't model big.LITTLE at all. If we did, it would be via having a SoC with one a15mpcore and one a7mpcore. (This is how the hardware does it -- there are two multicore clusters, plus some cache coherency interconnect magic.) > Besides, not all CPUs have an MPCore, Cortex-A8 and Cortex-A5 come to > mind, so we should be aware that ARMCPU child<>s on the MPCore will lead > to asymmetry between SoCs. For the A8 and A5 the SoC object would just instantiate them directly -- there's no equivalent in the hardware of the "n CPUs and their private devices" layer. So I think any asymmetry between SoCs in QEMU is just a reflection of the differences in the hardware. thanks -- PMM
Am 16.06.2014 13:11, schrieb Peter Maydell: > On 16 June 2014 11:58, Andreas Färber <afaerber@suse.de> wrote: >> Well, for Cortex-A9 that may work. But Cortex-A15 (and Cortex-A5x if >> existant by now) should also be refactored alongside, as proof of >> concept - can you really create num_cpu cortex-a15 CPUs on the MPCore >> for a big.LITTLE configuration? I'd be really surprised if there were >> separate MPCore devices per cluster. That would then indicate that the >> homogeneity assumption among CPUs within an MPCore is wrong and we need >> to let its parent create the CPUs rather than an MPCore property. > > Not sure what the relevance of big.LITTLE is here -- QEMU > simply doesn't support heterogenous CPU configurations so > we can't model big.LITTLE at all. Not today, but neither can the user fiddle with properties before realize today. So better not put blockers to known future features, in particular since I've been working into that direction. Two Cortex-As with identical software features and just different cache sizes like A53/A57 should be the easiest inhomogeneous configuration, compared to my Vybrid A5+M4 work. > If we did, it would be > via having a SoC with one a15mpcore and one a7mpcore. > (This is how the hardware does it -- there are two > multicore clusters, plus some cache coherency interconnect > magic.) Then I'm surprised and we have one issue less to worry about. :) Thanks, Andreas
Am 16.06.2014 12:44, schrieb Peter Maydell: > On 16 June 2014 11:19, Andreas Färber <afaerber@suse.de> wrote: >> Am 16.06.2014 06:43, schrieb Peter Crosthwaite: >>> 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. > > Was I? I can't currently see any reason you'd want them to be > siblings of the container rather than children... > > I think property-listeners was the mechanism we talked > about for when you need to do something before realize > but it depends on some property, yes. Got a pointer to that? Waiting for review or merged in my absence? Andreas
On 16 June 2014 12:18, Andreas Färber <afaerber@suse.de> wrote: > Am 16.06.2014 12:44, schrieb Peter Maydell: >> I think property-listeners was the mechanism we talked >> about for when you need to do something before realize >> but it depends on some property, yes. > > Got a pointer to that? Waiting for review or merged in my absence? No, I think it was just a mailing list discussion where we said "this is how we would do it if we need to", in not much more detail than we have in this thread :-) -- PMM
On Mon, Jun 16, 2014 at 9:11 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 16 June 2014 11:58, Andreas Färber <afaerber@suse.de> wrote: >> Well, for Cortex-A9 that may work. But Cortex-A15 (and Cortex-A5x if >> existant by now) should also be refactored alongside, as proof of >> concept - can you really create num_cpu cortex-a15 CPUs on the MPCore >> for a big.LITTLE configuration? I'd be really surprised if there were >> separate MPCore devices per cluster. That would then indicate that the >> homogeneity assumption among CPUs within an MPCore is wrong and we need >> to let its parent create the CPUs rather than an MPCore property. > > Not sure what the relevance of big.LITTLE is here -- QEMU > simply doesn't support heterogenous CPU configurations so > we can't model big.LITTLE at all. If we did, it would be > via having a SoC with one a15mpcore and one a7mpcore. > (This is how the hardware does it -- there are two > multicore clusters, plus some cache coherency interconnect > magic.) > >> Besides, not all CPUs have an MPCore, Cortex-A8 and Cortex-A5 come to >> mind, so we should be aware that ARMCPU child<>s on the MPCore will lead >> to asymmetry between SoCs. > > For the A8 and A5 the SoC object would just instantiate them > directly -- there's no equivalent in the hardware of the > "n CPUs and their private devices" layer. So I think > any asymmetry between SoCs in QEMU is just a reflection > of the differences in the hardware. > +1. Either you have an MPCore package as part of your soc or standalone CPU. Both are valid instantiables on the SoC level. Regards, Peter > thanks > -- PMM >
Am 16.06.2014 13:22, schrieb Peter Crosthwaite: > On Mon, Jun 16, 2014 at 9:11 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 16 June 2014 11:58, Andreas Färber <afaerber@suse.de> wrote: >>> Besides, not all CPUs have an MPCore, Cortex-A8 and Cortex-A5 come to >>> mind, so we should be aware that ARMCPU child<>s on the MPCore will lead >>> to asymmetry between SoCs. >> >> For the A8 and A5 the SoC object would just instantiate them >> directly -- there's no equivalent in the hardware of the >> "n CPUs and their private devices" layer. So I think >> any asymmetry between SoCs in QEMU is just a reflection >> of the differences in the hardware. >> > > +1. Either you have an MPCore package as part of your soc or > standalone CPU. Both are valid instantiables on the SoC level. That's exactly what I said and PMM cut, so no argument here. Andreas
On Mon, Jun 16, 2014 at 05:46:24PM +1000, Peter Crosthwaite wrote: > On Mon, Jun 16, 2014 at 5:40 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > > On 10 June 2014 02:32, Alistair Francis <alistair.francis@xilinx.com> wrote: > >> This patch adds the Cortex-A9 ARM CPU to the A9MPCore. > > > > I think this is in general the right way to go. > > > >> + /* Properties for the A9 CPU */ > >> + DEFINE_PROP_UINT32("midr", A9MPPrivState, midr, 0), > >> + DEFINE_PROP_UINT64("reset-cbar", A9MPPrivState, reset_cbar, 0), > > > > It seems a bit error-prone if every container object has to manually > > mirror the properties of the individual CPUs out like this; maybe > > there's a better way we could come up with? > > > > Paolo, weren't you looking at passthrough properties for the > > virtio devices? > > > > Stefan added support for QDEV property aliasing. > > http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg06489.html > > The bigger issue though is how do you do an N:1 mapping. The container > should only have 1 "midr" prop, but it should mirror to all contained > CPUs. Should we add multiplicity to the aliasing feature? If we'll need 1:N alias properties in other places too. Stefan
Il 17/06/2014 09:16, Stefan Hajnoczi ha scritto: > > The bigger issue though is how do you do an N:1 mapping. The container > > should only have 1 "midr" prop, but it should mirror to all contained > > CPUs. Should we add multiplicity to the aliasing feature? > > If we'll need 1:N alias properties in other places too. I think in the case it's the contained object that should look for a midr property in the parent, and possibly create an alias to the parent's midr property. Paolo
On Tue, Jun 17, 2014 at 6:05 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 17/06/2014 09:16, Stefan Hajnoczi ha scritto: > >> > The bigger issue though is how do you do an N:1 mapping. The container >> > should only have 1 "midr" prop, but it should mirror to all contained >> > CPUs. Should we add multiplicity to the aliasing feature? >> >> If we'll need 1:N alias properties in other places too. Well A15 MPCore is the obvious one. Any homogeneous n-way container that wants to forward props could make use of this. > > > I think in the case it's the contained object that should look for a midr > property in the parent, and possibly create an alias to the parent's midr > property. > This sounds too difficult to me. How do you define whether a contained object is allowed to go looking into a parent for suitable aliasing oppurtunities without container level knowledge? The contained object should also be able to create it's property regularly in cases where the parent does not provide an aliasable which adds some ugly iffery to the contained's init fn. Ultimately I think the container should remain in full control and do the 1:N aliasing in a loop when appropriate. And some further food for thought, if we throw Andreas' heterogenous MPCore idea (one of the tangent topics on this thread) in the mix, a container will want to potentially alias the same property on two different children to two different props on the child. The child simply cannot handle this without knowing too much about it's container. Regards, Peter > Paolo >
On Tue, Jun 17, 2014 at 8:12 PM, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Tue, Jun 17, 2014 at 6:05 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> Il 17/06/2014 09:16, Stefan Hajnoczi ha scritto: >> >>> > The bigger issue though is how do you do an N:1 mapping. The container >>> > should only have 1 "midr" prop, but it should mirror to all contained >>> > CPUs. Should we add multiplicity to the aliasing feature? >>> >>> If we'll need 1:N alias properties in other places too. > > Well A15 MPCore is the obvious one. Any homogeneous n-way container > that wants to forward props could make use of this. > >> >> >> I think in the case it's the contained object that should look for a midr >> property in the parent, and possibly create an alias to the parent's midr >> property. >> > > This sounds too difficult to me. How do you define whether a contained > object is allowed to go looking into a parent for suitable aliasing > oppurtunities without container level knowledge? > > The contained object should also be able to create it's property > regularly in cases where the parent does not provide an aliasable > which adds some ugly iffery to the contained's init fn. > > Ultimately I think the container should remain in full control and do > the 1:N aliasing in a loop when appropriate. I agree, the container should have the responsibility of passing properties through. I submitted a v2 of the RFC which does just that (the MPcore container passes through the midr and reset-cbar properties) > > And some further food for thought, if we throw Andreas' heterogenous > MPCore idea (one of the tangent topics on this thread) in the mix, a > container will want to potentially alias the same property on two > different children to two different props on the child. The child > simply cannot handle this without knowing too much about it's > container. > > Regards, > Peter > >> Paolo >> >
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 = 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_ARM_CPU); + * object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL); + */ + memory_region_init(&s->container, obj, "a9mp-priv-container", 0x2000); sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->container); @@ -50,6 +56,40 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp) Error *err = NULL; int i; + /* Just a temporary measure to not break machines that init the CPU + * seperatly */ + if (!first_cpu) { + s->cpu = g_malloc(sizeof(ARMCPU) * s->num_cpu); + for (i = 0; i < s->num_cpu; i++) { + object_initialize((s->cpu + i), sizeof(*(s->cpu + i)), + "cortex-a9-" TYPE_ARM_CPU); + + 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->reset_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); + } + scudev = 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[] = { * Other boards may differ and should set this property appropriately. */ 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; + uint64_t reset_cbar; + A9SCUState scu; GICState gic; A9GTimerState gtimer;
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 <alistair.francis@xilinx.com> --- 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? hw/cpu/a9mpcore.c | 43 +++++++++++++++++++++++++++++++++++++++++++ include/hw/cpu/a9mpcore.h | 4 ++++ 2 files changed, 47 insertions(+), 0 deletions(-)