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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.02.2022 12:19, Roger Pau Monné wrote:
> On Fri, Feb 11, 2022 at 11:50:46AM +0100, Jan Beulich wrote:
>> 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.
> 
> But it's not a two step process for all CPUs. It's a two step process
> for the BSP, that will get it's cpuinfo filled by early_cpu_init
> first, and then by identify_cpu. OTHO APs will only get the
> information filled by identify_cpu.
> 
> Maybe APs don't care about having x86_cache_alignment correctly set?

They do care, and the field also isn't left uninitialized. See
initialize_cpu_data(). Like in many other places we assume full
symmetry among processors here.

Jan