Re: [PATCH v6 9/9] OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug

["Laszlo Ersek" <lersek@redhat.com>]

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
>