Re: [Qemu-devel] [PATCH 8/9] hw/arm/bcm2836: Hardcode correct CPU type
On 19 March 2018 at 14:40, Igor Mammedov wrote: > On Thu, 15 Mar 2018 17:13:03 + > Peter Maydell wrote: >> Igor, can you help with this question? For a board that always >> has one CPU type (because the real hardware only ever has >> one SoC, and that SoC QOM object hard codes the CPU type) >> does it still need to set the mc->default_cpu_type field in >> its MachineClass, or does that become pointless ? > > Since board ignores whatever were specified on '-cpu' and > there aren't any checks in board code for current_machine->cpu_type, > removing mc->default_cpu_type won't really affect anything. > > With current code in vl.c and with default_cpu_type set, user will > get error if he specified non existing cpu_model with -cpu. > If default_cpu_type were NULL, -cpu is completely ignored. > > But once http://patchwork.ozlabs.org/patch/870297/ is applied > it will error out the same way as if default_cpu_type were > set since vl.c won't rely on it anymore for parsing cpu_model. > > So in short it's ok to remove mc->default_cpu_type here. Thanks for the explanation. I think what I'll do is put these in as-is, and then have a followup patch that cleans up the default_cpu_types from raspi.c once that patch 870297 is in. -- PMM
Re: [Qemu-devel] [PATCH 8/9] hw/arm/bcm2836: Hardcode correct CPU type
On Thu, 15 Mar 2018 17:13:03 + Peter Maydell wrote: > On 13 March 2018 at 16:55, Andrew Baumann > wrote: > >> From: Qemu-devel >> bounces+andrew.baumann=microsoft@nongnu.org> On Behalf Of Peter > >> Maydell > >> Sent: Tuesday, 13 March 2018 08:35 > >> > >> Now we have separate types for BCM2386 and BCM2387, we might as well > >> just hard-code the CPU type they use rather than having it passed > >> through as an object property. This then lets us put the initialization > >> of the CPU object in init rather than realize. > > > What about the default_cpu_type field of MachineClass set in > > raspi[23]_machine_init? That seems irrelevant now... > > Igor, can you help with this question? For a board that always > has one CPU type (because the real hardware only ever has > one SoC, and that SoC QOM object hard codes the CPU type) > does it still need to set the mc->default_cpu_type field in > its MachineClass, or does that become pointless ? Since board ignores whatever were specified on '-cpu' and there aren't any checks in board code for current_machine->cpu_type, removing mc->default_cpu_type won't really affect anything. With current code in vl.c and with default_cpu_type set, user will get error if he specified non existing cpu_model with -cpu. If default_cpu_type were NULL, -cpu is completely ignored. But once http://patchwork.ozlabs.org/patch/870297/ is applied it will error out the same way as if default_cpu_type were set since vl.c won't rely on it anymore for parsing cpu_model. So in short it's ok to remove mc->default_cpu_type here. > > thanks > -- PMM
Re: [Qemu-devel] [PATCH 8/9] hw/arm/bcm2836: Hardcode correct CPU type
On 13 March 2018 at 23:16, Philippe Mathieu-Daudé wrote: > On 03/13/2018 06:09 PM, Peter Maydell wrote: >> On 13 March 2018 at 16:55, Andrew Baumann >> wrote: >>> At some point I remember seeing a patch to change this to cortex-a7. Is >>> there a reason we didn't make that change? >>> >>> (Background: the real Pi2 has an A7. When I first implemented the machine >>> model there was no A7 emulation in QEMU, so I used the A15 which was the >>> closest equivalent.) >> >> Yeah, we should do that. I'd forgotten about that, I think >> things just got lost in the shuffle of having several >> patchsets that tried to change the same things at once. >> >> I guess the simplest thing is to add a patch at the end of >> the series that fixes the cpu type for bcm2836. > > Peter, here is the patch Andrew remembered (maybe can be applied at the > end): > http://lists.nongnu.org/archive/html/qemu-devel/2018-02/msg04286.html Thanks for finding that. It doesn't apply after this refactoring, but I'll send out a patch which does the equivalent thing in the new code. In the meantime I'm going to apply this patchset to target-arm.next since I'm going to send out a pullreq with bugfixes for rc0 today. thanks -- PMM
Re: [Qemu-devel] [PATCH 8/9] hw/arm/bcm2836: Hardcode correct CPU type
On 13 March 2018 at 16:55, Andrew Baumann wrote: >> From: Qemu-devel > bounces+andrew.baumann=microsoft@nongnu.org> On Behalf Of Peter >> Maydell >> Sent: Tuesday, 13 March 2018 08:35 >> >> Now we have separate types for BCM2386 and BCM2387, we might as well >> just hard-code the CPU type they use rather than having it passed >> through as an object property. This then lets us put the initialization >> of the CPU object in init rather than realize. > What about the default_cpu_type field of MachineClass set in > raspi[23]_machine_init? That seems irrelevant now... Igor, can you help with this question? For a board that always has one CPU type (because the real hardware only ever has one SoC, and that SoC QOM object hard codes the CPU type) does it still need to set the mc->default_cpu_type field in its MachineClass, or does that become pointless ? thanks -- PMM
Re: [Qemu-devel] [PATCH 8/9] hw/arm/bcm2836: Hardcode correct CPU type
On 03/13/2018 06:09 PM, Peter Maydell wrote: > On 13 March 2018 at 16:55, Andrew Baumann > wrote: >>> From: Qemu-devel >> bounces+andrew.baumann=microsoft@nongnu.org> On Behalf Of Peter >>> Maydell >>> Sent: Tuesday, 13 March 2018 08:35 >>> >>> Now we have separate types for BCM2386 and BCM2387, we might as well >>> just hard-code the CPU type they use rather than having it passed >>> through as an object property. This then lets us put the initialization >>> of the CPU object in init rather than realize. >>> >>> Signed-off-by: Peter Maydell > >>> static const BCM283XInfo bcm283x_socs[] = { >>> { >>> .name = TYPE_BCM2836, >>> +.cpu_type = ARM_CPU_TYPE_NAME("cortex-a15"), >> >> At some point I remember seeing a patch to change this to cortex-a7. Is >> there a reason we didn't make that change? >> >> (Background: the real Pi2 has an A7. When I first implemented the machine >> model there was no A7 emulation in QEMU, so I used the A15 which was the >> closest equivalent.) > > Yeah, we should do that. I'd forgotten about that, I think > things just got lost in the shuffle of having several > patchsets that tried to change the same things at once. > > I guess the simplest thing is to add a patch at the end of > the series that fixes the cpu type for bcm2836. Peter, here is the patch Andrew remembered (maybe can be applied at the end): http://lists.nongnu.org/archive/html/qemu-devel/2018-02/msg04286.html > > >>> --- a/hw/arm/raspi.c >>> +++ b/hw/arm/raspi.c >>> @@ -150,8 +150,6 @@ static void raspi_init(MachineState *machine, int >>> version) >>> /* Setup the SOC */ >>> object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s->ram), >>> &error_abort); >>> -object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type", >>> -&error_abort); >>> object_property_set_int(OBJECT(&s->soc), smp_cpus, "enabled-cpus", >>> &error_abort); >>> int board_rev = version == 3 ? 0xa02082 : 0xa21041; >> >> What about the default_cpu_type field of MachineClass set in >> raspi[23]_machine_init? That seems irrelevant now... > > Mmm. It doesn't hurt anything, though. > >> Also, if anyone cares (I don't), we also just lost the ability >> to override the CPU type of a raspi model. > > Yeah, that's deliberate -- I think that letting the user randomly > plug nonexistent combinations together just confuses people when > they don't work. I guess I should call it out in the commit message > though. > > thanks > -- PMM >
Re: [Qemu-devel] [PATCH 8/9] hw/arm/bcm2836: Hardcode correct CPU type
On 03/13/2018 06:09 PM, Peter Maydell wrote: > On 13 March 2018 at 16:55, Andrew Baumann > wrote: >>> From: Qemu-devel >> bounces+andrew.baumann=microsoft@nongnu.org> On Behalf Of Peter >>> Maydell >>> Sent: Tuesday, 13 March 2018 08:35 >>> >>> Now we have separate types for BCM2386 and BCM2387, we might as well >>> just hard-code the CPU type they use rather than having it passed >>> through as an object property. This then lets us put the initialization >>> of the CPU object in init rather than realize. >>> >>> Signed-off-by: Peter Maydell > >>> static const BCM283XInfo bcm283x_socs[] = { >>> { >>> .name = TYPE_BCM2836, >>> +.cpu_type = ARM_CPU_TYPE_NAME("cortex-a15"), >> >> At some point I remember seeing a patch to change this to cortex-a7. Is >> there a reason we didn't make that change? >> >> (Background: the real Pi2 has an A7. When I first implemented the machine >> model there was no A7 emulation in QEMU, so I used the A15 which was the >> closest equivalent.) > > Yeah, we should do that. I'd forgotten about that, I think > things just got lost in the shuffle of having several > patchsets that tried to change the same things at once. > > I guess the simplest thing is to add a patch at the end of > the series that fixes the cpu type for bcm2836. Peter, here is the patch Andrew remembered (maybe can be applied at the end): http://lists.nongnu.org/archive/html/qemu-devel/2018-02/msg04286.html > > >>> --- a/hw/arm/raspi.c >>> +++ b/hw/arm/raspi.c >>> @@ -150,8 +150,6 @@ static void raspi_init(MachineState *machine, int >>> version) >>> /* Setup the SOC */ >>> object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s->ram), >>> &error_abort); >>> -object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type", >>> -&error_abort); >>> object_property_set_int(OBJECT(&s->soc), smp_cpus, "enabled-cpus", >>> &error_abort); >>> int board_rev = version == 3 ? 0xa02082 : 0xa21041; >> >> What about the default_cpu_type field of MachineClass set in >> raspi[23]_machine_init? That seems irrelevant now... > > Mmm. It doesn't hurt anything, though. > >> Also, if anyone cares (I don't), we also just lost the ability >> to override the CPU type of a raspi model. > > Yeah, that's deliberate -- I think that letting the user randomly > plug nonexistent combinations together just confuses people when > they don't work. I guess I should call it out in the commit message > though. > > thanks > -- PMM >
Re: [Qemu-devel] [PATCH 8/9] hw/arm/bcm2836: Hardcode correct CPU type
On 03/13/2018 04:34 PM, Peter Maydell wrote: > Now we have separate types for BCM2386 and BCM2387, we might as well > just hard-code the CPU type they use rather than having it passed > through as an object property. This then lets us put the initialization > of the CPU object in init rather than realize. > > Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé > --- > hw/arm/bcm2836.c | 22 +- > hw/arm/raspi.c | 2 -- > 2 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c > index 7140257c98..12f75b55a7 100644 > --- a/hw/arm/bcm2836.c > +++ b/hw/arm/bcm2836.c > @@ -25,16 +25,19 @@ > > struct BCM283XInfo { > const char *name; > +const char *cpu_type; > int clusterid; > }; > > static const BCM283XInfo bcm283x_socs[] = { > { > .name = TYPE_BCM2836, > +.cpu_type = ARM_CPU_TYPE_NAME("cortex-a15"), > .clusterid = 0xf, > }, > { > .name = TYPE_BCM2837, > +.cpu_type = ARM_CPU_TYPE_NAME("cortex-a53"), > .clusterid = 0x0, > }, > }; > @@ -42,6 +45,16 @@ static const BCM283XInfo bcm283x_socs[] = { > static void bcm2836_init(Object *obj) > { > BCM283XState *s = BCM283X(obj); > +BCM283XClass *bc = BCM283X_GET_CLASS(obj); > +const BCM283XInfo *info = bc->info; > +int n; > + > +for (n = 0; n < BCM283X_NCPUS; n++) { > +object_initialize(&s->cpus[n], sizeof(s->cpus[n]), > + info->cpu_type); > +object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]), > + &error_abort); > +} > > object_initialize(&s->control, sizeof(s->control), TYPE_BCM2836_CONTROL); > object_property_add_child(obj, "control", OBJECT(&s->control), NULL); > @@ -69,14 +82,6 @@ static void bcm2836_realize(DeviceState *dev, Error **errp) > > /* common peripherals from bcm2835 */ > > -obj = OBJECT(dev); > -for (n = 0; n < BCM283X_NCPUS; n++) { > -object_initialize(&s->cpus[n], sizeof(s->cpus[n]), > - s->cpu_type); > -object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]), > - &error_abort); > -} > - > obj = object_property_get_link(OBJECT(dev), "ram", &err); > if (obj == NULL) { > error_setg(errp, "%s: required ram link not found: %s", > @@ -168,7 +173,6 @@ static void bcm2836_realize(DeviceState *dev, Error > **errp) > } > > static Property bcm2836_props[] = { > -DEFINE_PROP_STRING("cpu-type", BCM283XState, cpu_type), > DEFINE_PROP_UINT32("enabled-cpus", BCM283XState, enabled_cpus, > BCM283X_NCPUS), > DEFINE_PROP_END_OF_LIST() > diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c > index f588720138..ae15997669 100644 > --- a/hw/arm/raspi.c > +++ b/hw/arm/raspi.c > @@ -150,8 +150,6 @@ static void raspi_init(MachineState *machine, int version) > /* Setup the SOC */ > object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s->ram), > &error_abort); > -object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type", > -&error_abort); > object_property_set_int(OBJECT(&s->soc), smp_cpus, "enabled-cpus", > &error_abort); > int board_rev = version == 3 ? 0xa02082 : 0xa21041; >
Re: [Qemu-devel] [PATCH 8/9] hw/arm/bcm2836: Hardcode correct CPU type
On 13 March 2018 at 16:55, Andrew Baumann wrote: >> From: Qemu-devel > bounces+andrew.baumann=microsoft@nongnu.org> On Behalf Of Peter >> Maydell >> Sent: Tuesday, 13 March 2018 08:35 >> >> Now we have separate types for BCM2386 and BCM2387, we might as well >> just hard-code the CPU type they use rather than having it passed >> through as an object property. This then lets us put the initialization >> of the CPU object in init rather than realize. >> >> Signed-off-by: Peter Maydell >> static const BCM283XInfo bcm283x_socs[] = { >> { >> .name = TYPE_BCM2836, >> +.cpu_type = ARM_CPU_TYPE_NAME("cortex-a15"), > > At some point I remember seeing a patch to change this to cortex-a7. Is there > a reason we didn't make that change? > > (Background: the real Pi2 has an A7. When I first implemented the machine > model there was no A7 emulation in QEMU, so I used the A15 which was the > closest equivalent.) Yeah, we should do that. I'd forgotten about that, I think things just got lost in the shuffle of having several patchsets that tried to change the same things at once. I guess the simplest thing is to add a patch at the end of the series that fixes the cpu type for bcm2836. >> --- a/hw/arm/raspi.c >> +++ b/hw/arm/raspi.c >> @@ -150,8 +150,6 @@ static void raspi_init(MachineState *machine, int >> version) >> /* Setup the SOC */ >> object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s->ram), >> &error_abort); >> -object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type", >> -&error_abort); >> object_property_set_int(OBJECT(&s->soc), smp_cpus, "enabled-cpus", >> &error_abort); >> int board_rev = version == 3 ? 0xa02082 : 0xa21041; > > What about the default_cpu_type field of MachineClass set in > raspi[23]_machine_init? That seems irrelevant now... Mmm. It doesn't hurt anything, though. > Also, if anyone cares (I don't), we also just lost the ability > to override the CPU type of a raspi model. Yeah, that's deliberate -- I think that letting the user randomly plug nonexistent combinations together just confuses people when they don't work. I guess I should call it out in the commit message though. thanks -- PMM
Re: [Qemu-devel] [PATCH 8/9] hw/arm/bcm2836: Hardcode correct CPU type
> From: Qemu-devel bounces+andrew.baumann=microsoft@nongnu.org> On Behalf Of Peter > Maydell > Sent: Tuesday, 13 March 2018 08:35 > > Now we have separate types for BCM2386 and BCM2387, we might as well > just hard-code the CPU type they use rather than having it passed > through as an object property. This then lets us put the initialization > of the CPU object in init rather than realize. > > Signed-off-by: Peter Maydell > --- > hw/arm/bcm2836.c | 22 +- > hw/arm/raspi.c | 2 -- > 2 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c > index 7140257c98..12f75b55a7 100644 > --- a/hw/arm/bcm2836.c > +++ b/hw/arm/bcm2836.c > @@ -25,16 +25,19 @@ > > struct BCM283XInfo { > const char *name; > +const char *cpu_type; > int clusterid; > }; > > static const BCM283XInfo bcm283x_socs[] = { > { > .name = TYPE_BCM2836, > +.cpu_type = ARM_CPU_TYPE_NAME("cortex-a15"), At some point I remember seeing a patch to change this to cortex-a7. Is there a reason we didn't make that change? (Background: the real Pi2 has an A7. When I first implemented the machine model there was no A7 emulation in QEMU, so I used the A15 which was the closest equivalent.) > .clusterid = 0xf, > }, > { > .name = TYPE_BCM2837, > +.cpu_type = ARM_CPU_TYPE_NAME("cortex-a53"), > .clusterid = 0x0, > }, > }; > @@ -42,6 +45,16 @@ static const BCM283XInfo bcm283x_socs[] = { > static void bcm2836_init(Object *obj) > { > BCM283XState *s = BCM283X(obj); > +BCM283XClass *bc = BCM283X_GET_CLASS(obj); > +const BCM283XInfo *info = bc->info; > +int n; > + > +for (n = 0; n < BCM283X_NCPUS; n++) { > +object_initialize(&s->cpus[n], sizeof(s->cpus[n]), > + info->cpu_type); > +object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]), > + &error_abort); > +} > > object_initialize(&s->control, sizeof(s->control), TYPE_BCM2836_CONTROL); > object_property_add_child(obj, "control", OBJECT(&s->control), NULL); > @@ -69,14 +82,6 @@ static void bcm2836_realize(DeviceState *dev, Error > **errp) > > /* common peripherals from bcm2835 */ > > -obj = OBJECT(dev); > -for (n = 0; n < BCM283X_NCPUS; n++) { > -object_initialize(&s->cpus[n], sizeof(s->cpus[n]), > - s->cpu_type); > -object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]), > - &error_abort); > -} > - > obj = object_property_get_link(OBJECT(dev), "ram", &err); > if (obj == NULL) { > error_setg(errp, "%s: required ram link not found: %s", > @@ -168,7 +173,6 @@ static void bcm2836_realize(DeviceState *dev, Error > **errp) > } > > static Property bcm2836_props[] = { > -DEFINE_PROP_STRING("cpu-type", BCM283XState, cpu_type), > DEFINE_PROP_UINT32("enabled-cpus", BCM283XState, enabled_cpus, > BCM283X_NCPUS), > DEFINE_PROP_END_OF_LIST() > diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c > index f588720138..ae15997669 100644 > --- a/hw/arm/raspi.c > +++ b/hw/arm/raspi.c > @@ -150,8 +150,6 @@ static void raspi_init(MachineState *machine, int > version) > /* Setup the SOC */ > object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s->ram), > &error_abort); > -object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type", > -&error_abort); > object_property_set_int(OBJECT(&s->soc), smp_cpus, "enabled-cpus", > &error_abort); > int board_rev = version == 3 ? 0xa02082 : 0xa21041; What about the default_cpu_type field of MachineClass set in raspi[23]_machine_init? That seems irrelevant now... Also, if anyone cares (I don't), we also just lost the ability to override the CPU type of a raspi model. Otherwise, Reviewed-by: Andrew Baumann Cheers, Andrew
[Qemu-devel] [PATCH 8/9] hw/arm/bcm2836: Hardcode correct CPU type
Now we have separate types for BCM2386 and BCM2387, we might as well just hard-code the CPU type they use rather than having it passed through as an object property. This then lets us put the initialization of the CPU object in init rather than realize. Signed-off-by: Peter Maydell --- hw/arm/bcm2836.c | 22 +- hw/arm/raspi.c | 2 -- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c index 7140257c98..12f75b55a7 100644 --- a/hw/arm/bcm2836.c +++ b/hw/arm/bcm2836.c @@ -25,16 +25,19 @@ struct BCM283XInfo { const char *name; +const char *cpu_type; int clusterid; }; static const BCM283XInfo bcm283x_socs[] = { { .name = TYPE_BCM2836, +.cpu_type = ARM_CPU_TYPE_NAME("cortex-a15"), .clusterid = 0xf, }, { .name = TYPE_BCM2837, +.cpu_type = ARM_CPU_TYPE_NAME("cortex-a53"), .clusterid = 0x0, }, }; @@ -42,6 +45,16 @@ static const BCM283XInfo bcm283x_socs[] = { static void bcm2836_init(Object *obj) { BCM283XState *s = BCM283X(obj); +BCM283XClass *bc = BCM283X_GET_CLASS(obj); +const BCM283XInfo *info = bc->info; +int n; + +for (n = 0; n < BCM283X_NCPUS; n++) { +object_initialize(&s->cpus[n], sizeof(s->cpus[n]), + info->cpu_type); +object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]), + &error_abort); +} object_initialize(&s->control, sizeof(s->control), TYPE_BCM2836_CONTROL); object_property_add_child(obj, "control", OBJECT(&s->control), NULL); @@ -69,14 +82,6 @@ static void bcm2836_realize(DeviceState *dev, Error **errp) /* common peripherals from bcm2835 */ -obj = OBJECT(dev); -for (n = 0; n < BCM283X_NCPUS; n++) { -object_initialize(&s->cpus[n], sizeof(s->cpus[n]), - s->cpu_type); -object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]), - &error_abort); -} - obj = object_property_get_link(OBJECT(dev), "ram", &err); if (obj == NULL) { error_setg(errp, "%s: required ram link not found: %s", @@ -168,7 +173,6 @@ static void bcm2836_realize(DeviceState *dev, Error **errp) } static Property bcm2836_props[] = { -DEFINE_PROP_STRING("cpu-type", BCM283XState, cpu_type), DEFINE_PROP_UINT32("enabled-cpus", BCM283XState, enabled_cpus, BCM283X_NCPUS), DEFINE_PROP_END_OF_LIST() diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c index f588720138..ae15997669 100644 --- a/hw/arm/raspi.c +++ b/hw/arm/raspi.c @@ -150,8 +150,6 @@ static void raspi_init(MachineState *machine, int version) /* Setup the SOC */ object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s->ram), &error_abort); -object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type", -&error_abort); object_property_set_int(OBJECT(&s->soc), smp_cpus, "enabled-cpus", &error_abort); int board_rev = version == 3 ? 0xa02082 : 0xa21041; -- 2.16.2