Hi Tom,
It likely would have been better to define AsmCpuid() to
set ECX=0.
However, how would new source code know if the BaseLib they
linking against has this new behavior or not?
It is probably safer to do what you propose which is to use
AsmCpuidEx() that specifies exactly how ECX is set.
Mike
> -----Original Message-----
> From: Tom Lendacky <thomas.lenda...@amd.com>
> Sent: Monday, November 6, 2023 3:16 PM
> To: devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kin...@intel.com>; Gao, Liming <liming....@intel.com>; Liu,
> Zhiguang <zhiguang....@intel.com>
> Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>;
> Kumar, Rahul R <rahul.r.ku...@intel.com>; Gerd Hoffmann
> <kra...@redhat.com>; Ard Biesheuvel <ardb+tianoc...@kernel.org>;
> Michael Roth <michael.r...@amd.com>
> Subject: Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use
> AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf
>
> 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.
>
> Thanks,
> Tom
>
> >
> > Fixes: d4d7c9ad5fe5 ("UefiCpuPkg/MpInitLib: use BSP to do extended
> ...")
> > Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
> > ---
> > UefiCpuPkg/Library/MpInitLib/AmdSev.c | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/AmdSev.c
> b/UefiCpuPkg/Library/MpInitLib/AmdSev.c
> > index bda4960f6fd3..d34f9513e002 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/AmdSev.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/AmdSev.c
> > @@ -256,7 +256,14 @@ FillExchangeInfoDataSevEs (
> > if (StdRangeMax >= CPUID_EXTENDED_TOPOLOGY) {
> > CPUID_EXTENDED_TOPOLOGY_EBX ExtTopoEbx;
> >
> > - AsmCpuid (CPUID_EXTENDED_TOPOLOGY, NULL, &ExtTopoEbx.Uint32,
> NULL, NULL);
> > + AsmCpuidEx (
> > + CPUID_EXTENDED_TOPOLOGY,
> > + 0,
> > + NULL,
> > + &ExtTopoEbx.Uint32,
> > + NULL,
> > + NULL
> > + );
> > ExchangeInfo->ExtTopoAvail =
> !!ExtTopoEbx.Bits.LogicalProcessors;
> > }
> > }
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110779): https://edk2.groups.io/g/devel/message/110779
Mute This Topic: https://groups.io/mt/102432782/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
