Re: [edk2-devel] [PATCH v9 10/10] OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug

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

On 03/12/21 07:26, 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 comments from v8:
>     
>     (1) Remove inconsistent comment style (and stray newline) around the newly
>     added ICH9_LPC_SMI_F_CPU_HOT_UNPLUG.
>     (2) Remove spurious empty line.
> 
>  OvmfPkg/SmmControl2Dxe/SmiFeatures.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
> index c9d875543205..3e2e61e4dbd0 100644
> --- a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
> +++ b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
> @@ -28,7 +28,12 @@
>  // "etc/smi/supported-features" and "etc/smi/requested-features" fw_cfg files.
>  //
>  #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

Per my v8 comment, this hunk is not supposed to change the amount of
vertical whitespace, in either direction. v8 added an empty line that
wasn't called for, and this version removes one, which is also unjustified.

But I'll fix that up when I merge this series (= soon).

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

>  //
>  // Provides a scratch buffer (allocated in EfiReservedMemoryType type memory)
>  // for the S3 boot script fragment to write to and read from.
> @@ -112,7 +117,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,6 +126,7 @@ 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;
>    QemuFwCfgSelectItem (mRequestedFeaturesItem);
> @@ -166,6 +173,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).
>