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
