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

[Alexander Graf]

On 15.08.2013, at 18:08, Andreas Färber wrote:

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

Even 440 cores seem to have a matching mask, at least in Linux:

        {
                .pvr_mask               = 0xf0000fff,
                .pvr_value              = 0x40000850,
                .cpu_name               = "440GR Rev. A",
                .cpu_features           = CPU_FTRS_44X,
                .cpu_user_features      = COMMON_USER_BOOKE,
                .mmu_features           = MMU_FTR_TYPE_44x,
                .icache_bsize           = 32,
                .dcache_bsize           = 32,
                .machine_check          = machine_check_4xx,
                .platform               = "ppc440",
        },

But you're right, there isn't as much of a scheme to them as with the others. 
So for 440 we would need the intermediate level to allow for multiple PVR 
matches.

So you're saying you'd just always make families abstract and have the pvr mask 
matching happen on actual core revisions? That just feels gross somehow. Meh.

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

I think it makes a lot of sense to share the existing CPU table infrastructure.


Alex