Re: [PATCH v8 10/10] OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug


Laszlo Ersek
 

On 02/22/21 08:19, Ankur Arora wrote:
Advertise OVMF support for CPU hot-unplug and negotiate it
if QEMU requests the feature.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Igor Mammedov <imammedo@redhat.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>
---

Notes:
Addresses the following review comments:
(1,3) s/hot unplug/hot-unplug/
(2) Get rid of the reference to the made up ICH9_APM_CNT_CPU_HOT_UNPLUG
(4,6) Remove the artificial tie in between
ICH9_LPC_SMI_F_CPU_HOTPLUG, ICH9_LPC_SMI_F_CPU_HOT_UNPLUG.
(5) Fully spell out "SMI on CPU hot-unplug".
(7) Emit separate messages on negotiation (or not) of
ICH9_LPC_SMI_F_CPU_HOT_UNPLUG.

OvmfPkg/SmmControl2Dxe/SmiFeatures.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
index c9d875543205..b1d59a559dae 100644
--- a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
+++ b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
@@ -29,6 +29,12 @@
//
#define ICH9_LPC_SMI_F_CPU_HOTPLUG BIT1

+// The following bit value stands for "enable CPU hot-unplug, and inject an SMI
+// with control value ICH9_APM_CNT_CPU_HOTPLUG upon hot-unplug", in the
+// "etc/smi/supported-features" and "etc/smi/requested-features" fw_cfg files.
+//
+#define ICH9_LPC_SMI_F_CPU_HOT_UNPLUG BIT2
+
(1) The comment formatting is inconsistent with the rest. The line right
after the (extant) "ICH9_LPC_SMI_F_CPU_HOTPLUG" macro definition should
be "//", and not an empty line.

In fact, this new hunk should not introduce any empty line.


//
// Provides a scratch buffer (allocated in EfiReservedMemoryType type memory)
// for the S3 boot script fragment to write to and read from.
@@ -112,7 +118,8 @@ NegotiateSmiFeatures (
QemuFwCfgReadBytes (sizeof mSmiFeatures, &mSmiFeatures);

//
- // We want broadcast SMI, SMI on CPU hotplug, and nothing else.
+ // We want broadcast SMI, SMI on CPU hotplug, SMI on CPU hot-unplug
+ // and nothing else.
//
RequestedFeaturesMask = ICH9_LPC_SMI_F_BROADCAST;
if (!MemEncryptSevIsEnabled ()) {
@@ -120,8 +127,10 @@ NegotiateSmiFeatures (
// For now, we only support hotplug with SEV disabled.
//
RequestedFeaturesMask |= ICH9_LPC_SMI_F_CPU_HOTPLUG;
+ RequestedFeaturesMask |= ICH9_LPC_SMI_F_CPU_HOT_UNPLUG;
}
mSmiFeatures &= RequestedFeaturesMask;
+
(2) Spurious empty line addition (it's likely left over from removing
the earlier attempt to "be smarter than QEMU").

The rest seems fine.

Thanks!
Laszlo


QemuFwCfgSelectItem (mRequestedFeaturesItem);
QemuFwCfgWriteBytes (sizeof mSmiFeatures, &mSmiFeatures);

@@ -166,6 +175,13 @@ NegotiateSmiFeatures (
__FUNCTION__));
}

+ if ((mSmiFeatures & ICH9_LPC_SMI_F_CPU_HOT_UNPLUG) == 0) {
+ DEBUG ((DEBUG_INFO, "%a: CPU hot-unplug not negotiated\n", __FUNCTION__));
+ } else {
+ DEBUG ((DEBUG_INFO, "%a: CPU hot-unplug with SMI negotiated\n",
+ __FUNCTION__));
+ }
+
//
// Negotiation successful (although we may not have gotten the optimal
// feature set).

Join devel@edk2.groups.io to automatically receive all group messages.