From: Ni, Ray <ray.ni@intel.com>
Sent: Friday, January 19, 2024 3:49 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>; devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>; Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Michael Roth <michael.roth@amd.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf

thanks,

ray

Hi Ray,

It is about having deterministic behavior if a call if made for
a CPUID EAX value that does depend on ECX.  If ECX is not zeroed,
then it will have a random value that may return different
information.

The problem statement from Tom is not about zeroing ECX.  It is
about avoiding code bugs where AsmCpuid() is called for an Index
value that is documented to depend on ECX.  In this case, we would
want an error condition so the developer knows they should use
AsmCpuidEx() instead.

From looking at the Intel SDM, there is a small set of Index
values that do not look at ECX at all.  We could consider
adding an ASSERT() condition in AsmCpuid() if Index is
a value that depends on ECX.  Perhaps in DEBUG_CODE() so
it is not always present.

Mike

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Friday, January 19, 2024 2:01 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; devel@edk2.groups.io; Gao, Liming
> <liming.gao@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>; Dong,
> Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
> Gerd Hoffmann <kraxel@redhat.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>
> Cc: Michael Roth <michael.roth@amd.com>
> Subject: RE: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use
> AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf
>
> Mike,
> I agree with your words after "However".
> Zeroing ECX in AsmCpuid() is confusing to future code maintainer: If
> CPUID instruction does
> not consume "ECX", why is it needed to zero "ECX"?
>
> Thanks,
> Ray
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > Sent: Friday, January 19, 2024 7:11 AM
> > To: Tom Lendacky <thomas.lendacky@amd.com>; devel@edk2.groups.io;
> > Gao, Liming <liming.gao@intel.com>; Liu, Zhiguang
> <zhiguang.liu@intel.com>;
> > Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Rahul R
> > <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Ard
> > Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Michael Roth <michael.roth@amd.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > Subject: RE: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use
> > AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf
> >
> > Hi Tom,
> >
> > I do not see any harm in zeroing ECX in AsmCpuid().
> >
> > If it is not zeroed, then it would have an undefined value.
> >
> > However, calling AsmCpuid() for any Index that evaluates ECX
> > (including a check for 0) should never be done.  If ECX is
> > evaluated for a given Index, then AsmCpuIdEx() must be used.
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Tom Lendacky <thomas.lendacky@amd.com>
> > > Sent: Wednesday, January 17, 2024 1:26 PM
> > > To: devel@edk2.groups.io; Kinney, Michael D
> > > <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>;
> Liu,
> > > Zhiguang <zhiguang.liu@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Ni,
> > > Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
> Gerd
> > > Hoffmann <kraxel@redhat.com>; Ard Biesheuvel
> > <ardb+tianocore@kernel.org>
> > > Cc: Michael Roth <michael.roth@amd.com>
> > > Subject: Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use
> > > AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf
> > >
> > > On 11/28/23 08:35, Lendacky, Thomas via groups.io wrote:
> > > > On 11/6/23 17:15, Tom Lendacky wrote:
> > > >> On 11/6/23 16:45, Lendacky, Thomas via groups.io wrote:
> > > >>> The CPUID_EXTENDED_TOPOLOGY CPUID leaf takes a subleaf as input
> > when
> > > >>> returning CPUID information. However, the AsmCpuid() function
> does
> > > not
> > > >>> zero out ECX before the CPUID instruction, so the input leaf is
> used
> > > as
> > > >>> the sub-leaf for the CPUID request and returns erroneous/invalid
> > > CPUID
> > > >>> data, since the intent of the request was to get data related to
> > > sub-leaf
> > > >>> 0. Instead, use AsmCpuidEx() for the CPUID_EXTENDED_TOPOLOGY
> leaf.
> > > >>
> > > >> Alternatively, the AsmCpuid() function could be changed to XOR
> ECX
> > > >> before invoking the CPUID instruction. This would ensure that the
> 0
> > > >> sub-leaf is returned for any CPUID leaves that support sub-
> leaves.
> > > >> Thoughts?
> > > >>
> > > >> Adding some additional maintainers for their thoughts, too.
> > > >
> > > > Any thoughts on this approach (as a separate, unrelated patch) to
> > > > eliminate future issues that could pop up?
> > > >
> > > > Seems like zeroing out ECX before calling CPUID would be an
> > > appropriate
> > > > thing to do, but I'm not sure if that will have any impact on the
> > > existing
> > > > code base... it shouldn't, but you never know.
> > >
> > > Just a re-ping for thoughts on this.
> > >
> > > Thanks,
> > > Tom
> > >
> > > >
> > > > Thanks,
> > > > Tom
> > > >
> > > >>
> > > >> Thanks,
> > > >> Tom