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

[Andreas Färber]

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

Cheers,
Andreas

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