Re: [Qemu-ppc] Cache sizes and device tree

[Alexander Graf]

On 20.11.2012, at 00:26, David Gibson wrote:

> On Mon, Nov 19, 2012 at 11:38:35PM +0100, Alexander Graf wrote:
>> 
>> On 19.11.2012, at 23:05, David Gibson wrote:
>> 
>>> On Mon, Nov 19, 2012 at 12:32:51PM +0100, Alexander Graf wrote:
>>>> 
>>>> On 16.11.2012, at 04:36, David Gibson wrote:
>>>> 
>>>>> Hi Alex,
>>>>> 
>>>>> At present, pseries doesn't set the i-cache-size and d-cache-size
>>>>> properties on the device tree CPU nodes.  That's a bit of a pain,
>>>>> because it means the guest kernel then doesn't provide some files in
>>>>> /sys and that means that lscpu bombs out with an error.
>>>>> 
>>>>> So the question is what should we put in there.  For KVM it's
>>>>> straightforward enough for us to find the host L1 cache sizes and put
>>>>> those in the guest device tree.  However to make it work for full emu
>>>>> as well, we'll need to add this cache size information to the qemu cpu
>>>>> table somewhere.
>>>> 
>>>> Well, cache size should be modeled pretty much the same way as cache
>>>> line size, right? Today, for cache line size, we just set the
>>>> respective values in the cpu init functions.
>>>> 
>>>>> In practice, I think we need a way to add it incrementally - since I
>>>>> really don't want to have to look up and confirm the L1 cache size for
>>>>> every one of the hundreds of supported CPU models in order to make an
>>>>> initial patch.
>>>> 
>>>> Sure. Just add a check if (env->cache_size_l1) and only set the dt
>>>> property in that case ;). For KVM. override the env value similar to
>>>> how we do it for the timebase.
>>> 
>>> Erm.. I'm not sure which timebase override you're referring to.
>> 
>> I am referring to kvmppc_get_tbfreq() :).
> 
> Hrm.  I was hoping not to have to explicitly test for kvm for this in
> the machine, as is necessary for kvmppc_get_tbfreq).

Ah, I think I'm slowly starting to understand what you're getting at :).

Since all that -cpu host does is search in our "known CPUs" table for entries 
that match the host's CPU, we want to have the cache size information in the 
CPU definition. So there is no need at all to fetch the host's values, right?

> 
>>>>> I'm thinking it should probably go into ppc_def_t.  That will need a
>>>>> new DEF macro which includes the cache sizes, and then there will be a
>>>>> somewhat ugly but manageable transition to the new macro as we add
>>>>> cache sizes to various CPUs.  But then I'm not entirely sure how to
>>>>> get to the information when we're building the devtree, since there
>>>>> isn't actually an obvious link from the CPUPPCState to the ppc_def_t
>>>>> it was created from.
>>>> 
>>>> I think just shoving it into env should be enough, no? Do we have
>>>> any cache information that goes beyond vcpu level, so that we can
>>>> not populate it in env?
>>> 
>>> Well.. I was thinking ppc_def_t because there's an obvious place to
>>> retrieve the host value in the kvm case - kvmppc_host_cpu_def().  It's
>>> not so clear to me where we would do that if it's only in env.
>> 
>> Well, usually everything in env gets populated by the init
>> function. Our current -cpu host implementation doesn't allow us to
>> override the cpu init function. We should allow it to do so.
>> 
>> Instead of calling the base_spec init function, we should call a
>> host init function which calls the base_spec init function and then
>> overrides env variables based on what we want to override, like
>> cache size.
> 
> Hm, ok.  I could do it from kvmppc_fixup_cpu().  Does that seem
> reasonable?

I guess I was confused at first. Cache size is always identical for a specific 
PVR, right? So yes, it should be handled exactly like the [d|i]cache_line_size 
fields. Just add a check in the fdt code to only actually populate the device 
tree values when env->cache_size_l1 != 0. Then add the respective cache size 
information in the POWER7 and 970 init functions :).

However, if cache sizes can change when in the same PVR, we do want to be able 
to override it in the KVM case. Then we want the same as the timebase code, 
with the default coming from env.


Alex