Re: [Qemu-devel] [PATCH 00/24] generalize parsing of cpu_model (part 4)

[Peter Maydell]

On 18 January 2018 at 15:31, Philippe Mathieu-Daudé  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

Re: [Qemu-devel] [PATCH 00/24] generalize parsing of cpu_model (part 4)

[Philippe Mathieu-Daudé]

(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  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
> 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 00/24] generalize parsing of cpu_model (part 4)

[Igor Mammedov]

On Thu, 18 Jan 2018 13:49:25 +
Peter Maydell  wrote:

> On 18 January 2018 at 13:45, Igor Mammedov  wrote:
> > On Thu, 18 Jan 2018 13:36:40 +
> > Peter Maydell  wrote:
> >  
> >> On 18 January 2018 at 13:34, Igor Mammedov  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

Re: [Qemu-devel] [PATCH 00/24] generalize parsing of cpu_model (part 4)

[Peter Maydell]

On 18 January 2018 at 13:45, Igor Mammedov  wrote:
> On Thu, 18 Jan 2018 13:36:40 +
> Peter Maydell  wrote:
>
>> On 18 January 2018 at 13:34, Igor Mammedov  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

Re: [Qemu-devel] [PATCH 00/24] generalize parsing of cpu_model (part 4)

[Igor Mammedov]

On Thu, 18 Jan 2018 13:36:40 +
Peter Maydell  wrote:

> On 18 January 2018 at 13:34, Igor Mammedov  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

Re: [Qemu-devel] [PATCH 00/24] generalize parsing of cpu_model (part 4)

[Peter Maydell]

On 18 January 2018 at 13:34, Igor Mammedov  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

Re: [Qemu-devel] [PATCH 00/24] generalize parsing of cpu_model (part 4)

[Igor Mammedov]

On Thu, 18 Jan 2018 13:10:13 +
Peter Maydell  wrote:

> On 18 January 2018 at 13:06, Igor Mammedov  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

Re: [Qemu-devel] [PATCH 00/24] generalize parsing of cpu_model (part 4)

[Peter Maydell]

On 18 January 2018 at 13:06, Igor Mammedov  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

Re: [Qemu-devel] [PATCH 00/24] generalize parsing of cpu_model (part 4)

[Igor Mammedov]

On Thu, 18 Jan 2018 10:50:19 +
Peter Maydell  wrote:

> On 18 January 2018 at 10:43, Igor Mammedov  wrote:
> > Peter Maydell  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

Re: [Qemu-devel] [PATCH 00/24] generalize parsing of cpu_model (part 4)

[Peter Maydell]

On 18 January 2018 at 10:43, Igor Mammedov  wrote:
> Peter Maydell  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

Re: [Qemu-devel] [PATCH 00/24] generalize parsing of cpu_model (part 4)

[Igor Mammedov]

On Wed, 17 Jan 2018 20:30:14 +
Peter Maydell  wrote:

> On 17 January 2018 at 19:15, Igor Mammedov  wrote:
> > On Wed, 17 Jan 2018 16:12:09 +
> > Peter Maydell  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

Re: [Qemu-devel] [PATCH 00/24] generalize parsing of cpu_model (part 4)

[Peter Maydell]

On 17 January 2018 at 19:15, Igor Mammedov  wrote:
> On Wed, 17 Jan 2018 16:12:09 +
> Peter Maydell  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

Re: [Qemu-devel] [PATCH 00/24] generalize parsing of cpu_model (part 4)

[Igor Mammedov]

On Wed, 17 Jan 2018 16:12:09 +
Peter Maydell  wrote:

> On 17 January 2018 at 15:43, Igor Mammedov  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

Re: [Qemu-devel] [PATCH 00/24] generalize parsing of cpu_model (part 4)

[Peter Maydell]

On 17 January 2018 at 15:43, Igor Mammedov  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