[edk2-devel] [PATCH 0/2] SEV-SNP guest support fixes

[edk2-devel] [PATCH 0/2] SEV-SNP guest support fixes

[Lendacky, Thomas via groups.io]

This patch series provides fixes around AP startup and sorting:

- The CPUID_EXTENDED_TOPOLOGY CPUID leaf takes a sub-leaf as input. The
  current SEV-SNP support is attempting to retrieve the this leaf with
  sub-leaf 0, but is calling AsmCpuid(), which does not clear ECX before
  invoking the CPUID instruction. Therefore, because of the calling
  convention, the leaf value becomes the sub-leaf value and ends up
  returning incorrect information. Change the call from AsmCpuid() to
  AsmCpuidEx().

- When sorting the CPUs by APIC ID, the VMSA associated with the vCPU
  should follow the APIC ID. Update the sorting code to swap the VMSA
  pointer during the sort.

---

These patches are based on commit:
da219919538b ("BaseTools: GenFw: auto-set nxcompat flag")

Tom Lendacky (2):
  UefiCpuPkg/MpInitLib: Use AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY
    leaf
  UefiCpuPkg/MpInitLib: Copy SEV-ES save area pointer during APIC ID
    sorting

 UefiCpuPkg/Library/MpInitLib/AmdSev.c | 9 ++++++++-
 UefiCpuPkg/Library/MpInitLib/MpLib.c  | 8 +++++++-
 2 files changed, 15 insertions(+), 2 deletions(-)

-- 
2.42.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110766): https://edk2.groups.io/g/devel/message/110766
Mute This Topic: https://groups.io/mt/102432041/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf

[Lendacky, Thomas via groups.io]

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.

Fixes: d4d7c9ad5fe5 ("UefiCpuPkg/MpInitLib: use BSP to do extended ...")
Signed-off-by: Tom Lendacky <thomas.lendacky@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;
   }
 }
-- 
2.42.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110767): https://edk2.groups.io/g/devel/message/110767
Mute This Topic: https://groups.io/mt/102432043/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 2/2] UefiCpuPkg/MpInitLib: Copy SEV-ES save area pointer during APIC ID sorting

[Lendacky, Thomas via groups.io]

With SEV-SNP, the SEV-ES save area for a vCPU should be unique to that
vCPU. After commit 3323359a811a, the VMSA allocation was re-used, but when
sorting the CPUs by APIC ID, the save area was not updated to follow the
original CPU. Similar to the StartupApSignal address, the SevEsSaveArea
address should be updated when sorting the CPUs.

This does not cause an issue at this time because all APs are in HLT state
and then are (re)started at the same time, with the same VMSA contents.
However, this should be fixed to account for any change in future
behavior.

Fixes: 3323359a811a ("UefiCpuPkg/MpInitLib: Reuse VMSA allocation to ...")
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 9a6ec5db5ce9..a41b9e5701af 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -370,6 +370,7 @@ SortApicId (
   UINT32           ApCount;
   CPU_INFO_IN_HOB  *CpuInfoInHob;
   volatile UINT32  *StartupApSignal;
+  VOID             *SevEsSaveArea;
 
   ApCount      = CpuMpData->CpuCount - 1;
   CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
@@ -397,12 +398,17 @@ SortApicId (
         CopyMem (&CpuInfoInHob[Index1], &CpuInfo, sizeof (CPU_INFO_IN_HOB));
 
         //
-        // Also exchange the StartupApSignal.
+        // Also exchange the StartupApSignal and SevEsSaveArea.
         //
         StartupApSignal                            = CpuMpData->CpuData[Index3].StartupApSignal;
         CpuMpData->CpuData[Index3].StartupApSignal =
           CpuMpData->CpuData[Index1].StartupApSignal;
         CpuMpData->CpuData[Index1].StartupApSignal = StartupApSignal;
+
+        SevEsSaveArea                            = CpuMpData->CpuData[Index3].SevEsSaveArea;
+        CpuMpData->CpuData[Index3].SevEsSaveArea =
+          CpuMpData->CpuData[Index1].SevEsSaveArea;
+        CpuMpData->CpuData[Index1].SevEsSaveArea = SevEsSaveArea;
       }
     }
 
-- 
2.42.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110768): https://edk2.groups.io/g/devel/message/110768
Mute This Topic: https://groups.io/mt/102432047/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf

[Lendacky, Thomas via groups.io]

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.lendacky@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 (#110771): https://edk2.groups.io/g/devel/message/110771
Mute This Topic: https://groups.io/mt/102432782/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf

[Michael D Kinney]

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.lendacky@amd.com>
> Sent: Monday, November 6, 2023 3:16 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>
> Cc: 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>;
> Michael Roth <michael.roth@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.lendacky@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] SEV-SNP guest support fixes

[Gerd Hoffmann]

On Mon, Nov 06, 2023 at 04:45:29PM -0600, Tom Lendacky wrote:
> This patch series provides fixes around AP startup and sorting:
> 
> - The CPUID_EXTENDED_TOPOLOGY CPUID leaf takes a sub-leaf as input. The
>   current SEV-SNP support is attempting to retrieve the this leaf with
>   sub-leaf 0, but is calling AsmCpuid(), which does not clear ECX before
>   invoking the CPUID instruction. Therefore, because of the calling
>   convention, the leaf value becomes the sub-leaf value and ends up
>   returning incorrect information. Change the call from AsmCpuid() to
>   AsmCpuidEx().
> 
> - When sorting the CPUs by APIC ID, the VMSA associated with the vCPU
>   should follow the APIC ID. Update the sorting code to swap the VMSA
>   pointer during the sort.

Series:
Acked-by: Gerd Hoffmann <kraxel@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110831): https://edk2.groups.io/g/devel/message/110831
Mute This Topic: https://groups.io/mt/102432041/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] SEV-SNP guest support fixes

[Lendacky, Thomas via groups.io]

On 11/7/23 03:55, Gerd Hoffmann wrote:
> On Mon, Nov 06, 2023 at 04:45:29PM -0600, Tom Lendacky wrote:
>> This patch series provides fixes around AP startup and sorting:
>>
>> - The CPUID_EXTENDED_TOPOLOGY CPUID leaf takes a sub-leaf as input. The
>>    current SEV-SNP support is attempting to retrieve the this leaf with
>>    sub-leaf 0, but is calling AsmCpuid(), which does not clear ECX before
>>    invoking the CPUID instruction. Therefore, because of the calling
>>    convention, the leaf value becomes the sub-leaf value and ends up
>>    returning incorrect information. Change the call from AsmCpuid() to
>>    AsmCpuidEx().
>>
>> - When sorting the CPUs by APIC ID, the VMSA associated with the vCPU
>>    should follow the APIC ID. Update the sorting code to swap the VMSA
>>    pointer during the sort.
> 
> Series:
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> 

Eric/Ray/Rahul,

Have you had a chance to review this series? These bug fixes would be good 
to get in for the next/latest release...

Thanks,
Tom


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111416): https://edk2.groups.io/g/devel/message/111416
Mute This Topic: https://groups.io/mt/102432041/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] SEV-SNP guest support fixes

[Lendacky, Thomas via groups.io]

On 11/17/23 15:43, Tom Lendacky wrote:
> 
> 
> On 11/7/23 03:55, Gerd Hoffmann wrote:
>> On Mon, Nov 06, 2023 at 04:45:29PM -0600, Tom Lendacky wrote:
>>> This patch series provides fixes around AP startup and sorting:
>>>
>>> - The CPUID_EXTENDED_TOPOLOGY CPUID leaf takes a sub-leaf as input. The
>>>    current SEV-SNP support is attempting to retrieve the this leaf with
>>>    sub-leaf 0, but is calling AsmCpuid(), which does not clear ECX before
>>>    invoking the CPUID instruction. Therefore, because of the calling
>>>    convention, the leaf value becomes the sub-leaf value and ends up
>>>    returning incorrect information. Change the call from AsmCpuid() to
>>>    AsmCpuidEx().
>>>
>>> - When sorting the CPUs by APIC ID, the VMSA associated with the vCPU
>>>    should follow the APIC ID. Update the sorting code to swap the VMSA
>>>    pointer during the sort.
>>
>> Series:
>> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
>>
> 
> Eric/Ray/Rahul,
> 
> Have you had a chance to review this series? These bug fixes would be good 
> to get in for the next/latest release...

Eric/Ray/Rahul,

Any feedback? If not, can you Ack them so they can be merged... they 
already missed the edk2-stable202311 tag.

Thanks,
Tom

> 
> Thanks,
> Tom


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111756): https://edk2.groups.io/g/devel/message/111756
Mute This Topic: https://groups.io/mt/102432041/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

Thanks,
Ray
> -----Original Message-----
> From: Tom Lendacky <thomas.lendacky@amd.com>
> Sent: Tuesday, November 7, 2023 6:46 AM
> To: devel@edk2.groups.io
> Cc: 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>; Michael Roth
> <michael.roth@amd.com>
> Subject: [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx() for
> CPUID_EXTENDED_TOPOLOGY leaf
> 
> 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.
> 
> Fixes: d4d7c9ad5fe5 ("UefiCpuPkg/MpInitLib: use BSP to do extended ...")
> Signed-off-by: Tom Lendacky <thomas.lendacky@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;
>    }
>  }
> --
> 2.42.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111805): https://edk2.groups.io/g/devel/message/111805
Mute This Topic: https://groups.io/mt/102432043/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] UefiCpuPkg/MpInitLib: Copy SEV-ES save area pointer during APIC ID sorting

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

Thanks,
Ray
> -----Original Message-----
> From: Tom Lendacky <thomas.lendacky@amd.com>
> Sent: Tuesday, November 7, 2023 6:46 AM
> To: devel@edk2.groups.io
> Cc: 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>; Michael Roth
> <michael.roth@amd.com>
> Subject: [PATCH 2/2] UefiCpuPkg/MpInitLib: Copy SEV-ES save area pointer
> during APIC ID sorting
> 
> With SEV-SNP, the SEV-ES save area for a vCPU should be unique to that
> vCPU. After commit 3323359a811a, the VMSA allocation was re-used, but
> when
> sorting the CPUs by APIC ID, the save area was not updated to follow the
> original CPU. Similar to the StartupApSignal address, the SevEsSaveArea
> address should be updated when sorting the CPUs.
> 
> This does not cause an issue at this time because all APs are in HLT state
> and then are (re)started at the same time, with the same VMSA contents.
> However, this should be fixed to account for any change in future
> behavior.
> 
> Fixes: 3323359a811a ("UefiCpuPkg/MpInitLib: Reuse VMSA allocation to ...")
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 9a6ec5db5ce9..a41b9e5701af 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -370,6 +370,7 @@ SortApicId (
>    UINT32           ApCount;
>    CPU_INFO_IN_HOB  *CpuInfoInHob;
>    volatile UINT32  *StartupApSignal;
> +  VOID             *SevEsSaveArea;
> 
>    ApCount      = CpuMpData->CpuCount - 1;
>    CpuInfoInHob = (CPU_INFO_IN_HOB
> *)(UINTN)CpuMpData->CpuInfoInHob;
> @@ -397,12 +398,17 @@ SortApicId (
>          CopyMem (&CpuInfoInHob[Index1], &CpuInfo, sizeof
> (CPU_INFO_IN_HOB));
> 
>          //
> -        // Also exchange the StartupApSignal.
> +        // Also exchange the StartupApSignal and SevEsSaveArea.
>          //
>          StartupApSignal                            =
> CpuMpData->CpuData[Index3].StartupApSignal;
>          CpuMpData->CpuData[Index3].StartupApSignal =
>            CpuMpData->CpuData[Index1].StartupApSignal;
>          CpuMpData->CpuData[Index1].StartupApSignal = StartupApSignal;
> +
> +        SevEsSaveArea                            =
> CpuMpData->CpuData[Index3].SevEsSaveArea;
> +        CpuMpData->CpuData[Index3].SevEsSaveArea =
> +          CpuMpData->CpuData[Index1].SevEsSaveArea;
> +        CpuMpData->CpuData[Index1].SevEsSaveArea = SevEsSaveArea;
>        }
>      }
> 
> --
> 2.42.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111806): https://edk2.groups.io/g/devel/message/111806
Mute This Topic: https://groups.io/mt/102432047/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf

[Lendacky, Thomas via groups.io]

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.

Thanks,
Tom

> 
> Thanks,
> Tom
> 
>>
>> Fixes: d4d7c9ad5fe5 ("UefiCpuPkg/MpInitLib: use BSP to do extended ...")
>> Signed-off-by: Tom Lendacky <thomas.lendacky@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 (#111810): https://edk2.groups.io/g/devel/message/111810
Mute This Topic: https://groups.io/mt/102432782/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf

[Lendacky, Thomas via groups.io]

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 (#113959): https://edk2.groups.io/g/devel/message/113959
Mute This Topic: https://groups.io/mt/102432782/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf

[Michael D Kinney]

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114015): https://edk2.groups.io/g/devel/message/114015
Mute This Topic: https://groups.io/mt/102432782/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf

[Ni, Ray]

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114039): https://edk2.groups.io/g/devel/message/114039
Mute This Topic: https://groups.io/mt/102432782/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf

[Michael D Kinney]

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114100): https://edk2.groups.io/g/devel/message/114100
Mute This Topic: https://groups.io/mt/102432782/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf

[Ni, Ray]

[-- Attachment #1: Type: text/plain, Size: 6380 bytes --]

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.kinney@intel.com>
Sent: Saturday, January 20, 2024 7:16:14 AM
To: Ni, Ray <ray.ni@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>; devel@edk2.groups.io <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

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114103): https://edk2.groups.io/g/devel/message/114103
Mute This Topic: https://groups.io/mt/102432782/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 9254 bytes --]

Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf

[Michael D Kinney]

[-- Attachment #1: Type: text/plain, Size: 8691 bytes --]

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.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

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.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Sent: Saturday, January 20, 2024 7:16:14 AM
To: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Tom Lendacky <thomas.lendacky@amd.com<mailto:thomas.lendacky@amd.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kumar, Rahul R <rahul.r.kumar@intel.com<mailto:rahul.r.kumar@intel.com>>; Gerd Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>; Ard Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>
Cc: Michael Roth <michael.roth@amd.com<mailto:michael.roth@amd.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@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.ni@intel.com<mailto:ray.ni@intel.com>>
> Sent: Friday, January 19, 2024 2:01 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Tom Lendacky
> <thomas.lendacky@amd.com<mailto:thomas.lendacky@amd.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Gao, Liming
> <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>; Dong,
> Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kumar, Rahul R <rahul.r.kumar@intel.com<mailto:rahul.r.kumar@intel.com>>;
> Gerd Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>; Ard Biesheuvel
> <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>
> Cc: Michael Roth <michael.roth@amd.com<mailto: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<mailto:michael.d.kinney@intel.com>>
> > Sent: Friday, January 19, 2024 7:11 AM
> > To: Tom Lendacky <thomas.lendacky@amd.com<mailto:thomas.lendacky@amd.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>;
> > Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Liu, Zhiguang
> <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>;
> > Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Kumar,
> Rahul R
> > <rahul.r.kumar@intel.com<mailto:rahul.r.kumar@intel.com>>; Gerd Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>; Ard
> > Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>
> > Cc: Michael Roth <michael.roth@amd.com<mailto:michael.roth@amd.com>>; Kinney, Michael D
> > <michael.d.kinney@intel.com<mailto: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<mailto:thomas.lendacky@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.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>;
> Liu,
> > > Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>;
> Ni,
> > > Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Kumar, Rahul R <rahul.r.kumar@intel.com<mailto:rahul.r.kumar@intel.com>>;
> Gerd
> > > Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>; Ard Biesheuvel
> > <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>
> > > Cc: Michael Roth <michael.roth@amd.com<mailto: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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114105): https://edk2.groups.io/g/devel/message/114105
Mute This Topic: https://groups.io/mt/102432782/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 14526 bytes --]

Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf

[Ni, Ray]

[-- Attachment #1: Type: text/plain, Size: 9961 bytes --]

Mike, Tom,
How about AsmCpuId() adds ASSERT() to reject those CPUID leaves that do not have sub-leaf?

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.

Thanks,
Ray
From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Saturday, January 20, 2024 9:49 AM
To: Ni, Ray <ray.ni@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

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.ni@intel.com<mailto:ray.ni@intel.com>>
Sent: Friday, January 19, 2024 3:49 PM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Tom Lendacky <thomas.lendacky@amd.com<mailto:thomas.lendacky@amd.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kumar, Rahul R <rahul.r.kumar@intel.com<mailto:rahul.r.kumar@intel.com>>; Gerd Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>; Ard Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>
Cc: Michael Roth <michael.roth@amd.com<mailto:michael.roth@amd.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@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.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Sent: Saturday, January 20, 2024 7:16:14 AM
To: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Tom Lendacky <thomas.lendacky@amd.com<mailto:thomas.lendacky@amd.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kumar, Rahul R <rahul.r.kumar@intel.com<mailto:rahul.r.kumar@intel.com>>; Gerd Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>; Ard Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>
Cc: Michael Roth <michael.roth@amd.com<mailto:michael.roth@amd.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@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.ni@intel.com<mailto:ray.ni@intel.com>>
> Sent: Friday, January 19, 2024 2:01 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Tom Lendacky
> <thomas.lendacky@amd.com<mailto:thomas.lendacky@amd.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Gao, Liming
> <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>; Dong,
> Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kumar, Rahul R <rahul.r.kumar@intel.com<mailto:rahul.r.kumar@intel.com>>;
> Gerd Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>; Ard Biesheuvel
> <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>
> Cc: Michael Roth <michael.roth@amd.com<mailto: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<mailto:michael.d.kinney@intel.com>>
> > Sent: Friday, January 19, 2024 7:11 AM
> > To: Tom Lendacky <thomas.lendacky@amd.com<mailto:thomas.lendacky@amd.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>;
> > Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Liu, Zhiguang
> <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>;
> > Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Kumar,
> Rahul R
> > <rahul.r.kumar@intel.com<mailto:rahul.r.kumar@intel.com>>; Gerd Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>; Ard
> > Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>
> > Cc: Michael Roth <michael.roth@amd.com<mailto:michael.roth@amd.com>>; Kinney, Michael D
> > <michael.d.kinney@intel.com<mailto: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<mailto:thomas.lendacky@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.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>;
> Liu,
> > > Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>;
> Ni,
> > > Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Kumar, Rahul R <rahul.r.kumar@intel.com<mailto:rahul.r.kumar@intel.com>>;
> Gerd
> > > Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>; Ard Biesheuvel
> > <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>
> > > Cc: Michael Roth <michael.roth@amd.com<mailto: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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114113): https://edk2.groups.io/g/devel/message/114113
Mute This Topic: https://groups.io/mt/102432782/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 17006 bytes --]

Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf

[Lendacky, Thomas via groups.io]

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.kinney@intel.com>
> Sent: Saturday, January 20, 2024 9:49 AM
> To: Ni, Ray <ray.ni@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
> 
> 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.ni@intel.com<mailto:ray.ni@intel.com>>
> Sent: Friday, January 19, 2024 3:49 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Tom Lendacky <thomas.lendacky@amd.com<mailto:thomas.lendacky@amd.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kumar, Rahul R <rahul.r.kumar@intel.com<mailto:rahul.r.kumar@intel.com>>; Gerd Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>; Ard Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>
> Cc: Michael Roth <michael.roth@amd.com<mailto:michael.roth@amd.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@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.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
> Sent: Saturday, January 20, 2024 7:16:14 AM
> To: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Tom Lendacky <thomas.lendacky@amd.com<mailto:thomas.lendacky@amd.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kumar, Rahul R <rahul.r.kumar@intel.com<mailto:rahul.r.kumar@intel.com>>; Gerd Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>; Ard Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>
> Cc: Michael Roth <michael.roth@amd.com<mailto:michael.roth@amd.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@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.ni@intel.com<mailto:ray.ni@intel.com>>
>> Sent: Friday, January 19, 2024 2:01 AM
>> To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Tom Lendacky
>> <thomas.lendacky@amd.com<mailto:thomas.lendacky@amd.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Gao, Liming
>> <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>; Dong,
>> Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kumar, Rahul R <rahul.r.kumar@intel.com<mailto:rahul.r.kumar@intel.com>>;
>> Gerd Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>; Ard Biesheuvel
>> <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>
>> Cc: Michael Roth <michael.roth@amd.com<mailto: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<mailto:michael.d.kinney@intel.com>>
>>> Sent: Friday, January 19, 2024 7:11 AM
>>> To: Tom Lendacky <thomas.lendacky@amd.com<mailto:thomas.lendacky@amd.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>;
>>> Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Liu, Zhiguang
>> <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>;
>>> Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Kumar,
>> Rahul R
>>> <rahul.r.kumar@intel.com<mailto:rahul.r.kumar@intel.com>>; Gerd Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>; Ard
>>> Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>
>>> Cc: Michael Roth <michael.roth@amd.com<mailto:michael.roth@amd.com>>; Kinney, Michael D
>>> <michael.d.kinney@intel.com<mailto: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<mailto:thomas.lendacky@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.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>;
>> Liu,
>>>> Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>;
>> Ni,
>>>> Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Kumar, Rahul R <rahul.r.kumar@intel.com<mailto:rahul.r.kumar@intel.com>>;
>> Gerd
>>>> Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>; Ard Biesheuvel
>>> <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>
>>>> Cc: Michael Roth <michael.roth@amd.com<mailto: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
> 
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf

[Ni, Ray]

> 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.

Intel SDM or AMD equivalent "SDM" should not change a CPUID leaf in a way
that it does not consume ECX today but consume it tomorrow.
This is a non-backward compatible instruction change.

With that, it's easy to maintain a list of CPUID leaf that does not have sub-leaf
today, or a list of ones that have sub-leaf.
It's a feature to help on debuggability, similar to today's StrLen() implementation
in BaseLib that avoids reading the memory to no-end.


> 
> >
> > 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.

I agree with your concern.
How about having both? I mean not only zeroing out ECX but also rejecting the
CPUID leaves which have sub-leaf.
It's a lib API. I still don't want AsmCpuid to own the responsibility to call sub-leaf 0.
So, in debug build, AsmCpuid() asserts on some calls.
In release build, we get fixed behavior.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114122): https://edk2.groups.io/g/devel/message/114122
Mute This Topic: https://groups.io/mt/102432782/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-