Re: [PATCH CPU v1] cpuid: initialize cpuinfo with boot_cpu_data

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.02.2022 11:47, Roger Pau Monné wrote:
> On Fri, Feb 11, 2022 at 11:32:45AM +0100, Jan Beulich wrote:
>> On 11.02.2022 10:02, Roger Pau Monné wrote:
>>> On Fri, Feb 11, 2022 at 08:23:27AM +0100, Norbert Manthey wrote:
>>>> When re-identifying CPU data, we might use uninitialized data when
>>>> checking for the cache line property to adapt the cache
>>>> alignment. The data that depends on this uninitialized read is
>>>> currently not forwarded.
>>>>
>>>> To avoid problems in the future, initialize the data cpuinfo
>>>> structure before re-identifying the CPU again.
>>>>
>>>> The trace to hit the uninitialized read reported by Coverity is:
>>>>
>>>> bool recheck_cpu_features(unsigned int cpu)
>>>> ...
>>>>     struct cpuinfo_x86 c;
>>>>     ...
>>>>     identify_cpu(&c);
>>>>
>>>> void identify_cpu(struct cpuinfo_x86 *c)
>>>> ...
>>>>     generic_identify(c)
>>>>
>>>> static void generic_identify(struct cpuinfo_x86 *c)
>>>> ...
>>>
>>> Would it be more appropriate for generic_identify to also set
>>> x86_cache_alignment like it's done in early_cpu_init?
>>>
>>> generic_identify already re-fetches a bunch of stuff that's also
>>> set by early_cpu_init for the BSP.
>>
>> This would be an option, but how sure are you that there isn't
>> (going to be) another field with similar properties? We better
>> wouldn't require _everything_ to be re-filled in generic_identify().
> 
> So you think generic_identify should call into early_cpu_init, or even
> split the cpuinfo_x86 filling done in early_cpu_init into a non-init
> function that could be called by both generic_identify and
> early_cpu_init?

No, I think it is quite fine for this to be a two-step process. In
fact I was hoping to eliminate some of the remaining redundancy
where possible. recheck_cpu_features(), after all, cares about just
the feature flags, so doesn't require things like cache line
alignment to be filled in.

Jan