Re: [edk2-devel] [PATCH v6 4/9] OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
On 02/03/21 05:28, Ankur Arora wrote:
> On 2021-01-31 7:13 p.m., Laszlo Ersek wrote:
>> On 01/29/21 01:59, Ankur Arora wrote:
>>> +**/
>>> +STATIC
>>> +EFI_STATUS
>>> +UnplugCpus (
>>> + IN APIC_ID *ToUnplugApicIds,
>>> + IN UINT32 ToUnplugCount
>>> + )
>>> +{
>>> + EFI_STATUS Status;
>>> + UINT32 ToUnplugIdx;
>>> + UINTN ProcessorNum;
>>> +
>>> + ToUnplugIdx = 0;
>>> + while (ToUnplugIdx < ToUnplugCount) {
>>> + APIC_ID RemoveApicId;
>>> +
>>> + RemoveApicId = ToUnplugApicIds[ToUnplugIdx];
>>> +
>>> + //
>>> + // mCpuHotPlugData->ApicId maps ProcessorNum -> ApicId. Use it
>>> to find
>>> + // the ProcessorNum for the APIC ID to be removed.
>>> + //
>>> + for (ProcessorNum = 0;
>>> + ProcessorNum < mCpuHotPlugData->ArrayLength;
>>> + ProcessorNum++) {
>>> + if (mCpuHotPlugData->ApicId[ProcessorNum] == RemoveApicId) {
>>> + break;
>>> + }
>>> + }
>>> +
>>> + //
>>> + // Ignore the unplug if APIC ID not found
>>> + //
>>> + if (ProcessorNum == mCpuHotPlugData->ArrayLength) {
>>> + DEBUG ((DEBUG_INFO, "%a: did not find APIC ID " FMT_APIC_ID
>>> + " to unplug\n", __FUNCTION__, RemoveApicId));
>>
>> (2) Please use DEBUG_VERBOSE here.
>>
>> (I agree that we should have *one* DEBUG_INFO message that relates to
>> the removal of an individual processor; however, I think we should emit
>> that message when we finally signal QEMU to eject the processor.)
>
> Based on our discussion around establishing the correspondence between
> The ProcessorNum, APIC-ID and CPU selector, I'll change this to
> DEBUG_VERBOSE and add a new DEBUG_INFO print after successfully
> putting it in the APICIdMap.
Thanks!
[...]
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71112): https://edk2.groups.io/g/devel/message/71112
Mute This Topic: https://groups.io/mt/80199964/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v6 4/9] OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
On 2021-01-31 7:13 p.m., Laszlo Ersek wrote:
On 01/29/21 01:59, Ankur Arora wrote:
Introduce UnplugCpus() which maps each APIC ID being unplugged
onto the hardware ID of the processor and informs PiSmmCpuDxeSmm
of removal by calling EFI_SMM_CPU_SERVICE_PROTOCOL.RemoveProcessor().
With this change we handle the first phase of unplug where we collect
the CPUs that need to be unplugged and mark them for removal in SMM
data structures.
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
---
OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 84 ++
1 file changed, 84 insertions(+)
diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index 05b1f8cb63a6..70d69f6ed65b 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -188,6 +188,88 @@ RevokeNewSlot:
}
/**
+ Process to be hot-unplugged CPUs, per QemuCpuhpCollectApicIds().
+
+ For each such CPU, report the CPU to PiSmmCpuDxeSmm via
+ EFI_SMM_CPU_SERVICE_PROTOCOL. If the to be hot-unplugged CPU is
+ unknown, skip it silently.
+
+ @param[in] ToUnplugApicIdsThe APIC IDs of the CPUs that are about to be
+hot-unplugged.
+
+ @param[in] ToUnplugCount The number of filled-in APIC IDs in
+ToUnplugApicIds.
+
+ @retval EFI_SUCCESS Known APIC IDs have been removed from SMM data
+structures.
+
+ @return Error codes propagated from
+mMmCpuService->RemoveProcessor().
+
(1) Please drop this empty line (just before the '**/').
+**/
+STATIC
+EFI_STATUS
+UnplugCpus (
+ IN APIC_ID *ToUnplugApicIds,
+ IN UINT32 ToUnplugCount
+ )
+{
+ EFI_STATUS Status;
+ UINT32 ToUnplugIdx;
+ UINTN ProcessorNum;
+
+ ToUnplugIdx = 0;
+ while (ToUnplugIdx < ToUnplugCount) {
+APIC_IDRemoveApicId;
+
+RemoveApicId = ToUnplugApicIds[ToUnplugIdx];
+
+//
+// mCpuHotPlugData->ApicId maps ProcessorNum -> ApicId. Use it to find
+// the ProcessorNum for the APIC ID to be removed.
+//
+for (ProcessorNum = 0;
+ ProcessorNum < mCpuHotPlugData->ArrayLength;
+ ProcessorNum++) {
+ if (mCpuHotPlugData->ApicId[ProcessorNum] == RemoveApicId) {
+break;
+ }
+}
+
+//
+// Ignore the unplug if APIC ID not found
+//
+if (ProcessorNum == mCpuHotPlugData->ArrayLength) {
+ DEBUG ((DEBUG_INFO, "%a: did not find APIC ID " FMT_APIC_ID
+ " to unplug\n", __FUNCTION__, RemoveApicId));
(2) Please use DEBUG_VERBOSE here.
(I agree that we should have *one* DEBUG_INFO message that relates to
the removal of an individual processor; however, I think we should emit
that message when we finally signal QEMU to eject the processor.)
Based on our discussion around establishing the correspondence between
The ProcessorNum, APIC-ID and CPU selector, I'll change this to
DEBUG_VERBOSE and add a new DEBUG_INFO print after successfully
putting it in the APICIdMap.
(3) Please un-indent ("outdent"?) the second line by two spaces.
+ ToUnplugIdx++;
+ continue;
+}
+
+//
+// Mark ProcessorNum for removal from SMM data structures
+//
+Status = mMmCpuService->RemoveProcessor (mMmCpuService, ProcessorNum);
+
(4) It would be more idiomatic to remove this empty line (between Status
assignment and check).
+if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: RemoveProcessor(" FMT_APIC_ID "): %r\n",
+__FUNCTION__, RemoveApicId, Status));
+ goto Fatal;
(5) Please just "return Status" here, and drop the "Fatal" label.
+}
+
+ToUnplugIdx++;
+ }
+
+ //
+ // We've removed this set of APIC IDs from SMM data structures.
+ //
+ return EFI_SUCCESS;
+
+Fatal:
+ return Status;
+}
+
+/**
CPU Hotplug MMI handler function.
This is a root MMI handler.
@@ -303,6 +385,8 @@ CpuHotplugMmi (
if (PluggedCount > 0) {
Status = ProcessHotAddedCpus (mPluggedApicIds, PluggedCount);
+ } else if (ToUnplugCount > 0) {
+Status = UnplugCpus (mToUnplugApicIds, ToUnplugCount);
}
if (EFI_ERROR(Status)) {
(6) Hmm... What's the reason for the exclusivity?
Why is the following not better:
if (PluggedCount > 0) {
Status = ProcessHotAddedCpus (mPluggedApicIds, PluggedCount);
if (EFI_ERROR (Status)) {
goto Fatal;
}
}
if (ToUnplugCount > 0) {
Status = UnplugCpus (mToUnplugApicIds, ToUnplugCount);
if (EFI_ERROR (Status)) {
goto Fatal;
}
}
QemuCpuhpCollectApicIds() intentionally populates both arrays in a
single go. As I suggested earlier:
https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg06711.html
msgid:
[...] please handle
Re: [edk2-devel] [PATCH v6 4/9] OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
On 01/29/21 01:59, Ankur Arora wrote:
> Introduce UnplugCpus() which maps each APIC ID being unplugged
> onto the hardware ID of the processor and informs PiSmmCpuDxeSmm
> of removal by calling EFI_SMM_CPU_SERVICE_PROTOCOL.RemoveProcessor().
>
> With this change we handle the first phase of unplug where we collect
> the CPUs that need to be unplugged and mark them for removal in SMM
> data structures.
>
> 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
> ---
> OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 84
> ++
> 1 file changed, 84 insertions(+)
>
> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> index 05b1f8cb63a6..70d69f6ed65b 100644
> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> @@ -188,6 +188,88 @@ RevokeNewSlot:
> }
>
> /**
> + Process to be hot-unplugged CPUs, per QemuCpuhpCollectApicIds().
> +
> + For each such CPU, report the CPU to PiSmmCpuDxeSmm via
> + EFI_SMM_CPU_SERVICE_PROTOCOL. If the to be hot-unplugged CPU is
> + unknown, skip it silently.
> +
> + @param[in] ToUnplugApicIdsThe APIC IDs of the CPUs that are about to be
> +hot-unplugged.
> +
> + @param[in] ToUnplugCount The number of filled-in APIC IDs in
> +ToUnplugApicIds.
> +
> + @retval EFI_SUCCESS Known APIC IDs have been removed from SMM
> data
> +structures.
> +
> + @return Error codes propagated from
> +mMmCpuService->RemoveProcessor().
> +
(1) Please drop this empty line (just before the '**/').
> +**/
> +STATIC
> +EFI_STATUS
> +UnplugCpus (
> + IN APIC_ID *ToUnplugApicIds,
> + IN UINT32 ToUnplugCount
> + )
> +{
> + EFI_STATUS Status;
> + UINT32 ToUnplugIdx;
> + UINTN ProcessorNum;
> +
> + ToUnplugIdx = 0;
> + while (ToUnplugIdx < ToUnplugCount) {
> +APIC_IDRemoveApicId;
> +
> +RemoveApicId = ToUnplugApicIds[ToUnplugIdx];
> +
> +//
> +// mCpuHotPlugData->ApicId maps ProcessorNum -> ApicId. Use it to find
> +// the ProcessorNum for the APIC ID to be removed.
> +//
> +for (ProcessorNum = 0;
> + ProcessorNum < mCpuHotPlugData->ArrayLength;
> + ProcessorNum++) {
> + if (mCpuHotPlugData->ApicId[ProcessorNum] == RemoveApicId) {
> +break;
> + }
> +}
> +
> +//
> +// Ignore the unplug if APIC ID not found
> +//
> +if (ProcessorNum == mCpuHotPlugData->ArrayLength) {
> + DEBUG ((DEBUG_INFO, "%a: did not find APIC ID " FMT_APIC_ID
> + " to unplug\n", __FUNCTION__, RemoveApicId));
(2) Please use DEBUG_VERBOSE here.
(I agree that we should have *one* DEBUG_INFO message that relates to
the removal of an individual processor; however, I think we should emit
that message when we finally signal QEMU to eject the processor.)
(3) Please un-indent ("outdent"?) the second line by two spaces.
> + ToUnplugIdx++;
> + continue;
> +}
> +
> +//
> +// Mark ProcessorNum for removal from SMM data structures
> +//
> +Status = mMmCpuService->RemoveProcessor (mMmCpuService, ProcessorNum);
> +
(4) It would be more idiomatic to remove this empty line (between Status
assignment and check).
> +if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "%a: RemoveProcessor(" FMT_APIC_ID "): %r\n",
> +__FUNCTION__, RemoveApicId, Status));
> + goto Fatal;
(5) Please just "return Status" here, and drop the "Fatal" label.
> +}
> +
> +ToUnplugIdx++;
> + }
> +
> + //
> + // We've removed this set of APIC IDs from SMM data structures.
> + //
> + return EFI_SUCCESS;
> +
> +Fatal:
> + return Status;
> +}
> +
> +/**
>CPU Hotplug MMI handler function.
>
>This is a root MMI handler.
> @@ -303,6 +385,8 @@ CpuHotplugMmi (
>
>if (PluggedCount > 0) {
> Status = ProcessHotAddedCpus (mPluggedApicIds, PluggedCount);
> + } else if (ToUnplugCount > 0) {
> +Status = UnplugCpus (mToUnplugApicIds, ToUnplugCount);
>}
>
>if (EFI_ERROR(Status)) {
>
(6) Hmm... What's the reason for the exclusivity?
Why is the following not better:
if (PluggedCount > 0) {
Status = ProcessHotAddedCpus (mPluggedApicIds, PluggedCount);
if (EFI_ERROR (Status)) {
goto Fatal;
}
}
if (ToUnplugCount > 0) {
Status = UnplugCpus (mToUnplugApicIds, ToUnplugCount);
if (EFI_ERROR (Status)) {
goto Fatal;
}
}
QemuCpuhpCollectApicIds() intentionally populates both arrays in a
single go. As I suggested earlier:
https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg06711.html
msgid:
> [...] please handle plugs first, for which unused slots in
> mCpuHotPlugData.ApicId will
Re: [edk2-devel] [PATCH v6 4/9] OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
On 01/29/21 01:59, Ankur Arora wrote: > Introduce UnplugCpus() which maps each APIC ID being unplugged > onto the hardware ID of the processor and informs PiSmmCpuDxeSmm > of removal by calling EFI_SMM_CPU_SERVICE_PROTOCOL.RemoveProcessor(). > > With this change we handle the first phase of unplug where we collect > the CPUs that need to be unplugged and mark them for removal in SMM > data structures. > > 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 > --- > OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 84 > ++ > 1 file changed, 84 insertions(+) I intend to continue the review at this patch, next week. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#70919): https://edk2.groups.io/g/devel/message/70919 Mute This Topic: https://groups.io/mt/80199964/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v6 4/9] OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
Introduce UnplugCpus() which maps each APIC ID being unplugged
onto the hardware ID of the processor and informs PiSmmCpuDxeSmm
of removal by calling EFI_SMM_CPU_SERVICE_PROTOCOL.RemoveProcessor().
With this change we handle the first phase of unplug where we collect
the CPUs that need to be unplugged and mark them for removal in SMM
data structures.
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
---
OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 84 ++
1 file changed, 84 insertions(+)
diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index 05b1f8cb63a6..70d69f6ed65b 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -188,6 +188,88 @@ RevokeNewSlot:
}
/**
+ Process to be hot-unplugged CPUs, per QemuCpuhpCollectApicIds().
+
+ For each such CPU, report the CPU to PiSmmCpuDxeSmm via
+ EFI_SMM_CPU_SERVICE_PROTOCOL. If the to be hot-unplugged CPU is
+ unknown, skip it silently.
+
+ @param[in] ToUnplugApicIdsThe APIC IDs of the CPUs that are about to be
+hot-unplugged.
+
+ @param[in] ToUnplugCount The number of filled-in APIC IDs in
+ToUnplugApicIds.
+
+ @retval EFI_SUCCESS Known APIC IDs have been removed from SMM data
+structures.
+
+ @return Error codes propagated from
+mMmCpuService->RemoveProcessor().
+
+**/
+STATIC
+EFI_STATUS
+UnplugCpus (
+ IN APIC_ID *ToUnplugApicIds,
+ IN UINT32 ToUnplugCount
+ )
+{
+ EFI_STATUS Status;
+ UINT32 ToUnplugIdx;
+ UINTN ProcessorNum;
+
+ ToUnplugIdx = 0;
+ while (ToUnplugIdx < ToUnplugCount) {
+APIC_IDRemoveApicId;
+
+RemoveApicId = ToUnplugApicIds[ToUnplugIdx];
+
+//
+// mCpuHotPlugData->ApicId maps ProcessorNum -> ApicId. Use it to find
+// the ProcessorNum for the APIC ID to be removed.
+//
+for (ProcessorNum = 0;
+ ProcessorNum < mCpuHotPlugData->ArrayLength;
+ ProcessorNum++) {
+ if (mCpuHotPlugData->ApicId[ProcessorNum] == RemoveApicId) {
+break;
+ }
+}
+
+//
+// Ignore the unplug if APIC ID not found
+//
+if (ProcessorNum == mCpuHotPlugData->ArrayLength) {
+ DEBUG ((DEBUG_INFO, "%a: did not find APIC ID " FMT_APIC_ID
+ " to unplug\n", __FUNCTION__, RemoveApicId));
+ ToUnplugIdx++;
+ continue;
+}
+
+//
+// Mark ProcessorNum for removal from SMM data structures
+//
+Status = mMmCpuService->RemoveProcessor (mMmCpuService, ProcessorNum);
+
+if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: RemoveProcessor(" FMT_APIC_ID "): %r\n",
+__FUNCTION__, RemoveApicId, Status));
+ goto Fatal;
+}
+
+ToUnplugIdx++;
+ }
+
+ //
+ // We've removed this set of APIC IDs from SMM data structures.
+ //
+ return EFI_SUCCESS;
+
+Fatal:
+ return Status;
+}
+
+/**
CPU Hotplug MMI handler function.
This is a root MMI handler.
@@ -303,6 +385,8 @@ CpuHotplugMmi (
if (PluggedCount > 0) {
Status = ProcessHotAddedCpus (mPluggedApicIds, PluggedCount);
+ } else if (ToUnplugCount > 0) {
+Status = UnplugCpus (mToUnplugApicIds, ToUnplugCount);
}
if (EFI_ERROR(Status)) {
--
2.9.3
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70877): https://edk2.groups.io/g/devel/message/70877
Mute This Topic: https://groups.io/mt/80199964/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
