Define CPU_HOT_EJECT_DATA and add PCD PcdCpuHotEjectDataAddress,
which would be used to share CPU ejection state between
PiSmmCpuDxeSmm and OvmfPkg/CpuHotPlugSmm.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Aaron Young <aaron.young@oracle.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
UefiCpuPkg/Include/CpuHotPlugData.h | 21 +++++++++++++++++++++
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 +
UefiCpuPkg/UefiCpuPkg.dec | 5 +++++
UefiCpuPkg/UefiCpuPkg.uni | 4 ++++
4 files changed, 31 insertions(+)
diff --git a/UefiCpuPkg/Include/CpuHotPlugData.h b/UefiCpuPkg/Include/CpuHotPlugData.h
index 6321a149fa52..86f964550655 100644
--- a/UefiCpuPkg/Include/CpuHotPlugData.h
+++ b/UefiCpuPkg/Include/CpuHotPlugData.h
@@ -2,6 +2,7 @@
Definition for a structure sharing information for CPU hot plug.
Copyright (c) 2013 - 2015, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2021, Oracle Corporation.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -24,4 +25,24 @@ typedef struct {
UINT32 SmrrSize;
} CPU_HOT_PLUG_DATA;
+typedef
+VOID
+(EFIAPI *CPU_HOT_EJECT_FN)(
+ IN UINTN ProcessorNum
+ );
+
+#define CPU_EJECT_INVALID (MAX_UINT64)
+#define CPU_EJECT_WORKER (MAX_UINT64-1)
+
+#define CPU_HOT_EJECT_DATA_REVISION_1 0x00000001
+
+typedef struct {
+ UINT32 Revision; // Used for version identification for this structure
+ UINT32 ArrayLength; // The entries number of the following ApicId array and SmBase array
+
+ UINT64 *ApicIdMap; // Pointer to CpuIndex->ApicId map. Holds APIC IDs for pending ejects
+ CPU_HOT_EJECT_FN Handler; // Handler for CPU ejection
+ UINT64 Reserved;
+} CPU_HOT_EJECT_DATA;
+
#endif
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index 76b146299679..f79c874d74f1 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -131,6 +131,7 @@
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout ## CONSUMES
gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress ## SOMETIMES_CONSUMES
gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress ## SOMETIMES_PRODUCES
+ gUefiCpuPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress ## SOMETIMES_PRODUCES
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmCodeAccessCheckEnable ## CONSUMES
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode ## CONSUMES
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmShadowStackSize ## SOMETIMES_CONSUMES
diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index d83c084467b3..704ccc05f662 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -339,6 +339,11 @@
# @ValidList 0x80000001 | 0
gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress|0x0|UINT64|0x60000011
+ ## Contains the pointer to a CPU Hot Eject Data structure if CPU hot-plug is supported.
+ # @Prompt The pointer to CPU Hot Eject Data.
+ # @ValidList 0x80000001 | 0
+ gUefiCpuPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress|0x0|UINT64|0x60000017
+
## Indicates processor feature capabilities, each bit corresponding to a specific feature.
# @Prompt Processor feature capabilities.
# @ValidList 0x80000001 | 0
diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
index 219c1963bf08..b1721f256632 100644
--- a/UefiCpuPkg/UefiCpuPkg.uni
+++ b/UefiCpuPkg/UefiCpuPkg.uni
@@ -140,6 +140,10 @@
#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotPlugDataAddress_HELP #language en-US "Contains the pointer to a CPU Hot Plug Data structure if CPU hot-plug is supported."
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotEjectDataAddress_PROMPT #language en-US "The pointer to CPU Hot Eject Data"
+
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotEjectDataAddress_HELP #language en-US "Contains the pointer to a CPU Hot Eject Data structure if CPU hot-plug is supported."
+
#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuNumberOfReservedVariableMtrrs_PROMPT #language en-US "Number of reserved variable MTRRs"
#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuNumberOfReservedVariableMtrrs_HELP #language en-US "Specifies the number of variable MTRRs reserved for OS use."
--
2.9.3
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70383): https://edk2.groups.io/g/devel/message/70383
Mute This Topic: https://groups.io/mt/79697164/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Ankur,
On 01/15/21 08:45, Ankur Arora wrote:
> Define CPU_HOT_EJECT_DATA and add PCD PcdCpuHotEjectDataAddress,
> which would be used to share CPU ejection state between
> PiSmmCpuDxeSmm and OvmfPkg/CpuHotPlugSmm.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Aaron Young <aaron.young@oracle.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> ---
> UefiCpuPkg/Include/CpuHotPlugData.h | 21 +++++++++++++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 +
> UefiCpuPkg/UefiCpuPkg.dec | 5 +++++
> UefiCpuPkg/UefiCpuPkg.uni | 4 ++++
> 4 files changed, 31 insertions(+)
>
> diff --git a/UefiCpuPkg/Include/CpuHotPlugData.h b/UefiCpuPkg/Include/CpuHotPlugData.h
> index 6321a149fa52..86f964550655 100644
> --- a/UefiCpuPkg/Include/CpuHotPlugData.h
> +++ b/UefiCpuPkg/Include/CpuHotPlugData.h
> @@ -2,6 +2,7 @@
> Definition for a structure sharing information for CPU hot plug.
>
> Copyright (c) 2013 - 2015, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2021, Oracle Corporation.<BR>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> **/
> @@ -24,4 +25,24 @@ typedef struct {
> UINT32 SmrrSize;
> } CPU_HOT_PLUG_DATA;
>
> +typedef
> +VOID
> +(EFIAPI *CPU_HOT_EJECT_FN)(
> + IN UINTN ProcessorNum
> + );
> +
> +#define CPU_EJECT_INVALID (MAX_UINT64)
> +#define CPU_EJECT_WORKER (MAX_UINT64-1)
> +
> +#define CPU_HOT_EJECT_DATA_REVISION_1 0x00000001
> +
> +typedef struct {
> + UINT32 Revision; // Used for version identification for this structure
> + UINT32 ArrayLength; // The entries number of the following ApicId array and SmBase array
> +
> + UINT64 *ApicIdMap; // Pointer to CpuIndex->ApicId map. Holds APIC IDs for pending ejects
> + CPU_HOT_EJECT_FN Handler; // Handler for CPU ejection
> + UINT64 Reserved;
> +} CPU_HOT_EJECT_DATA;
> +
> #endif
I'm sorry, I still don't understand -- why is it necessary to modify
UefiCpuPkg *at all*, for this feature?
(1) The structure type for the data exchange should be in an OvmfPkg
header file.
(2) The dynamic PCD for transferring the address of the structure should
be declared in the "OvmfPkg.dec" file.
(3) The "handler" call is made
- from SmmCpuFeaturesRendezvousExit() in OvmfPkg/Library/SmmCpuFeaturesLib,
- to CpuEject() in OvmfPkg/CpuHotplugSmm.
First, this call should not need a function pointer at all -- the
CpuEject() logic should be flattened into
OvmfPkg/Library/SmmCpuFeaturesLib).
Second, if that's not possible -- please explain why --, then a function
pointer might be justified after all, but the information channel still
seems to have zero relevance for UefiCpuPkg.
It's possible I'm not seeing something; please explain.
Thanks
Laszlo
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> index 76b146299679..f79c874d74f1 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> @@ -131,6 +131,7 @@
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout ## CONSUMES
> gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress ## SOMETIMES_CONSUMES
> gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress ## SOMETIMES_PRODUCES
> + gUefiCpuPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress ## SOMETIMES_PRODUCES
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmCodeAccessCheckEnable ## CONSUMES
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode ## CONSUMES
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmShadowStackSize ## SOMETIMES_CONSUMES
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index d83c084467b3..704ccc05f662 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -339,6 +339,11 @@
> # @ValidList 0x80000001 | 0
> gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress|0x0|UINT64|0x60000011
>
> + ## Contains the pointer to a CPU Hot Eject Data structure if CPU hot-plug is supported.
> + # @Prompt The pointer to CPU Hot Eject Data.
> + # @ValidList 0x80000001 | 0
> + gUefiCpuPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress|0x0|UINT64|0x60000017
> +
> ## Indicates processor feature capabilities, each bit corresponding to a specific feature.
> # @Prompt Processor feature capabilities.
> # @ValidList 0x80000001 | 0
> diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
> index 219c1963bf08..b1721f256632 100644
> --- a/UefiCpuPkg/UefiCpuPkg.uni
> +++ b/UefiCpuPkg/UefiCpuPkg.uni
> @@ -140,6 +140,10 @@
>
> #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotPlugDataAddress_HELP #language en-US "Contains the pointer to a CPU Hot Plug Data structure if CPU hot-plug is supported."
>
> +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotEjectDataAddress_PROMPT #language en-US "The pointer to CPU Hot Eject Data"
> +
> +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotEjectDataAddress_HELP #language en-US "Contains the pointer to a CPU Hot Eject Data structure if CPU hot-plug is supported."
> +
> #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuNumberOfReservedVariableMtrrs_PROMPT #language en-US "Number of reserved variable MTRRs"
>
> #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuNumberOfReservedVariableMtrrs_HELP #language en-US "Specifies the number of variable MTRRs reserved for OS use."
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70388): https://edk2.groups.io/g/devel/message/70388
Mute This Topic: https://groups.io/mt/79697164/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 2021-01-15 12:17 a.m., Laszlo Ersek wrote:
> Hi Ankur,
>
> On 01/15/21 08:45, Ankur Arora wrote:
>> Define CPU_HOT_EJECT_DATA and add PCD PcdCpuHotEjectDataAddress,
>> which would be used to share CPU ejection state between
>> PiSmmCpuDxeSmm and OvmfPkg/CpuHotPlugSmm.
>>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Cc: Aaron Young <aaron.young@oracle.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>> ---
>> UefiCpuPkg/Include/CpuHotPlugData.h | 21 +++++++++++++++++++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 +
>> UefiCpuPkg/UefiCpuPkg.dec | 5 +++++
>> UefiCpuPkg/UefiCpuPkg.uni | 4 ++++
>> 4 files changed, 31 insertions(+)
>>
>> diff --git a/UefiCpuPkg/Include/CpuHotPlugData.h b/UefiCpuPkg/Include/CpuHotPlugData.h
>> index 6321a149fa52..86f964550655 100644
>> --- a/UefiCpuPkg/Include/CpuHotPlugData.h
>> +++ b/UefiCpuPkg/Include/CpuHotPlugData.h
>> @@ -2,6 +2,7 @@
>> Definition for a structure sharing information for CPU hot plug.
>>
>> Copyright (c) 2013 - 2015, Intel Corporation. All rights reserved.<BR>
>> +Copyright (c) 2021, Oracle Corporation.<BR>
>> SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> **/
>> @@ -24,4 +25,24 @@ typedef struct {
>> UINT32 SmrrSize;
>> } CPU_HOT_PLUG_DATA;
>>
>> +typedef
>> +VOID
>> +(EFIAPI *CPU_HOT_EJECT_FN)(
>> + IN UINTN ProcessorNum
>> + );
>> +
>> +#define CPU_EJECT_INVALID (MAX_UINT64)
>> +#define CPU_EJECT_WORKER (MAX_UINT64-1)
>> +
>> +#define CPU_HOT_EJECT_DATA_REVISION_1 0x00000001
>> +
>> +typedef struct {
>> + UINT32 Revision; // Used for version identification for this structure
>> + UINT32 ArrayLength; // The entries number of the following ApicId array and SmBase array
>> +
>> + UINT64 *ApicIdMap; // Pointer to CpuIndex->ApicId map. Holds APIC IDs for pending ejects
>> + CPU_HOT_EJECT_FN Handler; // Handler for CPU ejection
>> + UINT64 Reserved;
>> +} CPU_HOT_EJECT_DATA;
>> +
>> #endif
>
> I'm sorry, I still don't understand -- why is it necessary to modify
> UefiCpuPkg *at all*, for this feature?
>
> (1) The structure type for the data exchange should be in an OvmfPkg
> header file.
>
> (2) The dynamic PCD for transferring the address of the structure should
> be declared in the "OvmfPkg.dec" file.
>
> (3) The "handler" call is made
> - from SmmCpuFeaturesRendezvousExit() in OvmfPkg/Library/SmmCpuFeaturesLib,
> - to CpuEject() in OvmfPkg/CpuHotplugSmm.
>
> First, this call should not need a function pointer at all -- the
> CpuEject() logic should be flattened into
> OvmfPkg/Library/SmmCpuFeaturesLib).
Sure, it can be flattened -- but it's out of place in SmmCpuFeaturesLib.
All of the logic pertaining to the unplug is in CpuHotPlugSmm, so it seems to
make sense to locate the related ejection code along with it.
If you are concerned about paying the additional cost of an indirect call
then I think it should be possible to install the handler only when there
is an actual unplug to be done and remove it after.
>
> Second, if that's not possible -- please explain why --, then a function
> pointer might be justified after all, but the information channel still
> seems to have zero relevance for UefiCpuPkg.
The reason for keeping the PCD in UefiCpuPkg was that its declaration and
init are in SmmCpuFeaturesLib which gets folded into the UefiCpuPkg/PiSmmCpuDxe
execution context and so the export happening via OvmfPkg.dec seemed off.
That said, I guess your underlying point is that this adds unnecessary state to
non OVMF builds (?), which it does, so I can move the PCD to OvmfPkg.dec.
Thanks
Ankur
>
> It's possible I'm not seeing something; please explain.
>
> Thanks
> Laszlo
>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
>> index 76b146299679..f79c874d74f1 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
>> @@ -131,6 +131,7 @@
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout ## CONSUMES
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress ## SOMETIMES_CONSUMES
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress ## SOMETIMES_PRODUCES
>> + gUefiCpuPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress ## SOMETIMES_PRODUCES
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmCodeAccessCheckEnable ## CONSUMES
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode ## CONSUMES
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmShadowStackSize ## SOMETIMES_CONSUMES
>> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
>> index d83c084467b3..704ccc05f662 100644
>> --- a/UefiCpuPkg/UefiCpuPkg.dec
>> +++ b/UefiCpuPkg/UefiCpuPkg.dec
>> @@ -339,6 +339,11 @@
>> # @ValidList 0x80000001 | 0
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress|0x0|UINT64|0x60000011
>>
>> + ## Contains the pointer to a CPU Hot Eject Data structure if CPU hot-plug is supported.
>> + # @Prompt The pointer to CPU Hot Eject Data.
>> + # @ValidList 0x80000001 | 0
>> + gUefiCpuPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress|0x0|UINT64|0x60000017
>> +
>> ## Indicates processor feature capabilities, each bit corresponding to a specific feature.
>> # @Prompt Processor feature capabilities.
>> # @ValidList 0x80000001 | 0
>> diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
>> index 219c1963bf08..b1721f256632 100644
>> --- a/UefiCpuPkg/UefiCpuPkg.uni
>> +++ b/UefiCpuPkg/UefiCpuPkg.uni
>> @@ -140,6 +140,10 @@
>>
>> #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotPlugDataAddress_HELP #language en-US "Contains the pointer to a CPU Hot Plug Data structure if CPU hot-plug is supported."
>>
>> +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotEjectDataAddress_PROMPT #language en-US "The pointer to CPU Hot Eject Data"
>> +
>> +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotEjectDataAddress_HELP #language en-US "Contains the pointer to a CPU Hot Eject Data structure if CPU hot-plug is supported."
>> +
>> #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuNumberOfReservedVariableMtrrs_PROMPT #language en-US "Number of reserved variable MTRRs"
>>
>> #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuNumberOfReservedVariableMtrrs_HELP #language en-US "Specifies the number of variable MTRRs reserved for OS use."
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70435): https://edk2.groups.io/g/devel/message/70435
Mute This Topic: https://groups.io/mt/79697164/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 01/15/21 19:16, Ankur Arora wrote:
> On 2021-01-15 12:17 a.m., Laszlo Ersek wrote:
>> Hi Ankur,
>>
>> On 01/15/21 08:45, Ankur Arora wrote:
>>> Define CPU_HOT_EJECT_DATA and add PCD PcdCpuHotEjectDataAddress,
>>> which would be used to share CPU ejection state between
>>> PiSmmCpuDxeSmm and OvmfPkg/CpuHotPlugSmm.
>>>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> Cc: Aaron Young <aaron.young@oracle.com>
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>>> ---
>>> UefiCpuPkg/Include/CpuHotPlugData.h | 21
>>> +++++++++++++++++++++
>>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 +
>>> UefiCpuPkg/UefiCpuPkg.dec | 5 +++++
>>> UefiCpuPkg/UefiCpuPkg.uni | 4 ++++
>>> 4 files changed, 31 insertions(+)
>>>
>>> diff --git a/UefiCpuPkg/Include/CpuHotPlugData.h
>>> b/UefiCpuPkg/Include/CpuHotPlugData.h
>>> index 6321a149fa52..86f964550655 100644
>>> --- a/UefiCpuPkg/Include/CpuHotPlugData.h
>>> +++ b/UefiCpuPkg/Include/CpuHotPlugData.h
>>> @@ -2,6 +2,7 @@
>>> Definition for a structure sharing information for CPU hot plug.
>>> Copyright (c) 2013 - 2015, Intel Corporation. All rights
>>> reserved.<BR>
>>> +Copyright (c) 2021, Oracle Corporation.<BR>
>>> SPDX-License-Identifier: BSD-2-Clause-Patent
>>> **/
>>> @@ -24,4 +25,24 @@ typedef struct {
>>> UINT32 SmrrSize;
>>> } CPU_HOT_PLUG_DATA;
>>> +typedef
>>> +VOID
>>> +(EFIAPI *CPU_HOT_EJECT_FN)(
>>> + IN UINTN ProcessorNum
>>> + );
>>> +
>>> +#define CPU_EJECT_INVALID (MAX_UINT64)
>>> +#define CPU_EJECT_WORKER (MAX_UINT64-1)
>>> +
>>> +#define CPU_HOT_EJECT_DATA_REVISION_1 0x00000001
>>> +
>>> +typedef struct {
>>> + UINT32 Revision; // Used for version
>>> identification for this structure
>>> + UINT32 ArrayLength; // The entries number of the
>>> following ApicId array and SmBase array
>>> +
>>> + UINT64 *ApicIdMap; // Pointer to CpuIndex->ApicId
>>> map. Holds APIC IDs for pending ejects
>>> + CPU_HOT_EJECT_FN Handler; // Handler for CPU ejection
>>> + UINT64 Reserved;
>>> +} CPU_HOT_EJECT_DATA;
>>> +
>>> #endif
>>
>> I'm sorry, I still don't understand -- why is it necessary to modify
>> UefiCpuPkg *at all*, for this feature?
>>
>> (1) The structure type for the data exchange should be in an OvmfPkg
>> header file.
>>
>> (2) The dynamic PCD for transferring the address of the structure should
>> be declared in the "OvmfPkg.dec" file.
>>
>> (3) The "handler" call is made
>> - from SmmCpuFeaturesRendezvousExit() in
>> OvmfPkg/Library/SmmCpuFeaturesLib,
>> - to CpuEject() in OvmfPkg/CpuHotplugSmm.
>>
>> First, this call should not need a function pointer at all -- the
>> CpuEject() logic should be flattened into
>> OvmfPkg/Library/SmmCpuFeaturesLib).
>
> Sure, it can be flattened -- but it's out of place in SmmCpuFeaturesLib.
> All of the logic pertaining to the unplug is in CpuHotPlugSmm, so it
> seems to
> make sense to locate the related ejection code along with it.
OK. Having a (possibly NULL) function pointer also doubles as a "control
knob" to see whether the feature is enabled or not. I'm fine with the
function pointer, then.
> If you are concerned about paying the additional cost of an indirect call
> then I think it should be possible to install the handler only when there
> is an actual unplug to be done and remove it after.
No, that wasn't my concern.
>>
>> Second, if that's not possible -- please explain why --, then a function
>> pointer might be justified after all, but the information channel still
>> seems to have zero relevance for UefiCpuPkg.
>
> The reason for keeping the PCD in UefiCpuPkg was that its declaration and
> init are in SmmCpuFeaturesLib which gets folded into the
> UefiCpuPkg/PiSmmCpuDxe
> execution context and so the export happening via OvmfPkg.dec seemed off.
No, it's not off, that's precisely the goal. SmmCpuFeaturesLib is a
dedicated platform customization interface for PiSmmCpuDxeSmm. (By
platform, I mean "firmware platform"; such as OvmfPkg.)
SmmCpuFeaturesLib exists becuase PiSmmCpuDxeSmm wants it to exist.
If we can solve a task by hiding it entirely in SmmCpuFeaturesLib, in
connection with other parts of the firmware platform, we should do that.
That's why the SmmCpuFeaturesLib class was invented, with carefully
selected hook points. UefiCpuPkg in general is a core package, not a
firmware platform package, so we only modify UefiCpuPkg for platform
needs if that is absolutely unavoidable.
We are modifying the SmmCpuFeaturesLib instance provided by OvmfPkg, so
we should strive for keeping the internals of that solution internal to
OvmfPkg -- such as a PCD declared in OvmfPkg.dec, a structure type
defined in an OvmfPkg include file, and so on. We're welcome to stuff as
much platform logic into PiSmmCpuDxeSmm through our platform's
SmmCpuFeaturesLib instance as possible, so long as we have an actual
justified purpose with that "stuffing", and we honor the interface
contracts.
> That said, I guess your underlying point is that this adds unnecessary
> state to non OVMF builds (?), which it does, so I can move the PCD to OvmfPkg.dec.
Yes, that's my point. Ideally, the diffstat of the series should
entirely stay within OvmfPkg.
I would suggest even splitting off the last patch (for CpuDeadLoop())
into its own submission. That patch could be merged sooner than, and
independently of, the unplug feature for OVMF.
Is it OK with you if I ask you to submit a v4 like that, before I start
going through the series in detail?
A bit more feedback on folding this UefiCpuPkg content into OvmfPkg:
- "OvmfPkg/Include/CpuHotUnplug.h" looks good to me, for the header file
(feel free to replace Unplug with Eject though, if you like the latter more)
- in INF files, in every section, such as [Sources], [Pcds] etc, please
keep entries (filenames, PCD names) alphabetically sorted -- unless the
preexistent order already breaks this property
- don't bother about a .uni file under OvmfPkg
- in "OvmfPkg.dec", please find the PCD with the highest token value,
and for introducing the new PCD, use a new token value that's one
greater than the current maximum.
Thank you!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70449): https://edk2.groups.io/g/devel/message/70449
Mute This Topic: https://groups.io/mt/79697164/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 2021-01-15 10:44 a.m., Laszlo Ersek wrote: > On 01/15/21 19:16, Ankur Arora wrote: >> On 2021-01-15 12:17 a.m., Laszlo Ersek wrote: >>> Hi Ankur, >>> >>> On 01/15/21 08:45, Ankur Arora wrote: >>>> Define CPU_HOT_EJECT_DATA and add PCD PcdCpuHotEjectDataAddress, >>>> which would be used to share CPU ejection state between >>>> PiSmmCpuDxeSmm and OvmfPkg/CpuHotPlugSmm. >>>> [...] >>> >>> Second, if that's not possible -- please explain why --, then a function >>> pointer might be justified after all, but the information channel still >>> seems to have zero relevance for UefiCpuPkg. >> >> The reason for keeping the PCD in UefiCpuPkg was that its declaration and >> init are in SmmCpuFeaturesLib which gets folded into the >> UefiCpuPkg/PiSmmCpuDxe >> execution context and so the export happening via OvmfPkg.dec seemed off. > > No, it's not off, that's precisely the goal. SmmCpuFeaturesLib is a > dedicated platform customization interface for PiSmmCpuDxeSmm. (By > platform, I mean "firmware platform"; such as OvmfPkg.) > SmmCpuFeaturesLib exists becuase PiSmmCpuDxeSmm wants it to exist. > > If we can solve a task by hiding it entirely in SmmCpuFeaturesLib, in > connection with other parts of the firmware platform, we should do that. > That's why the SmmCpuFeaturesLib class was invented, with carefully > selected hook points. UefiCpuPkg in general is a core package, not a > firmware platform package, so we only modify UefiCpuPkg for platform > needs if that is absolutely unavoidable. > > We are modifying the SmmCpuFeaturesLib instance provided by OvmfPkg, so > we should strive for keeping the internals of that solution internal to > OvmfPkg -- such as a PCD declared in OvmfPkg.dec, a structure type > defined in an OvmfPkg include file, and so on. We're welcome to stuff as > much platform logic into PiSmmCpuDxeSmm through our platform's > SmmCpuFeaturesLib instance as possible, so long as we have an actual > justified purpose with that "stuffing", and we honor the interface > contracts. > >> That said, I guess your underlying point is that this adds unnecessary >> state to non OVMF builds (?), which it does, so I can move the PCD to OvmfPkg.dec. > > Yes, that's my point. Ideally, the diffstat of the series should > entirely stay within OvmfPkg. > > I would suggest even splitting off the last patch (for CpuDeadLoop()) > into its own submission. That patch could be merged sooner than, and > independently of, the unplug feature for OVMF. > > Is it OK with you if I ask you to submit a v4 like that, before I start > going through the series in detail? Sure. Let me send a v4 with these changes. Ankur > > A bit more feedback on folding this UefiCpuPkg content into OvmfPkg: > > - "OvmfPkg/Include/CpuHotUnplug.h" looks good to me, for the header file > (feel free to replace Unplug with Eject though, if you like the latter more) > > - in INF files, in every section, such as [Sources], [Pcds] etc, please > keep entries (filenames, PCD names) alphabetically sorted -- unless the > preexistent order already breaks this property > > - don't bother about a .uni file under OvmfPkg > > - in "OvmfPkg.dec", please find the PCD with the highest token value, > and for introducing the new PCD, use a new token value that's one > greater than the current maximum. > > Thank you! > Laszlo > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#70451): https://edk2.groups.io/g/devel/message/70451 Mute This Topic: https://groups.io/mt/79697164/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.