Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled

[Laszlo Ersek]

On 11/27/20 16:07, Igor Mammedov wrote:
> On Fri, 27 Nov 2020 15:48:34 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:

>> The firmware logic needs to be aware of is_removing though, at least
>> understand the existence of this bit, as the "get pending" command
>> will report such CPUs too that only have is_removing set. Shouldn't
>> be a problem, we just have to recognize it.
>
> firmware shouldn't see bit #2 normally, it's cleared in AML during
> CSCN, right after remove Notify is sent to OSPM. I don't see a reason
> for firmware to use it, I'd just mask it out on firmware side if it
> messes logic.
>
> potentially if we have concurrent plug/unplug for several CPUs,
> firmware might see bit #2 which it should ignore, it's for OSPM
> consumption only.

Yes, that's what I meant.

Currently, inside the scanning loop of the QemuCpuhpCollectApicIds()
function in "OvmfPkg/CpuHotplugSmm/QemuCpuhp.c", there is no branch that
simply skips a CPU that was reported by QEMU_CPUHP_CMD_GET_PENDING. Now,
such a branch will be necessary.

This is what I mean (just for illustration):

$ git diff -b -U5

> diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h 
> b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> index a34a6d3fae61..ddeef047c517 100644
> --- a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> +++ b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> @@ -32,10 +32,11 @@
>
>  #define QEMU_CPUHP_R_CPU_STAT                0x4
>  #define QEMU_CPUHP_STAT_ENABLED                BIT0
>  #define QEMU_CPUHP_STAT_INSERT                 BIT1
>  #define QEMU_CPUHP_STAT_REMOVE                 BIT2
> +#define QEMU_CPUHP_STAT_FIRMWARE_REMOVE        BIT4
>
>  #define QEMU_CPUHP_RW_CMD_DATA               0x8
>
>  #define QEMU_CPUHP_W_CPU_SEL                 0x0
>
> diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c 
> b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
> index 8d4a6693c8d6..9bff31628e61 100644
> --- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
> +++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
> @@ -258,35 +258,44 @@ QemuCpuhpCollectApicIds (
>        DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: insert\n", 
> __FUNCTION__,
>          CurrentSelector));
>
>        ExtendIds   = PluggedApicIds;
>        ExtendCount = PluggedCount;
> -    } else if ((CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) {
> -      DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: remove\n", 
> __FUNCTION__,
> -        CurrentSelector));
> +    } else if ((CpuStatus & QEMU_CPUHP_STAT_FIRMWARE_REMOVE) != 0) {
> +      DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: firmware remove\n",
> +        __FUNCTION__, CurrentSelector));
>
>        ExtendIds   = ToUnplugApicIds;
>        ExtendCount = ToUnplugCount;
> +    } else if ((CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) {
> +      DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: remove\n", 
> __FUNCTION__,
> +        CurrentSelector));
> +
> +      ExtendIds   = NULL;
> +      ExtendCount = NULL;
>      } else {
>        DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: no event\n",
>          __FUNCTION__, CurrentSelector));
>        break;
>      }
>
> +    ASSERT ((ExtendIds == NULL) == (ExtendCount == NULL));
> +    if (ExtendIds != NULL) {
>        //
> -    // Save the APIC ID of the CPU with the pending event, to the 
> corresponding
> -    // APIC ID array.
> +      // Save the APIC ID of the CPU with the pending event, to the
> +      // corresponding APIC ID array.
>        //
>        if (*ExtendCount == ApicIdCount) {
>          DEBUG ((DEBUG_ERROR, "%a: APIC ID array too small\n", __FUNCTION__));
>          return EFI_BUFFER_TOO_SMALL;
>        }
>        QemuCpuhpWriteCommand (MmCpuIo, QEMU_CPUHP_CMD_GET_ARCH_ID);
>        NewApicId = QemuCpuhpReadCommandData (MmCpuIo);
>        DEBUG ((DEBUG_VERBOSE, "%a: ApicId=" FMT_APIC_ID "\n", __FUNCTION__,
>          NewApicId));
>        ExtendIds[(*ExtendCount)++] = NewApicId;
> +    }
>
>      //
>      // We've processed the CPU with (known) pending events, but we must never
>      // clear events. Therefore we need to advance past this CPU manually;
>      // otherwise, QEMU_CPUHP_CMD_GET_PENDING would stick to the currently

Thanks
Laszlo