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 <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>
---
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] ToUnplugApicIds The 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_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));
+ 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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 <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> > --- > 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/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 <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>
> ---
> 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] ToUnplugApicIds The 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_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.)
(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: <a92b50df-f693-ebda-e549-7bc9e6d2d7a5@redhat.com>
> [...] please handle plugs first, for which unused slots in
> mCpuHotPlugData.ApicId will be populated, and *then* handle removals
> (in the same invocation of CpuHotplugMmi()).
Did that turn out as unviable (the "same invocation of CpuHotplugMmi()"
part)?
(7) As a side note, addressing point (6) above would allow you to
address my point (13) from the v5 patch#1 thread, too; i.e., nesting the
Status check under (PluggedCount > 0).
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70998): https://edk2.groups.io/g/devel/message/70998
Mute This Topic: https://groups.io/mt/80199964/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 <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>
>> ---
>> 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] ToUnplugApicIds The 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_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.
>
>
> (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: <a92b50df-f693-ebda-e549-7bc9e6d2d7a5@redhat.com>
>
>> [...] please handle plugs first, for which unused slots in
>> mCpuHotPlugData.ApicId will be populated, and *then* handle removals
>> (in the same invocation of CpuHotplugMmi()).
>
> Did that turn out as unviable (the "same invocation of CpuHotplugMmi()"
> part)?
No I had some confusion while looking at the underlying AddProcessor() and
RemoveProcessor() logic.
Looking at it again, it should work. Will fix.
Also acking the rest of your comments here.
Thanks
Ankur
>
>
> (7) As a side note, addressing point (6) above would allow you to
> address my point (13) from the v5 patch#1 thread, too; i.e., nesting the
> Status check under (PluggedCount > 0).
>
> Thanks!
> Laszlo
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71090): https://edk2.groups.io/g/devel/message/71090
Mute This Topic: https://groups.io/mt/80199964/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.