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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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.



-- 
Alexey