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.
> > 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.
So here we do pretty much strait-forward conversion from cpu_model
to cpu_type and nothing else.
>
>
> >
> > 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.
> 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.
> > 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.
>
>
> > }
> >
> > - 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
> > 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
>
> > oc = object_class_by_name(typename);
> > g_strfreev(cpuname);
> > g_free(typename);
> > --
> > 2.7.4
> >
> >
>
>
