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 <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>
---
 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
+// with control value ICH9_APM_CNT_CPU_HOT_UNPLUG upon hot unplug", in the
+// "etc/smi/supported-features" and "etc/smi/requested-features" fw_cfg files.
+// Is only negotiated alongside ICH9_LPC_SMI_F_CPU_HOTPLUG.
+//
+#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 ()) {
@@ -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);
 
@@ -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" : ""));
   }
 
   //
-- 
2.9.3



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70878): https://edk2.groups.io/g/devel/message/70878
Mute This Topic: https://groups.io/mt/80199973/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

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.

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71025): https://edk2.groups.io/g/devel/message/71025
Mute This Topic: https://groups.io/mt/80199973/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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 <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>
>> ---
>>   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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 02/03/21 06:46, Ankur Arora wrote:
> On 2021-02-01 9:37 a.m., Laszlo Ersek wrote:

>> (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?

Yes, totally. We don't try to "evict" CpuHotplugSmm in case the related
features are not supported/offered by QEMU, we'll just leave
CpuHotplugSmm unused.

Here's why: the SMI feature negotiation interface is locked down at a
certain point; the negotiation of all of the feature bits needs to
happen centrally, in a common spot; and it would require a really quirky
solution in the firmware to let independent drivers negotiate *subsets*
of the features.

You have correctly determined that SmmControl2Dxe, the runtime DXE
driver that produces EFI_SMM_CONTROL2_PROTOCOL, has nothing much to do
with CPU hot-(un)plug. It's just that this is the driver that first
used, and therefore now *owns*, the SMI feature negotiation. (See commit
5ba203b54e59 ("OvmfPkg/SmmControl2Dxe: negotiate
ICH9_LPC_SMI_F_CPU_HOTPLUG", 2020-08-24).)

So, to reformulate your question/statement: the firmware will retain the
ability to do hot-(un)plug even if QEMU doesn't contain (or enable)
those particular features.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71114): https://edk2.groups.io/g/devel/message/71114
Mute This Topic: https://groups.io/mt/80199973/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 2021-02-03 12:45 p.m., Laszlo Ersek wrote:
> On 02/03/21 06:46, Ankur Arora wrote:
>> On 2021-02-01 9:37 a.m., Laszlo Ersek wrote:
> 
>>> (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?
> 
> Yes, totally. We don't try to "evict" CpuHotplugSmm in case the related
> features are not supported/offered by QEMU, we'll just leave
> CpuHotplugSmm unused.
> 
> Here's why: the SMI feature negotiation interface is locked down at a
> certain point; the negotiation of all of the feature bits needs to
> happen centrally, in a common spot; and it would require a really quirky
> solution in the firmware to let independent drivers negotiate *subsets*
> of the features.

Right, I see your point. Firmware doesn't really get to stand on
ceremony when HW asks it to do stuff.

Thanks
Ankur

> 
> You have correctly determined that SmmControl2Dxe, the runtime DXE
> driver that produces EFI_SMM_CONTROL2_PROTOCOL, has nothing much to do
> with CPU hot-(un)plug. It's just that this is the driver that first
> used, and therefore now *owns*, the SMI feature negotiation. (See commit
> 5ba203b54e59 ("OvmfPkg/SmmControl2Dxe: negotiate
> ICH9_LPC_SMI_F_CPU_HOTPLUG", 2020-08-24).)
> 
> So, to reformulate your question/statement: the firmware will retain the
> ability to do hot-(un)plug even if QEMU doesn't contain (or enable)
> those particular features.
> 
> Thanks
> Laszlo
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71133): https://edk2.groups.io/g/devel/message/71133
Mute This Topic: https://groups.io/mt/80199973/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 02/01/21 18:37, 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 <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>
>> ---
>>  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.

... obviously: don't drop the part where you set the new bit! :) Sorry,
"hunk" was not the correct term.

Thanks!
Laszlo

> 
> 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.
> 
> Thanks!
> Laszlo
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71026): https://edk2.groups.io/g/devel/message/71026
Mute This Topic: https://groups.io/mt/80199973/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 02/01/21 18:40, Laszlo Ersek wrote:
> On 02/01/21 18:37, 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 <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>
>>> ---
>>>  OvmfPkg/SmmControl2Dxe/SmiFeatures.c | 25 ++++++++++++++++++++++---
>>>  1 file changed, 22 insertions(+), 3 deletions(-)

[...]

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

(8) ... Please refresh the commit message accordingly.

> ... obviously: don't drop the part where you set the new bit! :) Sorry,
> "hunk" was not the correct term.

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71027): https://edk2.groups.io/g/devel/message/71027
Mute This Topic: https://groups.io/mt/80199973/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-