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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Feb 11, 2022 at 12:42:11PM +0100, Jan Beulich wrote:
> 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.

Thanks, I did miss that part. Then I think it's fine to initialize the
struct in recheck_cpu_features to zero. That's likely to make it more
obvious if we somehow miss to update certain featuresets? (as they
would be empty instead of inheriting from boot CPU data).

Thanks, Roger.