[edk2-devel] [PATCH v9 02/10] OvmfPkg/CpuHotplugSmm: collect hot-unplug events

2021-03-11 Thread Ankur Arora
Process fw_remove events in QemuCpuhpCollectApicIds(), and collect APIC IDs
and QEMU CPU Selectors for CPUs being hot-unplugged.

In addition, we now ignore CPUs which only have remove set. These
CPUs haven't been processed by OSPM yet.

This is based on the QEMU hot-unplug protocol documented here:
  
https://lore.kernel.org/qemu-devel/20201204170939.1815522-3-imamm...@redhat.com/

Cc: Laszlo Ersek 
Cc: Jordan Justen 
Cc: Ard Biesheuvel 
Cc: Igor Mammedov 
Cc: Boris Ostrovsky 
Cc: Aaron Young 
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
Signed-off-by: Ankur Arora 
---

Notes:
Addresses the following comments from v8:
  (1) Fix commit message to mention that we collect cpu-selectors as well.
  (2,3,6) s/UnplugSelector/UnplugSelectors/ in CpuHotplug.c, QemuCpuhp.c
  (4) Fix comment above the declaration of the now renamed 
mToUnplugSelector.
  (5) Fix spacing around "||".
  (7) Fix QemuCpuCollectApicIds() comments to line up descriptions for
  ToUnplugSelectors and other params.
  (8) s/ExtendSel/ExtendSels/.
  (9) Add the (ExtendSels => ExtendIds) assert.
  (10) Fix the missing CurrentSelector++ bug.

 OvmfPkg/CpuHotplugSmm/QemuCpuhp.h |   1 +
 OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h |   1 +
 OvmfPkg/CpuHotplugSmm/CpuHotplug.c|  29 +--
 OvmfPkg/CpuHotplugSmm/QemuCpuhp.c | 101 +++---
 4 files changed, 93 insertions(+), 39 deletions(-)

diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h 
b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
index 8adaa0ad91f0..3e2c2192e1c0 100644
--- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
+++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
@@ -55,6 +55,7 @@ QemuCpuhpCollectApicIds (
   OUT APIC_ID  *PluggedApicIds,
   OUT UINT32   *PluggedCount,
   OUT APIC_ID  *ToUnplugApicIds,
+  OUT UINT32   *ToUnplugSelectors,
   OUT UINT32   *ToUnplugCount
   );
 
diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h 
b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
index a34a6d3fae61..2ec7a107a64d 100644
--- a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
+++ b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
@@ -34,6 +34,7 @@
 #define QEMU_CPUHP_STAT_ENABLEDBIT0
 #define QEMU_CPUHP_STAT_INSERT BIT1
 #define QEMU_CPUHP_STAT_REMOVE BIT2
+#define QEMU_CPUHP_STAT_FW_REMOVE  BIT4
 
 #define QEMU_CPUHP_RW_CMD_DATA   0x8
 
diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c 
b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index bf68fcd42914..ee1497b93140 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -45,13 +45,16 @@ STATIC CPU_HOT_PLUG_DATA *mCpuHotPlugData;
 // don't want to allocate SMRAM at OS runtime, and potentially fail (or
 // fragment the SMRAM map).
 //
-// These arrays provide room for ("possible CPU count" minus one) APIC IDs
-// each, as we don't expect every possible CPU to appear, or disappear, in a
-// single MMI. The numbers of used (populated) elements in the arrays are
+// The first array stores APIC IDs for hot-plug events, the second and the
+// third store APIC IDs and QEMU CPU Selectors (both indexed similarly) for
+// hot-unplug events. All of these provide room for "possible CPU count" minus
+// one elements as we don't expect every possible CPU to appear, or disappear,
+// in a single MMI. The numbers of used (populated) elements in the arrays are
 // determined on every MMI separately.
 //
 STATIC APIC_ID *mPluggedApicIds;
 STATIC APIC_ID *mToUnplugApicIds;
+STATIC UINT32  *mToUnplugSelectors;
 //
 // Address of the non-SMRAM reserved memory page that contains the Post-SMM Pen
 // for hot-added CPUs.
@@ -289,6 +292,7 @@ CpuHotplugMmi (
  mPluggedApicIds,
  &PluggedCount,
  mToUnplugApicIds,
+ mToUnplugSelectors,
  &ToUnplugCount
  );
   if (EFI_ERROR (Status)) {
@@ -333,7 +337,9 @@ CpuHotplugEntry (
   )
 {
   EFI_STATUS Status;
+  UINTN  Len;
   UINTN  Size;
+  UINTN  SizeSel;
 
   //
   // This module should only be included when SMM support is required.
@@ -387,8 +393,9 @@ CpuHotplugEntry (
   //
   // Allocate the data structures that depend on the possible CPU count.
   //
-  if (RETURN_ERROR (SafeUintnSub (mCpuHotPlugData->ArrayLength, 1, &Size)) ||
-  RETURN_ERROR (SafeUintnMult (sizeof (APIC_ID), Size, &Size))) {
+  if (RETURN_ERROR (SafeUintnSub (mCpuHotPlugData->ArrayLength, 1, &Len)) ||
+  RETURN_ERROR (SafeUintnMult (sizeof (APIC_ID), Len, &Size)) ||
+  RETURN_ERROR (SafeUintnMult (sizeof (UINT32), Len, &SizeSel))) {
 Status = EFI_ABORTED;
 DEBUG ((DEBUG_ERROR, "%a: invalid CPU_HOT_PLUG_DATA\n", __FUNCTION__));
 goto Fatal;
@@ -405,6 +412,12 @@ CpuHotplugEntry (
 DEBUG ((DEBUG_ERROR, "%a: MmAllocatePool(): %r\n", __FUNCTION__, Statu

Re: [edk2-devel] [PATCH v9 02/10] OvmfPkg/CpuHotplugSmm: collect hot-unplug events

2021-03-16 Thread Laszlo Ersek
On 03/12/21 07:26, Ankur Arora wrote:
> Process fw_remove events in QemuCpuhpCollectApicIds(), and collect APIC IDs
> and QEMU CPU Selectors for CPUs being hot-unplugged.
> 
> In addition, we now ignore CPUs which only have remove set. These
> CPUs haven't been processed by OSPM yet.
> 
> This is based on the QEMU hot-unplug protocol documented here:
>   
> https://lore.kernel.org/qemu-devel/20201204170939.1815522-3-imamm...@redhat.com/
> 
> Cc: Laszlo Ersek 
> Cc: Jordan Justen 
> Cc: Ard Biesheuvel 
> Cc: Igor Mammedov 
> Cc: Boris Ostrovsky 
> Cc: Aaron Young 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora 
> ---
> 
> Notes:
> Addresses the following comments from v8:
>   (1) Fix commit message to mention that we collect cpu-selectors as well.
>   (2,3,6) s/UnplugSelector/UnplugSelectors/ in CpuHotplug.c, QemuCpuhp.c
>   (4) Fix comment above the declaration of the now renamed 
> mToUnplugSelector.
>   (5) Fix spacing around "||".
>   (7) Fix QemuCpuCollectApicIds() comments to line up descriptions for
>   ToUnplugSelectors and other params.
>   (8) s/ExtendSel/ExtendSels/.
>   (9) Add the (ExtendSels => ExtendIds) assert.
>   (10) Fix the missing CurrentSelector++ bug.
> 
>  OvmfPkg/CpuHotplugSmm/QemuCpuhp.h |   1 +
>  OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h |   1 +
>  OvmfPkg/CpuHotplugSmm/CpuHotplug.c|  29 +--
>  OvmfPkg/CpuHotplugSmm/QemuCpuhp.c | 101 
> +++---
>  4 files changed, 93 insertions(+), 39 deletions(-)

Reviewed-by: Laszlo Ersek 

Thanks
Laszlo

> 
> diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h 
> b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
> index 8adaa0ad91f0..3e2c2192e1c0 100644
> --- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
> +++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
> @@ -55,6 +55,7 @@ QemuCpuhpCollectApicIds (
>OUT APIC_ID  *PluggedApicIds,
>OUT UINT32   *PluggedCount,
>OUT APIC_ID  *ToUnplugApicIds,
> +  OUT UINT32   *ToUnplugSelectors,
>OUT UINT32   *ToUnplugCount
>);
>  
> diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h 
> b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> index a34a6d3fae61..2ec7a107a64d 100644
> --- a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> +++ b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> @@ -34,6 +34,7 @@
>  #define QEMU_CPUHP_STAT_ENABLEDBIT0
>  #define QEMU_CPUHP_STAT_INSERT BIT1
>  #define QEMU_CPUHP_STAT_REMOVE BIT2
> +#define QEMU_CPUHP_STAT_FW_REMOVE  BIT4
>  
>  #define QEMU_CPUHP_RW_CMD_DATA   0x8
>  
> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c 
> b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> index bf68fcd42914..ee1497b93140 100644
> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> @@ -45,13 +45,16 @@ STATIC CPU_HOT_PLUG_DATA *mCpuHotPlugData;
>  // don't want to allocate SMRAM at OS runtime, and potentially fail (or
>  // fragment the SMRAM map).
>  //
> -// These arrays provide room for ("possible CPU count" minus one) APIC IDs
> -// each, as we don't expect every possible CPU to appear, or disappear, in a
> -// single MMI. The numbers of used (populated) elements in the arrays are
> +// The first array stores APIC IDs for hot-plug events, the second and the
> +// third store APIC IDs and QEMU CPU Selectors (both indexed similarly) for
> +// hot-unplug events. All of these provide room for "possible CPU count" 
> minus
> +// one elements as we don't expect every possible CPU to appear, or 
> disappear,
> +// in a single MMI. The numbers of used (populated) elements in the arrays 
> are
>  // determined on every MMI separately.
>  //
>  STATIC APIC_ID *mPluggedApicIds;
>  STATIC APIC_ID *mToUnplugApicIds;
> +STATIC UINT32  *mToUnplugSelectors;
>  //
>  // Address of the non-SMRAM reserved memory page that contains the Post-SMM 
> Pen
>  // for hot-added CPUs.
> @@ -289,6 +292,7 @@ CpuHotplugMmi (
>   mPluggedApicIds,
>   &PluggedCount,
>   mToUnplugApicIds,
> + mToUnplugSelectors,
>   &ToUnplugCount
>   );
>if (EFI_ERROR (Status)) {
> @@ -333,7 +337,9 @@ CpuHotplugEntry (
>)
>  {
>EFI_STATUS Status;
> +  UINTN  Len;
>UINTN  Size;
> +  UINTN  SizeSel;
>  
>//
>// This module should only be included when SMM support is required.
> @@ -387,8 +393,9 @@ CpuHotplugEntry (
>//
>// Allocate the data structures that depend on the possible CPU count.
>//
> -  if (RETURN_ERROR (SafeUintnSub (mCpuHotPlugData->ArrayLength, 1, &Size)) ||
> -  RETURN_ERROR (SafeUintnMult (sizeof (APIC_ID), Size, &Size))) {
> +  if (RETURN_ERROR (SafeUintnSub (mCpuHotPlugData->ArrayLength, 1, &Len)) ||
> +  RETURN_ERROR (SafeUintnMult (sizeof (APIC