On Tue, Sep 12, 2017 at 02:11:59PM +0200, Igor Mammedov wrote:
> On Tue, 5 Sep 2017 18:31:52 -0300
> Eduardo Habkost <ehabk...@redhat.com> wrote:
>
> > On Mon, Sep 04, 2017 at 04:01:02PM +0200, Igor Mammedov wrote:
> > > there are 2 use cases to deal with:
> > > 1: fixed CPU models per board/soc
> > > 2: boards with user configurable cpu_model and fallback to
> > > default cpu_model if user hasn't specified one explicitly
> > >
> > > For the 1st
> > > drop intermediate cpu_model parsing and use const cpu type
> > > directly, which replaces:
> > > typename = object_class_get_name(
> > > cpu_class_by_name(TYPE_ARM_CPU, cpu_model))
> > > object_new(typename)
> > > with
> > > object_new(FOO_CPU_TYPE_NAME)
> > > or
> > > cpu_generic_init(BASE_CPU_TYPE, "my cpu model")
> > > with
> > > cpu_create(FOO_CPU_TYPE_NAME)
> > >
> > > as result 1st use case doesn't have to invoke not necessary
> > > translation and not needed code is removed.
> > >
> > > For the 2nd
> > > 1: set default cpu type with MachineClass::default_cpu_type and
> > > 2: use generic cpu_model parsing that done before machine_init()
> > > is run and:
> > > 2.1: drop custom cpu_model parsing where pattern is:
> > > typename = object_class_get_name(
> > > cpu_class_by_name(TYPE_ARM_CPU, cpu_model))
> > > [parse_features(typename, cpu_model, &err) ]
> > >
> > > 2.2: or replace cpu_generic_init() which does what
> > > 2.1 does + create_cpu(typename) with just
> > > create_cpu(machine->cpu_type)
> > > as result cpu_name -> cpu_type translation is done using
> > > generic machine code one including parsing optional features
> > > if supported/present (removes a bunch of duplicated cpu_model
> > > parsing code) and default cpu type is defined in an uniform way
> > > within machine_class_init callbacks instead of adhoc places
> > > in boadr's machine_init code.
> > >
> > > Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> > > ---
> [...]
>
> > > @@ -285,20 +259,16 @@ static void armv7m_reset(void *opaque)
> > > Returns the ARMv7M device. */
> > >
> > > DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int
> > > num_irq,
> > > - const char *kernel_filename, const char *cpu_model)
> > > + const char *kernel_filename, const char *cpu_type)
> > > {
> > > DeviceState *armv7m;
> > >
> > > - if (cpu_model == NULL) {
> > > - cpu_model = "cortex-m3";
> > > - }
> > > -
> >
> > I was going to suggest doing the default_cpu_type stuff in a
> > separate patch, but it might require touching those lines twice.
> > So I guess this is OK.
> I've have tried it, but yes it's more changes and there is also
> chicken/egg problem, cleanest way I've stopped at is to get rid of
> cpu_model fallback + cpu_generic_init() in one go.
OK
>
>
> > > armv7m = qdev_create(NULL, "armv7m");
> > > qdev_prop_set_uint32(armv7m, "num-irq", num_irq);
> > > - qdev_prop_set_string(armv7m, "cpu-model", cpu_model);
> > > + qdev_prop_set_string(armv7m, "cpu-type", cpu_type);
> > > object_property_set_link(OBJECT(armv7m), OBJECT(get_system_memory()),
> > > "memory", &error_abort);
> > > - /* This will exit with an error if the user passed us a bad
> > > cpu_model */
> > > + /* This will exit with an error if the user passed us a bad cpu_type
> > > */
> > > qdev_init_nofail(armv7m);
> > >
> > > armv7m_load_kernel(ARM_CPU(first_cpu), kernel_filename, mem_size);
> [...]
>
> > > diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> > > index 20e60f1..0d7190a 100644
> > > --- a/hw/arm/highbank.c
> > > +++ b/hw/arm/highbank.c
> > > @@ -219,7 +219,6 @@ enum cxmachines {
> > > static void calxeda_init(MachineState *machine, enum cxmachines
> > > machine_id)
> > > {
> > > ram_addr_t ram_size = machine->ram_size;
> > > - const char *cpu_model = machine->cpu_model;
> > > const char *kernel_filename = machine->kernel_filename;
> > > const char *kernel_cmdline = machine->kernel_cmdline;
> > > const char *initrd_filename = machine->initrd_filename;
> > > @@ -236,19 +235,20 @@ static void calxeda_init(MachineState *machine,
> > > enum cxmachines machine_id)
> > >
> > > switch (machine_id) {
> > > case CALXEDA_HIGHBANK:
> > > - cpu_model = "cortex-a9";
> > > + machine->cpu_type = ARM_CPU_TYPE_NAME("cortex-a9");
> > > break;
> > > case CALXEDA_MIDWAY:
> > > - cpu_model = "cortex-a15";
> > > + machine->cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
> > > break;
> > > + default:
> > > + assert(0);
> > > }
> >
> > Why not delete this switch statement completely and set
> > default_cpu_type at midway_class_init() and
> > highbank_class_init()?
> it would allow '-cpu foo' to take effect which isn't what current code does,
> as series doesn't add valid_cpus[] check at the same time.
Oh, I see.
>
> So here we do pretty much strait-forward conversion from cpu_model
> to cpu_type and nothing else.
OK.
>
> >
> >
> > >
> > > for (n = 0; n < smp_cpus; n++) {
> > > - ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model);
> > > Object *cpuobj;
> > > ARMCPU *cpu;
> > >
> > > - cpuobj = object_new(object_class_get_name(oc));
> > > + cpuobj = object_new(machine->cpu_type);
> > > cpu = ARM_CPU(cpuobj);
> > >
> > > object_property_set_int(cpuobj, QEMU_PSCI_CONDUIT_SMC,
> [...]
>
> > > diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> > > index c16657d..79b317a 100644
> > > --- a/hw/arm/pxa2xx.c
> > > +++ b/hw/arm/pxa2xx.c
> > > @@ -2052,21 +2052,19 @@ static void pxa2xx_reset(void *opaque, int line,
> > > int level)
> > >
> > > /* Initialise a PXA270 integrated chip (ARM based core). */
> > > PXA2xxState *pxa270_init(MemoryRegion *address_space,
> > > - unsigned int sdram_size, const char *revision)
> > > + unsigned int sdram_size, const char *cpu_type)
> > > {
> > > PXA2xxState *s;
> > > int i;
> > > DriveInfo *dinfo;
> > > s = g_new0(PXA2xxState, 1);
> > >
> > > - if (revision && strncmp(revision, "pxa27", 5)) {
> > > + if (strncmp(cpu_type, ARM_CPU_TYPE_NAME("pxa27"), 5)) {
> >
> > Why are you using ARM_CPU_TYPE_NAME here, if you are only
> > checking if cpu_type starts with "pxa27"?
> mainly to show that we are dealing with types here,
> I can leave plain "pxa27" if you prefer.
Considering that we're dealing with a strncmp() hack that needs
ARM_CPU_TYPE_NAME to work in a very specific way, I would prefer
to leave the actual argument to strncmp() explicitly visible,
until we implement another solution.
>
>
> > I suggest adding a TODO here noting that we implement this using
> > either a TYPE_ARM_PXA27 subclass (so we can use
> > object_class_dynamic_cast()), or a ARMCPUClass field to identify
> > if the CPU is pxa27.
> dynamic cast would be nice, but ("pxa27", 5) cover a range of cpu types
> starting with this prefix, to use cast class structure should be reorganized
> to make all pxa27 based cpus have a common pxa27 ancestor.
> Or just use more verbose complete list of valid_cpus[]
>
> It's out of scope of this patch including TODO comment,
> all boards should be audited anyways to make proper use of valid_cpus[]
> and adding not related TODO comments here doesn't seem right.
True, now we have the plan of adding valid_cpus[], and this would
affect lots of other boards. I won't mind if you prefer to not
add the TODO comment on this series.
>
>
> > > fprintf(stderr, "Machine requires a PXA27x processor.\n");
> > > exit(1);
> > > }
> >
> > This would be another use case for a generic CPU model validation
> > mechanism in MachineClass.
> yep, just applied to a range of cpus instead of typical leaf class.
>
> [...]
>
> > > diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
> > > index c1145dd..3d1a231 100644
> > > --- a/hw/arm/strongarm.c
> > > +++ b/hw/arm/strongarm.c
> > > @@ -1581,23 +1581,19 @@ static const TypeInfo strongarm_ssp_info = {
> > >
> > > /* Main CPU functions */
> > > StrongARMState *sa1110_init(MemoryRegion *sysmem,
> > > - unsigned int sdram_size, const char *rev)
> > > + unsigned int sdram_size, const char
> > > *cpu_type)
> > > {
> > > StrongARMState *s;
> > > int i;
> > >
> > > s = g_new0(StrongARMState, 1);
> > >
> > > - if (!rev) {
> > > - rev = "sa1110-b5";
> > > - }
> > > -
> > > - if (strncmp(rev, "sa1110", 6)) {
> > > + if (strncmp(cpu_type, "sa1110", 6)) {
> > > error_report("Machine requires a SA1110 processor.");
> > > exit(1);
> >
> > Same suggestion as on pxa270_init(): adding a TODO here noting
> > that we implement this using either object_class_dynamic_cast(),
> > or a ARMCPUClass field to identify if the CPU is sa1110.
> same as pxa27, it's a case of range of cpus,
> TODO is orthogonal to patch topic, so I'd prefer not to add it here.
OK
>
> >
> >
> > > }
> > >
> > > - s->cpu = ARM_CPU(cpu_generic_init(TYPE_ARM_CPU, rev));
> > > + s->cpu = ARM_CPU(cpu_create(cpu_type));
> > >
> > > memory_region_allocate_system_memory(&s->sdram, NULL,
> > > "strongarm.sdram",
> > > sdram_size);
> > > diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
> > > index 8b757ff..75631f6 100644
> > > --- a/hw/arm/tosa.c
> > > +++ b/hw/arm/tosa.c
> > > @@ -219,7 +219,6 @@ static struct arm_boot_info tosa_binfo = {
> > >
> > > static void tosa_init(MachineState *machine)
> > > {
> > > - const char *cpu_model = machine->cpu_model;
> > > const char *kernel_filename = machine->kernel_filename;
> > > const char *kernel_cmdline = machine->kernel_cmdline;
> > > const char *initrd_filename = machine->initrd_filename;
> > > @@ -229,9 +228,6 @@ static void tosa_init(MachineState *machine)
> > > TC6393xbState *tmio;
> > > DeviceState *scp0, *scp1;
> > >
> > > - if (!cpu_model)
> > > - cpu_model = "pxa255";
> > > -
> >
> > Don't we need to set mc->default_cpu_type at
> > tosapda_machine_init() to replace this?
> board doesn't actually take user input and above remove code does nothing,
> look into pxa255_init() where it uses hardcoded cpu model
> cpu_generic_init(TYPE_ARM_CPU, "pxa255")
>
> another user connex_init() of pxa255_init() were already cleaned up
> or didn't have junk to begin with
You are right, I didn't notice cpu_model was an unused variable.
>
> > > mpu = pxa255_init(address_space_mem, tosa_binfo.ram_size);
> > >
> > > memory_region_init_ram(rom, NULL, "tosa.rom", TOSA_ROM,
> > > &error_fatal);
> [...]
>
> > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > > index 05c038b..feeeeb2 100644
> > > --- a/target/arm/cpu.c
> > > +++ b/target/arm/cpu.c
> > > @@ -867,7 +867,7 @@ static ObjectClass *arm_cpu_class_by_name(const char
> > > *cpu_model)
> > > }
> > >
> > > cpuname = g_strsplit(cpu_model, ",", 1);
> > > - typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpuname[0]);
> > > + typename = g_strdup_printf("%s" ARM_CPU_TYPE_SUFFIX, cpuname[0]);
> >
> > What about doing the same we do in x86 and s390:
> >
> > g_strdup_printf(ARM_CPU_TYPE_NAME("%s"), cpuname[0]);
> sure
>
I think this and the strncmp() line are the only suggestions that
I'm still keeping. But they shouldn't block the series, so:
Reviewed-by: Eduardo Habkost <ehabk...@redhat.com>
--
Eduardo
