On 2021-02-01 9:37 a.m., Laszlo Ersek wrote:
On 01/29/21 01:59, Ankur Arora wrote:
As part of the negotiation treat ICH9_LPC_SMI_F_CPU_HOT_UNPLUG as a
subfeature of feature flag ICH9_LPC_SMI_F_CPU_HOTPLUG, so enable it
only if the other is also being negotiated.
Cc: Laszlo Ersek <ler...@redhat.com>
Cc: Jordan Justen <jordan.l.jus...@intel.com>
Cc: Ard Biesheuvel <ard.biesheu...@arm.com>
Cc: Igor Mammedov <imamm...@redhat.com>
Cc: Boris Ostrovsky <boris.ostrov...@oracle.com>
Cc: Aaron Young <aaron.yo...@oracle.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
Signed-off-by: Ankur Arora <ankur.a.ar...@oracle.com>
---
OvmfPkg/SmmControl2Dxe/SmiFeatures.c | 25 ++++++++++++++++++++++---
1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
index c9d875543205..e70f3f8b58cb 100644
--- a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
+++ b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
@@ -29,6 +29,13 @@
//
#define ICH9_LPC_SMI_F_CPU_HOTPLUG BIT1
+// The following bit value stands for "enable CPU hot unplug, and inject an SMI
(1) s/hot unplug/hot-unplug/
+// with control value ICH9_APM_CNT_CPU_HOT_UNPLUG upon hot unplug", in the
(2) There is no such thing as ICH9_APM_CNT_CPU_HOT_UNPLUG; we use the
same SMI command value ICH9_APM_CNT_CPU_HOTPLUG (= 4) for unplug.
In QEMU, the macro is called OVMF_CPUHP_SMI_CMD.
(3) s/hot unplug/hot-unplug/.
+// "etc/smi/supported-features" and "etc/smi/requested-features" fw_cfg files.
+// Is only negotiated alongside ICH9_LPC_SMI_F_CPU_HOTPLUG.
(4) Please drop the last sentence (see more on it below).
+//
+#define ICH9_LPC_SMI_F_CPU_HOT_UNPLUG BIT2
+
//
// Provides a scratch buffer (allocated in EfiReservedMemoryType type memory)
// for the S3 boot script fragment to write to and read from.
@@ -112,7 +119,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, on CPU hot-unplug
+ // and nothing else.
//
RequestedFeaturesMask = ICH9_LPC_SMI_F_BROADCAST;
if (!MemEncryptSevIsEnabled ()) {
(5) Please spell out the full expression "SMI on CPU hot-unplug".
@@ -120,8 +128,18 @@ 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;
+
+ if (!(mSmiFeatures & ICH9_LPC_SMI_F_CPU_HOTPLUG) &&
+ (mSmiFeatures & ICH9_LPC_SMI_F_CPU_HOT_UNPLUG)) {
+ DEBUG ((DEBUG_WARN, "%a CPU host-features %Lx, requested mask %Lx\n",
+ __FUNCTION__, mSmiFeatures, RequestedFeaturesMask));
+
+ mSmiFeatures &= ~ICH9_LPC_SMI_F_CPU_HOT_UNPLUG;
+ }
+
QemuFwCfgSelectItem (mRequestedFeaturesItem);
QemuFwCfgWriteBytes (sizeof mSmiFeatures, &mSmiFeatures);
(6) Please drop this hunk. We don't try to be smarter than QEMU, in
general, whenever we perform feature negotiation.
Also, AFAICS, we will do the hotplug (and now hot-unplug) even if it wasn't
negotiated?
For example, the pre-patch code doesn't attempt to notice if QEMU
acknowledges ICH9_LPC_SMI_F_CPU_HOTPLUG but not ICH9_LPC_SMI_F_BROADCAST.
@@ -162,8 +180,9 @@ NegotiateSmiFeatures (
if ((mSmiFeatures & ICH9_LPC_SMI_F_CPU_HOTPLUG) == 0) {
DEBUG ((DEBUG_INFO, "%a: CPU hotplug not negotiated\n", __FUNCTION__));
} else {
- DEBUG ((DEBUG_INFO, "%a: CPU hotplug with SMI negotiated\n",
- __FUNCTION__));
+ DEBUG ((DEBUG_INFO, "%a: CPU hotplug%s with SMI negotiated\n",
+ __FUNCTION__,
+ (mSmiFeatures & ICH9_LPC_SMI_F_CPU_HOT_UNPLUG) ? ", unplug" : ""));
}
//
(7) Rather than combining these two in a common debug message, please
just add a separate "if" that follows the whole pattern seen with
ICH9_LPC_SMI_F_CPU_HOTPLUG. Thus, for each feature bit we care about,
we'll have a dedicated log message, saying yes or no.
Acking this set (and the ones down-thread.) Will fix.
Thanks
Ankur
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71093): https://edk2.groups.io/g/devel/message/71093
Mute This Topic: https://groups.io/mt/80199973/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
