qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Alexander Graf
Subject: Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
Date: Thu, 15 Aug 2013 13:59:39 +0200

On 15.08.2013, at 13:48, 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. :/

Maybe. I don't know - it's always a tradeoff of readability on different cases. 
Maybe we simply have too many CPU models too ;).

> 
> We do have the following:
> 
> "object"
> +- "device"
>   +- "cpu"
>      +- "powerpc64-cpu"
>         +- "POWER7-family-powerpc64-cpu" -> POWERPC_FAMILY()

Ah, there is the 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.

Ah, so your point is that POWER7+ is-a POWER7 today? That's most likely wrong, 
yes. Who pulled in that code?

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

That's not what this patch is about. It's about making the family classes 
instantiable. Could we just remove the abstract = true piece from the POWER7 
class and create that one as our generic POWER7 CPU?

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

Not sure I follow you on this one :).

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

Because we're being dumb ;).

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

That's the reason, yes :). There's just way too much complexity in this code. 
The macros were supposed to reduce that, but either they failed or the code is 
just still too complex despite them.


Alex




reply via email to

[Prev in Thread] Current Thread [Next in Thread]