Re: [Qemu-devel] [PATCH qom-next 08/12] target-i386: introduce cpu-model property for x86_cpu

Re: [Qemu-devel] [PATCH qom-next 08/12] target-i386: introduce cpu-model property for x86_cpu

[Igor Mammedov]

On 05/30/2012 05:22 PM, Andreas Färber wrote:
> Am 30.05.2012 00:10, schrieb Igor Mammedov:
>> it's probably intermidiate step till cpu modeled as
>> sub-classes. After then we probably could drop it.
>>
>> However it still could be used for overiding default
>> cpu subclasses definition, and probably renamed to
>> something like 'features'.
>>
>> v2:
>>   - remove accidential tcg_* init code move
>>
>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>> ---
>>   cpu-defs.h           |    2 +-
>>   hw/pc.c              |   10 ----------
>>   target-i386/cpu.c    |   24 ++++++++++++++++++++++++
>>   target-i386/helper.c |   17 ++++++++++++-----
>>   4 files changed, 37 insertions(+), 16 deletions(-)
>
> For me this is still the big no-go in this series:
>
> * Moving the default cpu_model into cpu_x86_init() buys us nothing IMO,
> it duplicates the property setting code and differs from all other
> targets where the default CPU is the machine's decision.
Agreed.

>
> * As you rightly point out, we are heading towards sub-classes and that
> contradicts this two-step initialization. I don't see how this is an
> intermediate step?
It's not clear to me how sub-classes contradict with two-step initialization,
, could you elaborate more on this?

About cpu_model property being an intermediate step, I think it is
an easy and safe way to hide/isolate cpu implementation details
in cpu.c and use only QOM interface to create cpus.
Later with an introduction of sub-classes it could be reduced to
feature override functionality and when cpu features are converted to
proper properties then cpu-model property could be dropped altogether.

There are 2 problems I am solving with cpu-model property:
1 - APIC should be created before cpu becomes run-able i.e. before
x86_cpu_realize(). Now it's not so but cpu_reset() that is called after APIC
is created kind of 'fixes' issue. Moving APIC [11/12] creation into property
forces us to create it at the proper time.

2 - I'm not sure yet how to deal with APIC creation with sub-classes
introduction. Should it be created in an each sub-class initfn
/doubts: code duplication/ or in parent class /doubts: lack of APICless 486 cpu model/?

I could move APIC from cpu-model property in initfn right now and
create it there and in cpu-model property destroy APIC if cpu_def
doesn't have APIC feature /only 486cpu, may we ignore it?/ or if
feature is asked to be disabled via feature flag in cpu-model string
then just disable it as it's done for the rest of supported cpus
in real hw.


> I admit, I am the one to blame for not redoing the x86 CPU subclasses
> patches yet - major issue being the built-in vs. -cpudef split:
> Now that Eduardo refactored the config file reading, I wonder if we can
> outsource the built-in CPUs to the config file? If so, then I would
> appreciate one of you x86 experts to do that please (may need some
> rebasing when the occasional features get added/tweaked). Then I can
> base a series on top that initializes CPU subclasses in a consistent way
> rather than duplicating data for the builtins just to make -cpudef work
> or creating different intermediate subclasses for both types.
I'll ask Eduardo how I can help here.

>
> * To override CPU features you should think about how to set x86 CPU
> features in a QOM way (think QMP), then we can design the infrastructure
> for setting them through global properties (or whatever needed) around
> it. I honestly don't know what the requirements are in practice, so I
> can't make suggestions there without some more feedback.
>
> Regards,
> Andreas
>

-- 
-----
  Igor

Re: [Qemu-devel] [PATCH qom-next 08/12] target-i386: introduce cpu-model property for x86_cpu

[Andreas Färber]

Am 04.06.2012 16:56, schrieb Igor Mammedov:
> On 05/30/2012 05:22 PM, Andreas Färber wrote:
>> Am 30.05.2012 00:10, schrieb Igor Mammedov:
>>> it's probably intermidiate step till cpu modeled as
>>> sub-classes. After then we probably could drop it.
>>>
>>> However it still could be used for overiding default
>>> cpu subclasses definition, and probably renamed to
>>> something like 'features'.
>>
>> * As you rightly point out, we are heading towards sub-classes and that
>> contradicts this two-step initialization. I don't see how this is an
>> intermediate step?
> It's not clear to me how sub-classes contradict with two-step
> initialization,
> , could you elaborate more on this?

CPU subclasses mean to me that for -cpu qemu64 we would have a QOM type
"qemu64" (or so). initfn would then take care of initializing all
default values, and from cpu_x86_init() we would parse the remaining
cpu_model parameters and set QOM properties on the CPU instance.

Original attempt:
https://github.com/afaerber/qemu-cpu/commit/a27feda42712606ca2303faeb6c7e8478660a1c1

Now, the contradiction is that once we have done object_new("qemu64") we
cannot change its type "qemu64" to anything else. Therefore I dislike
sticking cpu_model into a "cpu-model" property.

What I was talking about wrt features was doing in pseudocode:

object_new("qemu64")
object_property_set_int("family", 42)
object_property_set_string("vendor", "Me, myself and I")
object_property_set_bool("x2apic", true)
...

I.e. decoupling the back part of the cpu_model string from the model. My
patches in master that you and others have reviewed did this for the
mostly numeric CPUID parts (-cpu foo,x=42), with a view to code sharing.

What's missing is properties to set CPU features (-cpu foo,+x,-y). There
the question is how granular do we want to go and which types do we want
to use. The example above shows using a bool property for a specific
feature (without having checked that for correctness). Other
possibilities would be to have a feature string with all those
space-separated acronyms or an int that is a bitfield. One doesn't rule
out the other. Jan's requirement, I think, was to be able to set them
from global properties for pc-1.x backwards compatibility.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg