Re: [Qemu-ppc] [RFC PATCH v3] powerpc: add PVR mask support

[Alexander Graf]

On 15.08.2013, at 16:47, Andreas Färber wrote:

> Am 15.08.2013 15:55, schrieb Alexey Kardashevskiy:
>> On 08/15/2013 09:48 PM, Andreas Färber wrote:
>>> Am 15.08.2013 13:03, schrieb Alexander Graf:
>>>> 
>>>> On 15.08.2013, at 12:52, Andreas Färber wrote:
>>>> 
>>>>> Am 15.08.2013 10:45, schrieb Alexander Graf:
>>>>>> 
>>>>>> Yes, I think it makes sense to keep the full PVR around when we want to 
>>>>>> be specific. What I'm referring to is class specific logic that can 
>>>>>> assemble major/minor numbers from the command line. So
>>>>>> 
>>>>>> -cpu POWER7,major=2,minor=0
>>>>>> 
>>>>>> would result in a PVR value that is identical to POWER7_v2.0. The 
>>>>>> assembly of this PVR value is class specific, because different classes 
>>>>>> of CPUs have different semantics for their major and minor numbers.
>>>>>> 
>>>>>> That way in the future we won't have to add any new version specific CPU 
>>>>>> types but instead the user can assemble those himself, making everyone's 
>>>>>> life a lot easier.
>>>>>> 
>>>>>> My point was that if we have that logic, we could at the same place just 
>>>>>> say "if my major/minor is 0, default to something reasonable".
>>>>>> 
>>>>>> But let's ask Andreas whether he has a better idea here :).
>>>>> 
>>>>> If you read the previous discussion on the initial POWER7+ patch, I
>>>>> believe I had proposed major-version / minor-version or so properties at
>>>>> family level, to be able to use different implementations or none at all
>>>>> where we don't see a scheme.
>>>> 
>>>> Sounds like a good idea.
>>>> 
>>>>> However if we want to use that from -cpu as
>>>>> in your example above, we would have to implement custom parsing code
>>>>> for cpu_model, which I would rather avoid, given we want to replace it
>>>>> with -device in the future.
>>>> 
>>>> Can't we make this generic QOM property parsing code?
>>>> 
>>>>  -cpu POWER7,major-version=2,minor-version=0
>>>> 
>>>> would do
>>>> 
>>>>  cpu = new POWER7(major-version = 2, minor_version = 0);
>>>> 
>>>> and then the POWER7 class can decide what to do with this additional 
>>>> information?
>>> 
>>> That is "custom parsing code for cpu_model" in target-ppc then. x86 has
>>> its own implementation and so does sparc, both not fully QOM'ified yet,
>>> so there is no one-size-fits-all.
>>> 
>>>>> But maybe I didn't fully catch the exact question. :)
>>>>> 
>>>>> The custom parenting strikes me as a wrong consequence of us not having
>>>>> fully QOM'ified / cleaned up the family classes yet. We had discussed
>>>>> two ways: Either have, e.g., POWER7+ inherit from POWER7 (which looks
>>>>> like the only reason this is being done here) and/or have, e.g., POWER5+
>>>>> copy and modify 970fx values via #defines.
>>>> 
>>>> IIUC the family parenting is orthogonal to this. Here we're looking at 
>>>> having families as classes at all. Currently we don't - we only have 
>>>> explicit versioned implementations as classes.
>>> 
>>> That's simply not true!!! All is hidden by macros as requested by you -
>>> sounds as if that was a bad idea after all. :/
>>> 
>>> We do have the following:
>>> 
>>> "object"
>>> +- "device"
>>>   +- "cpu"
>>>      +- "powerpc64-cpu"
>>>         +- "POWER7-family-powerpc64-cpu" -> POWERPC_FAMILY()
>>>            +- "POWER7_v2.0-powerpc64-cpu" -> POWERPC_DEF_SVR()
>>>               +- "host-powerpc64-cpu" (depending on host PVR)
>>> 
>>> That's why I was saying: If we need POWER7+-specific family code, we
>>> need to have a POWER7P family and not reuse POWER7 as conveniently done
>>> today. All is there to implement properties or whatever at that level.
>>> 
>>> And that's also why trying to do the parent tweaking in
>>> POWERPC_DEF_FAMILY_MEMBER() is bogus. The existing infrastructure just
>>> needs to be used the right way, sigh.
>>> 
>>> And to clean up the aliases business, we should simply move them into
>>> the POWER7_v2.0-powerpc64-cpu level class as an array, I think. That
>>> would greatly simplify -cpu ?, and the alias-to-type lookup would get
>>> faster at the same time since we wouldn't be looking at unavailable
>>> models anymore.
>>> 
>>>> Whether we have
>>>> 
>>>> PowerPC
>>>>  `- POWER7
>>>>    `- POWER7+
>>>>      `- POWER7+ v1.0
>>>> 
>>>> or
>>>> 
>>>> PowerPC
>>>>  `- POWER7+
>>>>    `- POWER7+ v1.0
>>>> 
>>>> is a different question I think.
>>> 
>>> My question is: Why are you guys trying to create yet another type for
>>> "POWER7" when we already have one. The only plausible-to-me explanation
>>> was that avoidance of separate POWER7P family was the core cause, but
>>> apparently the core problem is that no one except me is actually
>>> grasping the macro'fied code or at least you lost the overview during
>>> your vacation... :(
>> 
>> 
>> I am not trying to add any additional POWER7. We do not have POWER7 in QEMU
>> at all, just some approaching/approximation (whatever word suits, sorry for
>> my weak, terrible english). POWER7 (forget about POWER7+ and others) with
>> PVR=0x003FAABB would still be absolutely valid POWER7 everywhere but QEMU
>> (until we support the exact PVR with the specific patch which would add
>> _anything_ new but just definition). Sorry for my deep ignorance if I miss
>> the point. Thank you.
> 
> There is nothing wrong with finding a mask or wildcard solution to that
> problem, I already indicated so on the original POWER+ patch. The point
> of the whole discussion is how to get there in the least invasive way.
> Not whether, just how.
> 
> I think - unlike Alex apparently - that the least invasive way is to
> leave models as they are and to add masking support to families and KVM
> code only.

Not sure I understand. What is KVM specific about this?

> I'm already trying to get away from extending the
> POWERPC_DEF* macros for Prerna's fw_name, which are starting to get a
> big conflict point these days and a future pain if everyone extends them
> for the feature of the day. Note that I started with reading v3, not
> everything from the start, and am therefore not pointing fingers at
> anyone. It may be that you were given some unfortunate suggestions and
> too quick in implementing them.
> 
> When we instantiate a -cpu POWER9 then having one POWER9_vX.Y around to
> back it doesn't really hurt. Unlike ARM's MIDR there doesn't seem to be
> an encoding of IBM vendor or POWER family in the PVR. The macros and
> their new implementation are not the way they are because I consider
> them the nicest thing in the world but because the name+pvr+svr+family
> combination made them work for the whole zoo of models we carry around
> and started to give us some inheritance through QOM. Making the POWER7
> family non-abstract would require the same kind of macro "overloading"
> for POWERPC_FAMILY that I'm trying to contain for POWERPC_DEF ATM. So
> what I am still thinking about is how to handle there being multiple
> matches for a PVR - I am considering putting them into a list and
> comparing values for closest match. So that if you have a v2.4 and QEMU
> knows v2.1 and v2.3 we take v2.3 and fill in the v2.4 PVR.

I think this goes into the wrong direction. We should have one single unified 
scheme to model core versions and -cpu host should be able to override them for 
a family, no? I don't see how instantiating a POWER7_v20 object on a POWER7_v23 
system is any improvement over instantiating a POWER7 object.


Alex