| Message ID | 1516203816-19374-1-git-send-email-imammedo@redhat.com |
|---|---|
| Headers | show |
| Series | generalize parsing of cpu_model (part 4) | expand |
On 17 January 2018 at 15:43, Igor Mammedov <imammedo@redhat.com> wrote: > Series is finishing work on generalizing cpu_model parsing > and limiting parts that deal with inconsistent cpu_model > naming to "-cpu" CLI option processing in vl.c/*-user.main.c > and FOO_cpu_class_by_name() callbacks. > > It introduces TARGET_DEFAULT_CPU_TYPE which must be defined > by each target and is used setting default cpu type for > linux/bsd-user targets and as anchor point to pick cpu class > that provides target specific FOO_cpu_class_by_name() > callback for cpu_parse_cpu_model() in null-machine.c > which is compiled for all targets that have system > mode emulation. I like moving this from being an ifdef ladder into per-cpu code, but I don't think the definition belongs in target/$ARCH. It's part of the choice usermode makes about how to handle binaries it's loading, so it should go in linux-user/$ARCH/target_cpu.h. target/$ARCH should really be for things that are properties of the architecture. thanks -- PMM
On Wed, 17 Jan 2018 16:12:09 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On 17 January 2018 at 15:43, Igor Mammedov <imammedo@redhat.com> wrote: > > Series is finishing work on generalizing cpu_model parsing > > and limiting parts that deal with inconsistent cpu_model > > naming to "-cpu" CLI option processing in vl.c/*-user.main.c > > and FOO_cpu_class_by_name() callbacks. > > > > It introduces TARGET_DEFAULT_CPU_TYPE which must be defined > > by each target and is used setting default cpu type for > > linux/bsd-user targets and as anchor point to pick cpu class > > that provides target specific FOO_cpu_class_by_name() > > callback for cpu_parse_cpu_model() in null-machine.c > > which is compiled for all targets that have system > > mode emulation. > > I like moving this from being an ifdef ladder into per-cpu > code, but I don't think the definition belongs in target/$ARCH. > It's part of the choice usermode makes about how to handle > binaries it's loading, so it should go in linux-user/$ARCH/target_cpu.h. > target/$ARCH should really be for things that are properties > of the architecture. That's used not only by linux-user but also reused by null-machine.c to get access to a target specific cpu_class_by_name() callback. I admit that it's a convoluted API i.e. for cpu_parse_cpu_model() to require a target specific CPU type to resolve cpu_model name, but that's what we ended up with and have now. It seemed logical to me to put YET_ANOTHER_CPU_TYPE to target/$ARCH/cpu.h along with other target specific CPU type macros'. Main goal of this series is not TARGET_DEFAULT_CPU_TYPE and its abuse by null-machine.c, but rather getting rid of cpu_model handling across whole tree (which is easy to misuse due to existing APIs and its general availability) and limiting cpu_model translation to option parsing+target specific cpu_class_by_name() callbacks. We can build on top of that linux-user specific extension to pick CPU type based on ELF notes, the difference would be that it will work with cpu types and not with cpu_model as it were implemented in: [PATCH v3 0/4] linux-user: select CPU type according ELF header values > thanks > -- PMM
On 17 January 2018 at 19:15, Igor Mammedov <imammedo@redhat.com> wrote: > On Wed, 17 Jan 2018 16:12:09 +0000 > Peter Maydell <peter.maydell@linaro.org> wrote: >> I like moving this from being an ifdef ladder into per-cpu >> code, but I don't think the definition belongs in target/$ARCH. >> It's part of the choice usermode makes about how to handle >> binaries it's loading, so it should go in linux-user/$ARCH/target_cpu.h. >> target/$ARCH should really be for things that are properties >> of the architecture. > That's used not only by linux-user but also reused by null-machine.c > to get access to a target specific cpu_class_by_name() callback. That usage must want a different name, though, surely? For Arm the default CPU for linux-user is 'any' but that is usermode only and won't work for system emulation so null-machine.c will need to pick something else. thanks -- PMM
On Wed, 17 Jan 2018 20:30:14 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On 17 January 2018 at 19:15, Igor Mammedov <imammedo@redhat.com> wrote: > > On Wed, 17 Jan 2018 16:12:09 +0000 > > Peter Maydell <peter.maydell@linaro.org> wrote: > >> I like moving this from being an ifdef ladder into per-cpu > >> code, but I don't think the definition belongs in target/$ARCH. > >> It's part of the choice usermode makes about how to handle > >> binaries it's loading, so it should go in linux-user/$ARCH/target_cpu.h. > >> target/$ARCH should really be for things that are properties > >> of the architecture. > > That's used not only by linux-user but also reused by null-machine.c > > to get access to a target specific cpu_class_by_name() callback. > > That usage must want a different name, though, surely? > For Arm the default CPU for linux-user is 'any' but that > is usermode only and won't work for system emulation so > null-machine.c will need to pick something else. not really in general as boards set their own default types and secondly it applies only to null-machine. Though in both cases it work the same just fine because current API works like this (system emulation) vl.c: current_machine->cpu_type = machine_class->default_cpu_type; if (cpu_model) { current_machine->cpu_type = cpu_parse_cpu_model(machine_class->default_cpu_type, cpu_model); ... } which would result for null-machine (patch 20/24) in: cpu_parse_cpu_model(TARGET_DEFAULT_CPU_TYPE, cpu_model): oc = cpu_class_by_name(TARGET_DEFAULT_CPU_TYPE, cpu_model): cc = CPU_CLASS(object_class_by_name(TARGET_DEFAULT_CPU_TYPE)) cc->class_by_name(cpu_model) so it doesn't really matter for system emulation what exact type is used as a proxy to reach callback, as each target implements only one cb in base CPU class and any leaf class will work fine to reach the same class_by_name() cb. So type that is default for linux-user can be abused for this purpose and could be used as linux-default at the same time. Ugly and hackish, yes. But it isolates cpu_model handling to a few places and series removes API that uses it, so we won't have to watch out for patches that would bring cpu_model back into boards code after that. On top of that, we could work on making cpu_parse_cpu_model() API not to require proxy cpu type and that would affect only 3 remaining callers in vl.c,bsd/linux-user/main.c But that another re-factoring beyond the scope of this series, which is already big as it is. > thanks > -- PMM
On 18 January 2018 at 10:43, Igor Mammedov <imammedo@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> wrote: >> That usage must want a different name, though, surely? >> For Arm the default CPU for linux-user is 'any' but that >> is usermode only and won't work for system emulation so >> null-machine.c will need to pick something else. > not really in general as boards set their own default types > and secondly it applies only to null-machine. > Though in both cases it work the same just fine because > current API works like this (system emulation) > vl.c: > current_machine->cpu_type = machine_class->default_cpu_type; > if (cpu_model) { > current_machine->cpu_type = > cpu_parse_cpu_model(machine_class->default_cpu_type, cpu_model); > ... > } > > which would result for null-machine (patch 20/24) in: > > cpu_parse_cpu_model(TARGET_DEFAULT_CPU_TYPE, cpu_model): > oc = cpu_class_by_name(TARGET_DEFAULT_CPU_TYPE, cpu_model): > cc = CPU_CLASS(object_class_by_name(TARGET_DEFAULT_CPU_TYPE)) > cc->class_by_name(cpu_model) In system emulation we don't define the "any" cpu type at all, so I would expect cpu_class_by_name() to always return an error here. thanks -- PMM
On Thu, 18 Jan 2018 10:50:19 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On 18 January 2018 at 10:43, Igor Mammedov <imammedo@redhat.com> wrote: > > Peter Maydell <peter.maydell@linaro.org> wrote: > >> That usage must want a different name, though, surely? > >> For Arm the default CPU for linux-user is 'any' but that > >> is usermode only and won't work for system emulation so > >> null-machine.c will need to pick something else. > > > not really in general as boards set their own default types > > and secondly it applies only to null-machine. > > Though in both cases it work the same just fine because > > current API works like this (system emulation) > > vl.c: > > current_machine->cpu_type = machine_class->default_cpu_type; > > if (cpu_model) { > > current_machine->cpu_type = > > cpu_parse_cpu_model(machine_class->default_cpu_type, cpu_model); > > ... > > } > > > > which would result for null-machine (patch 20/24) in: > > > > cpu_parse_cpu_model(TARGET_DEFAULT_CPU_TYPE, cpu_model): > > oc = cpu_class_by_name(TARGET_DEFAULT_CPU_TYPE, cpu_model): > > cc = CPU_CLASS(object_class_by_name(TARGET_DEFAULT_CPU_TYPE)) > > cc->class_by_name(cpu_model) > > In system emulation we don't define the "any" cpu type > at all, so I would expect cpu_class_by_name() to always > return an error here. My bad, 'make check' was fine but it looks like we don't test null-machine with -cpu foo, I should add a patch for that along with series on respin. I've looked and such case is rather an exception, I can fix it up in 2 ways: 1st: target/arm/cpu.h +#if !defined(CONFIG_USER_ONLY) +#define TARGET_DEFAULT_CPU_TYPE TYPE_ARM_CPU +else +#define TARGET_DEFAULT_CPU_TYPE ARM_CPU_TYPE_NAME("any") +#endif or 2nd is to compile in "any" type in system mode (which most targets do), roughly it would amount to: target/arm/cpu.c @@ -1671,10 +1671,8 @@ static const ARMCPUInfo arm_cpus[] = { { .name = "pxa270-b1", .initfn = pxa270b1_initfn }, { .name = "pxa270-c0", .initfn = pxa270c0_initfn }, { .name = "pxa270-c5", .initfn = pxa270c5_initfn }, -#ifdef CONFIG_USER_ONLY { .name = "any", .initfn = arm_any_initfn }, #endif -#endif { .name = NULL } }; and it would allow us to drop/cleanup more ifdefs in target/arm/cpu.c I'd prefer 2nd approach, so code would be more consistent with other targets and as benefit with less ifdefs. > thanks > -- PMM
On 18 January 2018 at 13:06, Igor Mammedov <imammedo@redhat.com> wrote: > I've looked and such case is rather an exception, > I can fix it up in 2 ways: > 1st: > target/arm/cpu.h > +#if !defined(CONFIG_USER_ONLY) > +#define TARGET_DEFAULT_CPU_TYPE TYPE_ARM_CPU > +else > +#define TARGET_DEFAULT_CPU_TYPE ARM_CPU_TYPE_NAME("any") > +#endif This is weird, because TYPE_ARM_CPU isn't really a sensible thing to use for anything, so you've really set it up as a "this is only of any use for null-machine.c", in which case you should just do that in null-machine.c. > or 2nd is to compile in "any" type in system mode > (which most targets do), roughly it would amount to: > target/arm/cpu.c > @@ -1671,10 +1671,8 @@ static const ARMCPUInfo arm_cpus[] = { > { .name = "pxa270-b1", .initfn = pxa270b1_initfn }, > { .name = "pxa270-c0", .initfn = pxa270c0_initfn }, > { .name = "pxa270-c5", .initfn = pxa270c5_initfn }, > -#ifdef CONFIG_USER_ONLY > { .name = "any", .initfn = arm_any_initfn }, > #endif > -#endif > { .name = NULL } > }; This is definitely wrong. We deliberately don't provide "any" in system mode, because it's not a sensible thing for users to try to use with board emulation. We disabled it some while back to avoid users trying it by accident and getting confused. In general, for Arm you really need to know which CPU you want to use and why. So: linux-user and bsd-user: should use "any" board models: should use whatever CPU that board is designed for null-machine: if it genuinely doesn't care, then pick one at random But there is no single sensible "default CPU type" at an architecture level, which is why you can't define a TARGET_DEFAULT_CPU_TYPE in target/arm/cpu.h. Any code that thinks it wants that should instead be defining it own default that makes sense for that context. thanks -- PMM
On Thu, 18 Jan 2018 13:10:13 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On 18 January 2018 at 13:06, Igor Mammedov <imammedo@redhat.com> wrote: > > I've looked and such case is rather an exception, > > I can fix it up in 2 ways: > > 1st: > > target/arm/cpu.h > > +#if !defined(CONFIG_USER_ONLY) > > +#define TARGET_DEFAULT_CPU_TYPE TYPE_ARM_CPU > > +else > > +#define TARGET_DEFAULT_CPU_TYPE ARM_CPU_TYPE_NAME("any") > > +#endif > > This is weird, because TYPE_ARM_CPU isn't really > a sensible thing to use for anything, so you've really set > it up as a "this is only of any use for null-machine.c", > in which case you should just do that in null-machine.c. yep, that would be only for null-machine.c use as proxy type, however null-machine.c is build for every target so this proxy type can't be defined null-machine.c unless we resort to ifdef ladder there. How about adding to each $ARCH/cpu.h a null-machine dedicated define: #define CPU_RESOLVING_TYPE TYPE_FOO_CPU using that in null machine and renaming TARGET_DEFAULT_CPU_TYPE to USERONLY_DEFAULT_CPU_TYPE but I'd still keep it within $ARCH/cpu.h so we won't have to create a bunch of new linux-user/$ARCH/target_elf.h files just for that and duplicate it to bsd-user/$ARCH/target_elf.h > thanks > -- PMM
On 18 January 2018 at 13:34, Igor Mammedov <imammedo@redhat.com> wrote: > and renaming > > TARGET_DEFAULT_CPU_TYPE to USERONLY_DEFAULT_CPU_TYPE > > but I'd still keep it within $ARCH/cpu.h so we won't > have to create a bunch of new linux-user/$ARCH/target_elf.h > files just for that and duplicate it to bsd-user/$ARCH/target_elf.h We already have linux-user/$ARCH/target_cpu.h, which is exactly for this kind of define. thanks -- PMM
On Thu, 18 Jan 2018 13:36:40 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On 18 January 2018 at 13:34, Igor Mammedov <imammedo@redhat.com> wrote: > > and renaming > > > > TARGET_DEFAULT_CPU_TYPE to USERONLY_DEFAULT_CPU_TYPE > > > > but I'd still keep it within $ARCH/cpu.h so we won't > > have to create a bunch of new linux-user/$ARCH/target_elf.h > > files just for that and duplicate it to bsd-user/$ARCH/target_elf.h > > We already have linux-user/$ARCH/target_cpu.h, which is exactly > for this kind of define. we don't have it for bsd-user though and it would be code duplication to add such. Using $ARCH/cpu.h seems to be a better fit for sharing default across liux/bsd-user. > thanks > -- PMM
On 18 January 2018 at 13:45, Igor Mammedov <imammedo@redhat.com> wrote: > On Thu, 18 Jan 2018 13:36:40 +0000 > Peter Maydell <peter.maydell@linaro.org> wrote: > >> On 18 January 2018 at 13:34, Igor Mammedov <imammedo@redhat.com> wrote: >> > and renaming >> > >> > TARGET_DEFAULT_CPU_TYPE to USERONLY_DEFAULT_CPU_TYPE >> > >> > but I'd still keep it within $ARCH/cpu.h so we won't >> > have to create a bunch of new linux-user/$ARCH/target_elf.h >> > files just for that and duplicate it to bsd-user/$ARCH/target_elf.h >> >> We already have linux-user/$ARCH/target_cpu.h, which is exactly >> for this kind of define. > we don't have it for bsd-user though and it would be > code duplication to add such. > Using $ARCH/cpu.h seems to be a better fit for sharing > default across liux/bsd-user. Yes, bsd-user is a bit unmaintained and behind linux-user in its structuring. I don't mind bsd-user being a bit of a mess, but I don't want problems in bsd-user to cause us to put code where it shouldn't be in other parts of the codebase. (It's not inherently the case that "best CPU choice for Linux user" is the same as "best CPU choice for bsd-user" either.) thanks -- PMM
On Thu, 18 Jan 2018 13:49:25 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On 18 January 2018 at 13:45, Igor Mammedov <imammedo@redhat.com> wrote: > > On Thu, 18 Jan 2018 13:36:40 +0000 > > Peter Maydell <peter.maydell@linaro.org> wrote: > > > >> On 18 January 2018 at 13:34, Igor Mammedov <imammedo@redhat.com> wrote: > >> > and renaming > >> > > >> > TARGET_DEFAULT_CPU_TYPE to USERONLY_DEFAULT_CPU_TYPE > >> > > >> > but I'd still keep it within $ARCH/cpu.h so we won't > >> > have to create a bunch of new linux-user/$ARCH/target_elf.h > >> > files just for that and duplicate it to bsd-user/$ARCH/target_elf.h > >> > >> We already have linux-user/$ARCH/target_cpu.h, which is exactly > >> for this kind of define. > > we don't have it for bsd-user though and it would be > > code duplication to add such. > > Using $ARCH/cpu.h seems to be a better fit for sharing > > default across liux/bsd-user. > > Yes, bsd-user is a bit unmaintained and behind linux-user in > its structuring. I don't mind bsd-user being a bit of a mess, > but I don't want problems in bsd-user to cause us to put code > where it shouldn't be in other parts of the codebase. (It's > not inherently the case that "best CPU choice for Linux user" > is the same as "best CPU choice for bsd-user" either.) Ok, if there isn't objections wrt above mentioned code duplication in *-user, I surely can implement it as far as I can remove cpu_model along with it. > > thanks > -- PMM
(CC'ing linux-user maintainers) Hi Peter, Igor, On 01/18/2018 10:10 AM, Peter Maydell wrote: > On 18 January 2018 at 13:06, Igor Mammedov <imammedo@redhat.com> wrote: >> I've looked and such case is rather an exception, >> I can fix it up in 2 ways: >> 1st: >> target/arm/cpu.h >> +#if !defined(CONFIG_USER_ONLY) >> +#define TARGET_DEFAULT_CPU_TYPE TYPE_ARM_CPU >> +else >> +#define TARGET_DEFAULT_CPU_TYPE ARM_CPU_TYPE_NAME("any") >> +#endif > > This is weird, because TYPE_ARM_CPU isn't really > a sensible thing to use for anything, so you've really set > it up as a "this is only of any use for null-machine.c", > in which case you should just do that in null-machine.c. > >> or 2nd is to compile in "any" type in system mode >> (which most targets do), roughly it would amount to: >> target/arm/cpu.c >> @@ -1671,10 +1671,8 @@ static const ARMCPUInfo arm_cpus[] = { >> { .name = "pxa270-b1", .initfn = pxa270b1_initfn }, >> { .name = "pxa270-c0", .initfn = pxa270c0_initfn }, >> { .name = "pxa270-c5", .initfn = pxa270c5_initfn }, >> -#ifdef CONFIG_USER_ONLY >> { .name = "any", .initfn = arm_any_initfn }, >> #endif >> -#endif >> { .name = NULL } >> }; > > This is definitely wrong. We deliberately don't provide "any" > in system mode, because it's not a sensible thing for users > to try to use with board emulation. We disabled it some while > back to avoid users trying it by accident and getting confused. > > In general, for Arm you really need to know which CPU you want > to use and why. So: > linux-user and bsd-user: should use "any" I disagree on this, since userland binaries are compiled for a specific arch/ABI/FPU. Even without worrying about the FPU, it is unlikely the "any" cpu can run indifferently ARMv6 and ARMv7 binaries. IMHO ARMv5 should default to arm946, ARMv6 arm1176 and ARMv7 to cortex-a7. I think we should do the same for linux-user than system and remove the 'any' cpu for ARM. > board models: should use whatever CPU that board is designed for > null-machine: if it genuinely doesn't care, then pick one at random Should work :) Maybe pick the latest/best implemented, hoping this has more features to cover? > But there is no single sensible "default CPU type" at an architecture > level, which is why you can't define a TARGET_DEFAULT_CPU_TYPE > in target/arm/cpu.h. Any code that thinks it wants that should > instead be defining it own default that makes sense for that > context. > > thanks > -- PMM >
On 18 January 2018 at 15:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > I disagree on this, since userland binaries are compiled for a specific > arch/ABI/FPU. > Even without worrying about the FPU, it is unlikely the "any" cpu can > run indifferently ARMv6 and ARMv7 binaries. In practice this works fine. Generally for userspace the Arm architecture retains backwards compatibility. (This is not the case for kernel mode, obviously.) thanks -- PMM