On 11/27/20 16:07, Igor Mammedov wrote:
> On Fri, 27 Nov 2020 15:48:34 +0100
> Laszlo Ersek <ler...@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
