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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 08/16/2013 12:47 AM, 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. 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.


Sorry for my ignorance again, I overlooked that POWER7 is already a family
and it is abstract (did not see families in -cpu ? and got confused). And I
just did not expect families there since Alex Graf said they are not there yet.

I fixed my patch (which does not extend macros) but now I wonder if I
should keep posting it to the list. Do you have any plans to fix this thing
with masks any time soon? Thank you.


-- 
Alexey