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

[Andreas Färber]

Am 15.08.2013 17:51, schrieb Alexander Graf:
> 
> On 15.08.2013, at 17:43, Andreas Färber wrote:
> 
>> Am 15.08.2013 17:29, schrieb Alexander Graf:
>>>
>>> On 15.08.2013, at 16:47, Andreas Färber wrote:
>>>
>>>> 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.
>>>
>>> Not sure I understand. What is KVM specific about this?
>>
>> -cpu host is, it's in kvm.c.
>>
>> These patches are changing sort comparison code in translate_ppc.c
>> though, which is used in multiple places.
>>
>>>
>>>> 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.
>>>
>>> I think this goes into the wrong direction. We should have one single 
>>> unified scheme to model core versions and -cpu host should be able to 
>>> override them for a family, no? I don't see how instantiating a POWER7_v20 
>>> object on a POWER7_v23 system is any improvement over instantiating a 
>>> POWER7 object.
>>
>> There is no one unified scheme, as we have discussed in your absence.
>>
>> My point is, a) -cpu POWER7 should result in valid values
> 
> Yes :)

...which requires a specific vX.Y PVR in addition to the mask, i.e. a
model in our current terms. :)

Consider that there may be differences between models within one family,
otherwise there would be little point to distinguish them.

>> and b) you
>> asked to have a unified macro scheme that works for all CPUs, you got
>> it, now instead you are asking for something that is nice to POWERx, and
>> we cannot make POWERx family different from the rest wrt macros unless
>> we break the scheme, which you specifically wanted to have, to avoid
>> boilerplate QOM code you said. Now you want the full customization
>> goodness that you were against just before! :)
> 
> Ah, nonono, I don't want POWER to be any different. I want things unified and 
> consistent. Any time I mention "POWER7" I also mean "e500" or "440" or any 
> other family class we have out there.
> 
> What I was proposing was to make _all_ families non-abstract and have _all_ 
> families support major/minor parameters.

Again, I pointed out looong ago on the POWER7+ patch
http://patchwork.ozlabs.org/patch/264176/
(which you really could've looked up yourself by now!)
that major/minor does not apply to all CPUs. It works for POWER and for
e500, but that's about it. I specifically gave 440 as an example where
it doesn't!

Note that there's no strict necessity for "host" to be derived from any
existing model, it seemed convenient to me at the time. It could just as
well be created in-place in KVM code iff you can figure out via ioctls
or assembly code what MMU, flags, etc. to fill in beyond PVR - not sure
which fields are even relevant for KVM, I just looked for patterns and
possible OOD / build-time optimizations in that code. :)

Andreas

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