On 1/20/24 00:48, Ni, Ray via groups.io wrote:
Mike, Tom,
How about AsmCpuId() adds ASSERT() to reject those CPUID leaves that do not
have sub-leaf?
Do you mean those CPUID leaves that do have a sub-leaf?
The worry I have here is that it becomes a maintenance concern having to
update the list of leaves with sub-leaves whenever a new CPUID leaf with
sub-leaves is introduced.
Another concern I have if AsmCpuid() zeros ECX is callers would call
AsmCpuid(leaf) instead of AsmCpuidEx (leaf, 0).
This would cause the caller code a mess.
I think it's just a mindset that I have from kernel programming. In Linux,
the AsmCpuid() equivalent will explicitly zero the ECX register so that
you get the zero sub-leaf if that leaf has sub-leaves.
Using AsmCpuid() where ECX is non-zero and possibly random, could make
debugging harder. By zeroing ECX, you will get repeatable return values to
help with debugging when things aren't going right.
You could then make note that AsmCpuid() always returns the zero sub-leaf
of any CPUID leaf that has sub-leaves.
I agree in principal that if the CPUID leaf has sub-leaves you should be
using AsmCpuidEx(). But as Mike points out, if you happen to use
AsmCpuid() by mistake, then the behavior and returned values are not
deterministic if ECX is not zeroed.
Thanks,
Tom
Thanks,
Ray
From: Kinney, Michael D <michael.d.kin...@intel.com>
Sent: Saturday, January 20, 2024 9:49 AM
To: Ni, Ray <ray...@intel.com>; Tom Lendacky <thomas.lenda...@amd.com>; devel@edk2.groups.io; Gao, Liming
<liming....@intel.com>; Liu, Zhiguang <zhiguang....@intel.com>; Dong, Eric <eric.d...@intel.com>; Kumar,
Rahul R <rahul.r.ku...@intel.com>; Gerd Hoffmann <kra...@redhat.com>; Ard Biesheuvel
<ardb+tianoc...@kernel.org>
Cc: Michael Roth <michael.r...@amd.com>; Kinney, Michael D
<michael.d.kin...@intel.com>
Subject: RE: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx()
for CPUID_EXTENDED_TOPOLOGY leaf
The issue is if AsmCpuid() is called for an Index value that does depend on
ECX. That would be a bug on the caller's part and would not have deterministic
behavior because ECX on input is not deterministic. That is the condition that
would be good to catch.
Mike
From: Ni, Ray <ray...@intel.com<mailto:ray...@intel.com>>
Sent: Friday, January 19, 2024 3:49 PM
To: Kinney, Michael D <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>; Tom Lendacky
<thomas.lenda...@amd.com<mailto:thomas.lenda...@amd.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Gao, Liming
<liming....@intel.com<mailto:liming....@intel.com>>; Liu, Zhiguang <zhiguang....@intel.com<mailto:zhiguang....@intel.com>>; Dong, Eric
<eric.d...@intel.com<mailto:eric.d...@intel.com>>; Kumar, Rahul R <rahul.r.ku...@intel.com<mailto:rahul.r.ku...@intel.com>>; Gerd Hoffmann
<kra...@redhat.com<mailto:kra...@redhat.com>>; Ard Biesheuvel <ardb+tianoc...@kernel.org<mailto:ardb+tianoc...@kernel.org>>
Cc: Michael Roth <michael.r...@amd.com<mailto:michael.r...@amd.com>>; Kinney, Michael D
<michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>
Subject: Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx()
for CPUID_EXTENDED_TOPOLOGY leaf
Mike,
For a certain Cupid leaf that does not have sub leaf, Cupid instruction does
not consume ecx and it always fills ecx with a determined value, defined by sdm.
So, I don't see any hurt to deterministic behavior if not zeroing ecx in
AsmCpuid.
thanks,
ray
________________________________
From: Kinney, Michael D
<michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>
Sent: Saturday, January 20, 2024 7:16:14 AM
To: Ni, Ray <ray...@intel.com<mailto:ray...@intel.com>>; Tom Lendacky <thomas.lenda...@amd.com<mailto:thomas.lenda...@amd.com>>;
devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>; Gao, Liming
<liming....@intel.com<mailto:liming....@intel.com>>; Liu, Zhiguang <zhiguang....@intel.com<mailto:zhiguang....@intel.com>>; Dong, Eric
<eric.d...@intel.com<mailto:eric.d...@intel.com>>; Kumar, Rahul R <rahul.r.ku...@intel.com<mailto:rahul.r.ku...@intel.com>>; Gerd Hoffmann
<kra...@redhat.com<mailto:kra...@redhat.com>>; Ard Biesheuvel <ardb+tianoc...@kernel.org<mailto:ardb+tianoc...@kernel.org>>
Cc: Michael Roth <michael.r...@amd.com<mailto:michael.r...@amd.com>>; Kinney, Michael D
<michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>
Subject: RE: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx()
for CPUID_EXTENDED_TOPOLOGY leaf
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...@intel.com<mailto:ray...@intel.com>>
Sent: Friday, January 19, 2024 2:01 AM
To: Kinney, Michael D
<michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>; Tom Lendacky
<thomas.lenda...@amd.com<mailto:thomas.lenda...@amd.com>>;
devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Gao, Liming
<liming....@intel.com<mailto:liming....@intel.com>>; Liu, Zhiguang
<zhiguang....@intel.com<mailto:zhiguang....@intel.com>>; Dong,
Eric <eric.d...@intel.com<mailto:eric.d...@intel.com>>; Kumar, Rahul R
<rahul.r.ku...@intel.com<mailto:rahul.r.ku...@intel.com>>;
Gerd Hoffmann <kra...@redhat.com<mailto:kra...@redhat.com>>; Ard Biesheuvel
<ardb+tianoc...@kernel.org<mailto:ardb+tianoc...@kernel.org>>
Cc: Michael Roth <michael.r...@amd.com<mailto:michael.r...@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.kin...@intel.com<mailto:michael.d.kin...@intel.com>>
Sent: Friday, January 19, 2024 7:11 AM
To: Tom Lendacky <thomas.lenda...@amd.com<mailto:thomas.lenda...@amd.com>>;
devel@edk2.groups.io<mailto:devel@edk2.groups.io>;
Gao, Liming <liming....@intel.com<mailto:liming....@intel.com>>; Liu, Zhiguang
<zhiguang....@intel.com<mailto:zhiguang....@intel.com>>;
Dong, Eric <eric.d...@intel.com<mailto:eric.d...@intel.com>>; Ni, Ray
<ray...@intel.com<mailto:ray...@intel.com>>; Kumar,
Rahul R
<rahul.r.ku...@intel.com<mailto:rahul.r.ku...@intel.com>>; Gerd Hoffmann
<kra...@redhat.com<mailto:kra...@redhat.com>>; Ard
Biesheuvel <ardb+tianoc...@kernel.org<mailto:ardb+tianoc...@kernel.org>>
Cc: Michael Roth <michael.r...@amd.com<mailto:michael.r...@amd.com>>; Kinney,
Michael D
<michael.d.kin...@intel.com<mailto:michael.d.kin...@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.lenda...@amd.com<mailto:thomas.lenda...@amd.com>>
Sent: Wednesday, January 17, 2024 1:26 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Kinney, Michael D
<michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>; Gao, Liming
<liming....@intel.com<mailto:liming....@intel.com>>;
Liu,
Zhiguang <zhiguang....@intel.com<mailto:zhiguang....@intel.com>>; Dong, Eric
<eric.d...@intel.com<mailto:eric.d...@intel.com>>;
Ni,
Ray <ray...@intel.com<mailto:ray...@intel.com>>; Kumar, Rahul R
<rahul.r.ku...@intel.com<mailto:rahul.r.ku...@intel.com>>;
Gerd
Hoffmann <kra...@redhat.com<mailto:kra...@redhat.com>>; Ard Biesheuvel
<ardb+tianoc...@kernel.org<mailto:ardb+tianoc...@kernel.org>>
Cc: Michael Roth <michael.r...@amd.com<mailto:michael.r...@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114117): https://edk2.groups.io/g/devel/message/114117
Mute This Topic: https://groups.io/mt/102432782/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
