Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Reinette Chatre]

Hi Borislav,

On 8/3/2019 2:44 AM, Borislav Petkov wrote:
> On Fri, Aug 02, 2019 at 01:11:13PM -0700, Reinette Chatre wrote:
>> This patch only makes it possible to determine whether cache is
>> inclusive for some x86 platforms while all platforms of all
>> architectures are given visibility into this new "inclusive" cache
>> information field within the global "struct cacheinfo".
> 
> And this begs the question: do the other architectures even need that
> thing exposed? As it is now, this is x86-only so I'm wondering whether
> adding all that machinery to the generic struct cacheinfo is even needed
> at this point.

I do not know if more users would appear in the future but the goal of
this patch is to make this information available to resctrl. Since there
are a few ways to do so I really appreciate your guidance on how to do
this right. I'd be happy to make needed changes.

> TBH, I'd do it differently: read CPUID at init time and cache the
> information whether the cache is inclusive locally and be done with it.
> It is going to be a single system-wide bit anyway as I'd strongly assume
> cache settings like inclusivity should be the same across the whole
> system.

I've been digesting your comment and tried a few options but I have been
unable to come up with something that fulfill all your suggestions -
specifically the "single system-wide bit" one. These areas of code are
unfamiliar to me so I am not confident what I came up with for other
suggestions are the right way either.

So far it seems I can do the following:
Introduce a new bitfield into struct cpuinfo_x86. There is one existing
bitfield in this structure ("initialized") so we could add a new one
("x86_cache_l3_inclusive"?) just before it. With this information
included in this struct it becomes available via the global
boot_cpu_data, this seems to address the "system-wide bit" but this
struct is also maintained for all other CPUs on the system so may not be
what you had in mind (not a "single system-wide bit")?

If proceeding with inclusion into struct cpuinfo_x86 this new bitfield
can be initialized within init_intel_cacheinfo(). There are a few cache
properties within cpuinfo_x86 and they are initialized in a variety of
paths. init_intel_cacheinfo() is initialized via init_intel() and it
already has the needed CPUID information available making initialization
here straight forward. Alternatively there is also identify_cpu() that
also initializes many cache properties ... but would need some more code
to obtain needed values.

Finally, if the above is done, the resctrl code could just refer to this
new property directly as obtained from boot_cpu_data.x86_cache_l3_inclusive

What do you think?

> 
> When the other arches do need it, we can extract that info "up" into the
> generic layer.

Reinette