Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On 2020-11-30 8:58 a.m., Laszlo Ersek wrote: On 11/28/20 00:48, Ankur Arora wrote: It is possible that there are CPUs with bits for both is_inserting and is_removing. In that case QemuCpuhpCollectApicIds() would put them in the PluggedApicIds array and the unplug eventually happens in the next firmware invocation. If a CPU has both is_inserting and fw_remove set, the firmware processes the hotplug in that invocation and the unplug happens whenever the OSPM triggers the firmware next. If these corner cases will actually work (I'm somewhat doubtful), that will be really great. Heh, yeah. That's a big if. I'll see if I can hit it in my testing. Thanks Ankur
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On 11/28/20 01:43, Ankur Arora wrote: > On 2020-11-27 7:19 a.m., Laszlo Ersek wrote: >> On 11/27/20 05:10, Ankur Arora wrote: >> >>> Yeah I was wondering what would happen for simultaneous hot add and >>> remove. >>> I guess we would always do remove first and then the add, unless we hit >>> the break due to max_cpus_per_pass and switch to hot-add mode. >> >> Considering the firmware only, I disagree with remove-then-add. >> >> EFI_SMM_CPU_SERVICE_PROTOCOL.AddProcessor() and >> EFI_SMM_CPU_SERVICE_PROTOCOL.RemoveProcessor() (implemented in >> SmmAddProcessor() and SmmRemoveProcessor() in >> "UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c", respectively) only mark the >> processors for addition/removal. The actual processing is done only >> later, in BSPHandler() --> SmmCpuUpdate(), when "all SMI handlers are >> finished" (see the comment in SmmRemoveProcessor()). >> >> Consequently, I would not suggest replacing a valid APIC ID in a >> particular mCpuHotPlugData.ApicId[Index] slot with INVALID_APIC_ID >> (corresponding to the unplug operation), and then possibly replacing >> INVALID_APIC_ID in the *same slot* with the APIC ID of the newly plugged >> CPU, in the exact same SMI invocation (= in the same execution of >> CpuHotplugMmi()). That might cause some component in edk2 to see the >> APIC ID in mCpuHotPlugData.ApicId[Index] to change from one valid ACPI >> ID to another valid APIC ID, and I don't even want to think about what >> kind of mess that could cause. > > Shudders. > >> >> So no, please handle plugs first, for which unused slots in >> mCpuHotPlugData.ApicId will be populated, and *then* handle removals (in >> the same invocation of CpuHotplugMmi()). > > Yeah, that ordering makes complete sense. > >> >> By the way, for unplug, you will not have to re-set >> mCpuHotPlugData.ApicId[Index] to INVALID_APIC_ID, as >> SmmRemoveProcessor() does that internally. You just have to locate the >> Index for the APIC ID being removed, for calling >> EFI_SMM_CPU_SERVICE_PROTOCOL.RemoveProcessor(). > > Right. The hotplug is more involved (given the need to pen the new CPU) > but for the unplug, AFAICS all the actual handling for removal is in > .RemoveProcessor() and at SMI exit in SmmCpuUpdate(). Yes, I got the same impression (without having tried to implement it, of course). Laszlo
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On 11/28/20 00:48, Ankur Arora wrote: > It is possible that there are CPUs with bits for both is_inserting and > is_removing. In that case QemuCpuhpCollectApicIds() would put them in the > PluggedApicIds array and the unplug eventually happens in the next > firmware invocation. > > If a CPU has both is_inserting and fw_remove set, the firmware processes > the > hotplug in that invocation and the unplug happens whenever the OSPM > triggers > the firmware next. If these corner cases will actually work (I'm somewhat doubtful), that will be really great. Thanks! Laszlo
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On 2020-11-27 7:19 a.m., Laszlo Ersek wrote: On 11/27/20 05:10, Ankur Arora wrote: Yeah I was wondering what would happen for simultaneous hot add and remove. I guess we would always do remove first and then the add, unless we hit the break due to max_cpus_per_pass and switch to hot-add mode. Considering the firmware only, I disagree with remove-then-add. EFI_SMM_CPU_SERVICE_PROTOCOL.AddProcessor() and EFI_SMM_CPU_SERVICE_PROTOCOL.RemoveProcessor() (implemented in SmmAddProcessor() and SmmRemoveProcessor() in "UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c", respectively) only mark the processors for addition/removal. The actual processing is done only later, in BSPHandler() --> SmmCpuUpdate(), when "all SMI handlers are finished" (see the comment in SmmRemoveProcessor()). Consequently, I would not suggest replacing a valid APIC ID in a particular mCpuHotPlugData.ApicId[Index] slot with INVALID_APIC_ID (corresponding to the unplug operation), and then possibly replacing INVALID_APIC_ID in the *same slot* with the APIC ID of the newly plugged CPU, in the exact same SMI invocation (= in the same execution of CpuHotplugMmi()). That might cause some component in edk2 to see the APIC ID in mCpuHotPlugData.ApicId[Index] to change from one valid ACPI ID to another valid APIC ID, and I don't even want to think about what kind of mess that could cause. Shudders. So no, please handle plugs first, for which unused slots in mCpuHotPlugData.ApicId will be populated, and *then* handle removals (in the same invocation of CpuHotplugMmi()). Yeah, that ordering makes complete sense. By the way, for unplug, you will not have to re-set mCpuHotPlugData.ApicId[Index] to INVALID_APIC_ID, as SmmRemoveProcessor() does that internally. You just have to locate the Index for the APIC ID being removed, for calling EFI_SMM_CPU_SERVICE_PROTOCOL.RemoveProcessor(). Right. The hotplug is more involved (given the need to pen the new CPU) but for the unplug, AFAICS all the actual handling for removal is in .RemoveProcessor() and at SMI exit in SmmCpuUpdate(). Thanks Ankur Thanks Laszlo
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On 2020-11-27 3:47 a.m., Igor Mammedov wrote:
On Thu, 26 Nov 2020 20:10:59 -0800
Ankur Arora wrote:
On 2020-11-26 12:38 p.m., Igor Mammedov wrote:
On Thu, 26 Nov 2020 12:17:27 +0100
Laszlo Ersek wrote:
On 11/24/20 13:25, Igor Mammedov wrote:
If firmware negotiates ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT feature,
OSPM on CPU eject will set bit #4 in CPU hotplug block for to be
ejected CPU to mark it for removal by firmware and trigger SMI
upcall to let firmware do actual eject.
Signed-off-by: Igor Mammedov
---
PS:
- abuse 5.1 machine type for now to turn off unplug feature
(it will be moved to 5.2 machine type once new merge window is open)
---
include/hw/acpi/cpu.h | 2 ++
docs/specs/acpi_cpu_hotplug.txt | 11 +--
hw/acpi/cpu.c | 18 --
hw/i386/acpi-build.c| 5 +
hw/i386/pc.c| 1 +
hw/isa/lpc_ich9.c | 2 +-
6 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
index 0eeedaa491..999caaf510 100644
--- a/include/hw/acpi/cpu.h
+++ b/include/hw/acpi/cpu.h
@@ -22,6 +22,7 @@ typedef struct AcpiCpuStatus {
uint64_t arch_id;
bool is_inserting;
bool is_removing;
+bool fw_remove;
uint32_t ost_event;
uint32_t ost_status;
} AcpiCpuStatus;
@@ -50,6 +51,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
typedef struct CPUHotplugFeatures {
bool acpi_1_compatible;
bool has_legacy_cphp;
+bool fw_unplugs_cpu;
const char *smi_path;
} CPUHotplugFeatures;
diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
index 9bb22d1270..f68ef6e06c 100644
--- a/docs/specs/acpi_cpu_hotplug.txt
+++ b/docs/specs/acpi_cpu_hotplug.txt
@@ -57,7 +57,11 @@ read access:
It's valid only when bit 0 is set.
2: Device remove event, used to distinguish device for which
no device eject request to OSPM was issued.
- 3-7: reserved and should be ignored by OSPM
+ 3: reserved and should be ignored by OSPM
+ 4: if set to 1, OSPM requests firmware to perform device eject,
+ firmware shall clear this event by writing 1 into it before
(1) s/clear this event/clear this event bit/
+ performing device eject.
(2) move the second and third lines ("firmware shall clear") over to
the write documentation, below? In particular:
+ 5-7: reserved and should be ignored by OSPM
[0x5-0x7] reserved
[0x8] Command data: (DWORD access)
contains 0 unless value last stored in 'Command field' is one of:
@@ -82,7 +86,10 @@ write access:
selected CPU device
3: if set to 1 initiates device eject, set by OSPM when it
triggers CPU device removal and calls _EJ0 method
-4-7: reserved, OSPM must clear them before writing to register
+4: if set to 1 OSPM hands over device eject to firmware,
+ Firmware shall issue device eject request as described above
+ (bit #3) and OSPM should not touch device eject bit (#3),
(3) it would be clearer if we documented the exact bit writing order
here:
- clear bit#4, *then* set bit#3 (two write accesses)
- versus clear bit#4 *and* set bit#3 (single access)
I was thinking that FW should not bother with clearing bit #4,
and QEMU should clear it when handling write to bit #3.
(it looks like I forgot to actually do that)
Why involve the firmware with bit #3 at all? If the firmware only reads bit #4
to detect fw_remove and then write (and thus reset) bit #4, isn't that
good enough?
That would needlessly complicate code on QEMU side and I don't want to
overload bit #4 with another semantics, and we already have bit #3 that
does eject. So unless there are issues with that, I'd stick to using
bit #3 for eject.
Sounds good.
+5-7: reserved, OSPM must clear them before writing to register
[0x5] Command field: (1 byte access)
value:
0: selects a CPU device with inserting/removing events and
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index f099b50927..09d2f20dae 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -71,6 +71,7 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr,
unsigned size)
val |= cdev->cpu ? 1 : 0;
val |= cdev->is_inserting ? 2 : 0;
val |= cdev->is_removing ? 4 : 0;
+val |= cdev->fw_remove ? 16 : 0;
trace_cpuhp_acpi_read_flags(cpu_st->selector, val);
break;
case ACPI_CPU_CMD_DATA_OFFSET_RW:
@@ -148,6 +149,8 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr,
uint64_t data,
hotplug_ctrl = qdev_get_hotplug_handler(dev);
hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
object_unparent(OBJECT(dev));
+
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On 2020-11-27 7:02 a.m., Laszlo Ersek wrote:
On 11/27/20 12:33, Igor Mammedov wrote:
On Thu, 26 Nov 2020 19:35:30 -0800
Ankur Arora wrote:
On 2020-11-26 4:46 a.m., Laszlo Ersek wrote:
On 11/26/20 11:24, Ankur Arora wrote:
On 2020-11-24 4:25 a.m., Igor Mammedov wrote:
If firmware negotiates ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT feature,
OSPM on CPU eject will set bit #4 in CPU hotplug block for to be
ejected CPU to mark it for removal by firmware and trigger SMI
upcall to let firmware do actual eject.
Signed-off-by: Igor Mammedov
---
PS:
- abuse 5.1 machine type for now to turn off unplug feature
(it will be moved to 5.2 machine type once new merge window is open)
---
include/hw/acpi/cpu.h | 2 ++
docs/specs/acpi_cpu_hotplug.txt | 11 +--
hw/acpi/cpu.c | 18 --
hw/i386/acpi-build.c | 5 +
hw/i386/pc.c | 1 +
hw/isa/lpc_ich9.c | 2 +-
6 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
index 0eeedaa491..999caaf510 100644
--- a/include/hw/acpi/cpu.h
+++ b/include/hw/acpi/cpu.h
@@ -22,6 +22,7 @@ typedef struct AcpiCpuStatus {
uint64_t arch_id;
bool is_inserting;
bool is_removing;
+ bool fw_remove;
uint32_t ost_event;
uint32_t ost_status;
} AcpiCpuStatus;
@@ -50,6 +51,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object
*owner,
typedef struct CPUHotplugFeatures {
bool acpi_1_compatible;
bool has_legacy_cphp;
+ bool fw_unplugs_cpu;
const char *smi_path;
} CPUHotplugFeatures;
diff --git a/docs/specs/acpi_cpu_hotplug.txt
b/docs/specs/acpi_cpu_hotplug.txt
index 9bb22d1270..f68ef6e06c 100644
--- a/docs/specs/acpi_cpu_hotplug.txt
+++ b/docs/specs/acpi_cpu_hotplug.txt
@@ -57,7 +57,11 @@ read access:
It's valid only when bit 0 is set.
2: Device remove event, used to distinguish device for which
no device eject request to OSPM was issued.
- 3-7: reserved and should be ignored by OSPM
+ 3: reserved and should be ignored by OSPM
+ 4: if set to 1, OSPM requests firmware to perform device
eject,
+ firmware shall clear this event by writing 1 into it
before
+ performing device eject> + 5-7: reserved and
should be ignored by OSPM
[0x5-0x7] reserved
[0x8] Command data: (DWORD access)
contains 0 unless value last stored in 'Command field' is
one of:
@@ -82,7 +86,10 @@ write access:
selected CPU device
3: if set to 1 initiates device eject, set by OSPM when it
triggers CPU device removal and calls _EJ0 method
- 4-7: reserved, OSPM must clear them before writing to
register
+ 4: if set to 1 OSPM hands over device eject to firmware,
+ Firmware shall issue device eject request as described
above
+ (bit #3) and OSPM should not touch device eject bit (#3),
+ 5-7: reserved, OSPM must clear them before writing to
register
[0x5] Command field: (1 byte access)
value:
0: selects a CPU device with inserting/removing events and
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index f099b50927..09d2f20dae 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -71,6 +71,7 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr
addr, unsigned size)
val |= cdev->cpu ? 1 : 0;
val |= cdev->is_inserting ? 2 : 0;
val |= cdev->is_removing ? 4 : 0;
+ val |= cdev->fw_remove ? 16 : 0;
I might be missing something but I don't see where cdev->fw_remove is being
set.
See just below, in the cpu_hotplug_wr() hunk. When bit#4 is written --
which happens through the ACPI code change --, fw_remove is inverted.
Thanks that makes sense. I was reading the AML building code all wrong.
We do set cdev->is_removing in acpi_cpu_unplug_request_cb() so AFAICS
we would always end up setting this bit:
val |= cdev->is_removing ? 4 : 0;
Also, if cdev->fw_remove and cdev->is_removing are both true, val would be
(4 | 16). I'm guessing that in that case the AML determines which case gets
handled but it might make sense to set just one of these?
"is_removing" is set directly in response to the device_del QMP command.
That QMP command is asynchronous to the execution of the guest OS.
j
"fw_remove" is set (by virtue of inverting) by ACPI CEJ0, which is
executed by the guest OS's ACPI interpreter, after the guest OS has
de-scheduled all processes from the CPU being removed (= basically after
the OS has willfully forgotten about the CPU).
Therefore, considering the bitmask (is_removing, fw_remove), three
variations make sense:
Just annotating these with the corresponding ACPI code to make sure
I have it straight. Please correct if my
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On 11/27/20 16:07, Igor Mammedov wrote:
> On Fri, 27 Nov 2020 15:48:34 +0100
> Laszlo Ersek 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_STAT0x4
> #define QEMU_CPUHP_STAT_ENABLEDBIT0
> #define QEMU_CPUHP_STAT_INSERT BIT1
> #define QEMU_CPUHP_STAT_REMOVE BIT2
> +#define QEMU_CPUHP_STAT_FIRMWARE_REMOVEBIT4
>
> #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
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On 11/27/20 05:10, Ankur Arora wrote: > Yeah I was wondering what would happen for simultaneous hot add and remove. > I guess we would always do remove first and then the add, unless we hit > the break due to max_cpus_per_pass and switch to hot-add mode. Considering the firmware only, I disagree with remove-then-add. EFI_SMM_CPU_SERVICE_PROTOCOL.AddProcessor() and EFI_SMM_CPU_SERVICE_PROTOCOL.RemoveProcessor() (implemented in SmmAddProcessor() and SmmRemoveProcessor() in "UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c", respectively) only mark the processors for addition/removal. The actual processing is done only later, in BSPHandler() --> SmmCpuUpdate(), when "all SMI handlers are finished" (see the comment in SmmRemoveProcessor()). Consequently, I would not suggest replacing a valid APIC ID in a particular mCpuHotPlugData.ApicId[Index] slot with INVALID_APIC_ID (corresponding to the unplug operation), and then possibly replacing INVALID_APIC_ID in the *same slot* with the APIC ID of the newly plugged CPU, in the exact same SMI invocation (= in the same execution of CpuHotplugMmi()). That might cause some component in edk2 to see the APIC ID in mCpuHotPlugData.ApicId[Index] to change from one valid ACPI ID to another valid APIC ID, and I don't even want to think about what kind of mess that could cause. So no, please handle plugs first, for which unused slots in mCpuHotPlugData.ApicId will be populated, and *then* handle removals (in the same invocation of CpuHotplugMmi()). By the way, for unplug, you will not have to re-set mCpuHotPlugData.ApicId[Index] to INVALID_APIC_ID, as SmmRemoveProcessor() does that internally. You just have to locate the Index for the APIC ID being removed, for calling EFI_SMM_CPU_SERVICE_PROTOCOL.RemoveProcessor(). Thanks Laszlo
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On 11/26/20 21:38, Igor Mammedov wrote:
> On Thu, 26 Nov 2020 12:17:27 +0100
> Laszlo Ersek wrote:
>
>> On 11/24/20 13:25, Igor Mammedov wrote:
>>> diff --git a/docs/specs/acpi_cpu_hotplug.txt
>>> b/docs/specs/acpi_cpu_hotplug.txt
>>> index 9bb22d1270..f68ef6e06c 100644
>>> --- a/docs/specs/acpi_cpu_hotplug.txt
>>> +++ b/docs/specs/acpi_cpu_hotplug.txt
>>> @@ -57,7 +57,11 @@ read access:
>>>It's valid only when bit 0 is set.
>>> 2: Device remove event, used to distinguish device for which
>>>no device eject request to OSPM was issued.
>>> - 3-7: reserved and should be ignored by OSPM
>>> + 3: reserved and should be ignored by OSPM
>>> + 4: if set to 1, OSPM requests firmware to perform device eject,
>>> + firmware shall clear this event by writing 1 into it before
>>
>> (1) s/clear this event/clear this event bit/
>>
>>> + performing device eject.
>>
>> (2) move the second and third lines ("firmware shall clear") over to
>> the write documentation, below? In particular:
>>
>>> + 5-7: reserved and should be ignored by OSPM
>>> [0x5-0x7] reserved
>>> [0x8] Command data: (DWORD access)
>>>contains 0 unless value last stored in 'Command field' is one of:
>>> @@ -82,7 +86,10 @@ write access:
>>> selected CPU device
>>> 3: if set to 1 initiates device eject, set by OSPM when it
>>> triggers CPU device removal and calls _EJ0 method
>>> -4-7: reserved, OSPM must clear them before writing to register
>>> +4: if set to 1 OSPM hands over device eject to firmware,
>>> + Firmware shall issue device eject request as described above
>>> + (bit #3) and OSPM should not touch device eject bit (#3),
>>
>> (3) it would be clearer if we documented the exact bit writing order
>> here:
>> - clear bit#4, *then* set bit#3 (two write accesses)
>> - versus clear bit#4 *and* set bit#3 (single access)
>
> I was thinking that FW should not bother with clearing bit #4,
> and QEMU should clear it when handling write to bit #3.
> (it looks like I forgot to actually do that)
That should work fine too, as long as it's clearly documented.
>>> @@ -332,6 +335,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
>>> #define CPU_INSERT_EVENT "CINS"
>>> #define CPU_REMOVE_EVENT "CRMV"
>>> #define CPU_EJECT_EVENT "CEJ0"
>>> +#define CPU_FW_EJECT_EVENT "CEJF"
>>>
>>> void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures
>>> opts,
>>> hwaddr io_base,
>>> @@ -384,7 +388,10 @@ void build_cpus_aml(Aml *table, MachineState *machine,
>>> CPUHotplugFeatures opts,
>>> aml_append(field, aml_named_field(CPU_REMOVE_EVENT, 1));
>>> /* initiates device eject, write only */
>>> aml_append(field, aml_named_field(CPU_EJECT_EVENT, 1));
>>> -aml_append(field, aml_reserved_field(4));
>>> +aml_append(field, aml_reserved_field(1));
>>> +/* tell firmware to do device eject, write only */
>>> +aml_append(field, aml_named_field(CPU_FW_EJECT_EVENT, 1));
>>> +aml_append(field, aml_reserved_field(2));
>>> aml_append(field, aml_named_field(CPU_COMMAND, 8));
>>> aml_append(cpu_ctrl_dev, field);
>>>
>>> @@ -419,6 +426,7 @@ void build_cpus_aml(Aml *table, MachineState *machine,
>>> CPUHotplugFeatures opts,
>>> Aml *ins_evt = aml_name("%s.%s", cphp_res_path, CPU_INSERT_EVENT);
>>> Aml *rm_evt = aml_name("%s.%s", cphp_res_path, CPU_REMOVE_EVENT);
>>> Aml *ej_evt = aml_name("%s.%s", cphp_res_path, CPU_EJECT_EVENT);
>>> +Aml *fw_ej_evt = aml_name("%s.%s", cphp_res_path,
>>> CPU_FW_EJECT_EVENT);
>>>
>>> aml_append(cpus_dev, aml_name_decl("_HID",
>>> aml_string("ACPI0010")));
>>> aml_append(cpus_dev, aml_name_decl("_CID", aml_eisaid("PNP0A05")));
>>> @@ -461,7 +469,13 @@ void build_cpus_aml(Aml *table, MachineState *machine,
>>> CPUHotplugFeatures opts,
>>>
>>> aml_append(method, aml_acquire(ctrl_lock, 0x));
>>> aml_append(method, aml_store(idx, cpu_selector));
>>> -aml_append(method, aml_store(one, ej_evt));
>>> +if (opts.fw_unplugs_cpu) {
>>> +aml_append(method, aml_store(one, fw_ej_evt));
>>> +aml_append(method, aml_store(aml_int(OVMF_CPUHP_SMI_CMD),
>>> + aml_name("%s", opts.smi_path)));
>>> +} else {
>>> +aml_append(method, aml_store(one, ej_evt));
>>> +}
>>> aml_append(method, aml_release(ctrl_lock));
>>> }
>>> aml_append(cpus_dev, method);
>>
>> Hmmm, OK, let me parse this.
>>
>> Assume there is a big bunch of device_del QMP commands, QEMU marks the
>> "remove" event pending on the corresponding set of CPUs, plus also makes
>> the ACPI interrupt pending. The ACPI
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On 11/27/20 12:33, Igor Mammedov wrote:
> On Thu, 26 Nov 2020 19:35:30 -0800
> Ankur Arora wrote:
>
>> On 2020-11-26 4:46 a.m., Laszlo Ersek wrote:
>>> On 11/26/20 11:24, Ankur Arora wrote:
On 2020-11-24 4:25 a.m., Igor Mammedov wrote:
> If firmware negotiates ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT feature,
> OSPM on CPU eject will set bit #4 in CPU hotplug block for to be
> ejected CPU to mark it for removal by firmware and trigger SMI
> upcall to let firmware do actual eject.
>
> Signed-off-by: Igor Mammedov
> ---
> PS:
> - abuse 5.1 machine type for now to turn off unplug feature
> (it will be moved to 5.2 machine type once new merge window is open)
> ---
> include/hw/acpi/cpu.h | 2 ++
> docs/specs/acpi_cpu_hotplug.txt | 11 +--
> hw/acpi/cpu.c | 18 --
> hw/i386/acpi-build.c | 5 +
> hw/i386/pc.c | 1 +
> hw/isa/lpc_ich9.c | 2 +-
> 6 files changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> index 0eeedaa491..999caaf510 100644
> --- a/include/hw/acpi/cpu.h
> +++ b/include/hw/acpi/cpu.h
> @@ -22,6 +22,7 @@ typedef struct AcpiCpuStatus {
> uint64_t arch_id;
> bool is_inserting;
> bool is_removing;
> + bool fw_remove;
> uint32_t ost_event;
> uint32_t ost_status;
> } AcpiCpuStatus;
> @@ -50,6 +51,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object
> *owner,
> typedef struct CPUHotplugFeatures {
> bool acpi_1_compatible;
> bool has_legacy_cphp;
> + bool fw_unplugs_cpu;
> const char *smi_path;
> } CPUHotplugFeatures;
> diff --git a/docs/specs/acpi_cpu_hotplug.txt
> b/docs/specs/acpi_cpu_hotplug.txt
> index 9bb22d1270..f68ef6e06c 100644
> --- a/docs/specs/acpi_cpu_hotplug.txt
> +++ b/docs/specs/acpi_cpu_hotplug.txt
> @@ -57,7 +57,11 @@ read access:
> It's valid only when bit 0 is set.
> 2: Device remove event, used to distinguish device for which
> no device eject request to OSPM was issued.
> - 3-7: reserved and should be ignored by OSPM
> + 3: reserved and should be ignored by OSPM
> + 4: if set to 1, OSPM requests firmware to perform device
> eject,
> + firmware shall clear this event by writing 1 into it
> before
> + performing device eject> + 5-7: reserved and
> should be ignored by OSPM
> [0x5-0x7] reserved
> [0x8] Command data: (DWORD access)
> contains 0 unless value last stored in 'Command field' is
> one of:
> @@ -82,7 +86,10 @@ write access:
> selected CPU device
> 3: if set to 1 initiates device eject, set by OSPM when it
> triggers CPU device removal and calls _EJ0 method
> - 4-7: reserved, OSPM must clear them before writing to
> register
> + 4: if set to 1 OSPM hands over device eject to firmware,
> + Firmware shall issue device eject request as described
> above
> + (bit #3) and OSPM should not touch device eject bit (#3),
> + 5-7: reserved, OSPM must clear them before writing to
> register
> [0x5] Command field: (1 byte access)
> value:
> 0: selects a CPU device with inserting/removing events and
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index f099b50927..09d2f20dae 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -71,6 +71,7 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr
> addr, unsigned size)
> val |= cdev->cpu ? 1 : 0;
> val |= cdev->is_inserting ? 2 : 0;
> val |= cdev->is_removing ? 4 : 0;
> + val |= cdev->fw_remove ? 16 : 0;
I might be missing something but I don't see where cdev->fw_remove is being
set.
>>>
>>> See just below, in the cpu_hotplug_wr() hunk. When bit#4 is written --
>>> which happens through the ACPI code change --, fw_remove is inverted.
>> Thanks that makes sense. I was reading the AML building code all wrong.
>>
>>>
>>>
We do set cdev->is_removing in acpi_cpu_unplug_request_cb() so AFAICS
we would always end up setting this bit:
> val |= cdev->is_removing ? 4 : 0;
Also, if cdev->fw_remove and cdev->is_removing are both true, val would be
(4 | 16). I'm guessing that in that case the AML determines which case gets
handled but it might make sense to set just one of these?
>>>
>>> "is_removing" is set directly in response to the device_del QMP command.
>>>
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On Fri, 27 Nov 2020 15:48:34 +0100
Laszlo Ersek wrote:
> On 11/26/20 21:38, Igor Mammedov wrote:
> > On Thu, 26 Nov 2020 12:17:27 +0100
> > Laszlo Ersek wrote:
> >
> >> On 11/24/20 13:25, Igor Mammedov wrote:
>
> >>> diff --git a/docs/specs/acpi_cpu_hotplug.txt
> >>> b/docs/specs/acpi_cpu_hotplug.txt
> >>> index 9bb22d1270..f68ef6e06c 100644
> >>> --- a/docs/specs/acpi_cpu_hotplug.txt
> >>> +++ b/docs/specs/acpi_cpu_hotplug.txt
> >>> @@ -57,7 +57,11 @@ read access:
> >>>It's valid only when bit 0 is set.
> >>> 2: Device remove event, used to distinguish device for which
> >>>no device eject request to OSPM was issued.
> >>> - 3-7: reserved and should be ignored by OSPM
> >>> + 3: reserved and should be ignored by OSPM
> >>> + 4: if set to 1, OSPM requests firmware to perform device
> >>> eject,
> >>> + firmware shall clear this event by writing 1 into it
> >>> before
> >>
> >> (1) s/clear this event/clear this event bit/
> >>
> >>> + performing device eject.
> >>
> >> (2) move the second and third lines ("firmware shall clear") over to
> >> the write documentation, below? In particular:
> >>
> >>> + 5-7: reserved and should be ignored by OSPM
> >>> [0x5-0x7] reserved
> >>> [0x8] Command data: (DWORD access)
> >>>contains 0 unless value last stored in 'Command field' is one
> >>> of:
> >>> @@ -82,7 +86,10 @@ write access:
> >>> selected CPU device
> >>> 3: if set to 1 initiates device eject, set by OSPM when it
> >>> triggers CPU device removal and calls _EJ0 method
> >>> -4-7: reserved, OSPM must clear them before writing to
> >>> register
> >>> +4: if set to 1 OSPM hands over device eject to firmware,
> >>> + Firmware shall issue device eject request as described
> >>> above
> >>> + (bit #3) and OSPM should not touch device eject bit (#3),
> >>>
> >>
> >> (3) it would be clearer if we documented the exact bit writing order
> >> here:
> >> - clear bit#4, *then* set bit#3 (two write accesses)
> >> - versus clear bit#4 *and* set bit#3 (single access)
> >
> > I was thinking that FW should not bother with clearing bit #4,
> > and QEMU should clear it when handling write to bit #3.
> > (it looks like I forgot to actually do that)
>
> That should work fine too, as long as it's clearly documented.
>
>
> >>> @@ -332,6 +335,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
> >>> #define CPU_INSERT_EVENT "CINS"
> >>> #define CPU_REMOVE_EVENT "CRMV"
> >>> #define CPU_EJECT_EVENT "CEJ0"
> >>> +#define CPU_FW_EJECT_EVENT "CEJF"
> >>>
> >>> void build_cpus_aml(Aml *table, MachineState *machine,
> >>> CPUHotplugFeatures opts,
> >>> hwaddr io_base,
> >>> @@ -384,7 +388,10 @@ void build_cpus_aml(Aml *table, MachineState
> >>> *machine, CPUHotplugFeatures opts,
> >>> aml_append(field, aml_named_field(CPU_REMOVE_EVENT, 1));
> >>> /* initiates device eject, write only */
> >>> aml_append(field, aml_named_field(CPU_EJECT_EVENT, 1));
> >>> -aml_append(field, aml_reserved_field(4));
> >>> +aml_append(field, aml_reserved_field(1));
> >>> +/* tell firmware to do device eject, write only */
> >>> +aml_append(field, aml_named_field(CPU_FW_EJECT_EVENT, 1));
> >>> +aml_append(field, aml_reserved_field(2));
> >>> aml_append(field, aml_named_field(CPU_COMMAND, 8));
> >>> aml_append(cpu_ctrl_dev, field);
> >>>
> >>> @@ -419,6 +426,7 @@ void build_cpus_aml(Aml *table, MachineState
> >>> *machine, CPUHotplugFeatures opts,
> >>> Aml *ins_evt = aml_name("%s.%s", cphp_res_path,
> >>> CPU_INSERT_EVENT);
> >>> Aml *rm_evt = aml_name("%s.%s", cphp_res_path, CPU_REMOVE_EVENT);
> >>> Aml *ej_evt = aml_name("%s.%s", cphp_res_path, CPU_EJECT_EVENT);
> >>> +Aml *fw_ej_evt = aml_name("%s.%s", cphp_res_path,
> >>> CPU_FW_EJECT_EVENT);
> >>>
> >>> aml_append(cpus_dev, aml_name_decl("_HID",
> >>> aml_string("ACPI0010")));
> >>> aml_append(cpus_dev, aml_name_decl("_CID",
> >>> aml_eisaid("PNP0A05")));
> >>> @@ -461,7 +469,13 @@ void build_cpus_aml(Aml *table, MachineState
> >>> *machine, CPUHotplugFeatures opts,
> >>>
> >>> aml_append(method, aml_acquire(ctrl_lock, 0x));
> >>> aml_append(method, aml_store(idx, cpu_selector));
> >>> -aml_append(method, aml_store(one, ej_evt));
> >>> +if (opts.fw_unplugs_cpu) {
> >>> +aml_append(method, aml_store(one, fw_ej_evt));
> >>> +aml_append(method, aml_store(aml_int(OVMF_CPUHP_SMI_CMD),
> >>> + aml_name("%s", opts.smi_path)));
> >>> +} else {
> >>> +aml_append(method, aml_store(one, ej_evt));
> >>> +}
> >>>
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On Thu, 26 Nov 2020 20:10:59 -0800
Ankur Arora wrote:
> On 2020-11-26 12:38 p.m., Igor Mammedov wrote:
> > On Thu, 26 Nov 2020 12:17:27 +0100
> > Laszlo Ersek wrote:
> >
> >> On 11/24/20 13:25, Igor Mammedov wrote:
> >>> If firmware negotiates ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT feature,
> >>> OSPM on CPU eject will set bit #4 in CPU hotplug block for to be
> >>> ejected CPU to mark it for removal by firmware and trigger SMI
> >>> upcall to let firmware do actual eject.
> >>>
> >>> Signed-off-by: Igor Mammedov
> >>> ---
> >>> PS:
> >>>- abuse 5.1 machine type for now to turn off unplug feature
> >>> (it will be moved to 5.2 machine type once new merge window is open)
> >>> ---
> >>> include/hw/acpi/cpu.h | 2 ++
> >>> docs/specs/acpi_cpu_hotplug.txt | 11 +--
> >>> hw/acpi/cpu.c | 18 --
> >>> hw/i386/acpi-build.c| 5 +
> >>> hw/i386/pc.c| 1 +
> >>> hw/isa/lpc_ich9.c | 2 +-
> >>> 6 files changed, 34 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> >>> index 0eeedaa491..999caaf510 100644
> >>> --- a/include/hw/acpi/cpu.h
> >>> +++ b/include/hw/acpi/cpu.h
> >>> @@ -22,6 +22,7 @@ typedef struct AcpiCpuStatus {
> >>> uint64_t arch_id;
> >>> bool is_inserting;
> >>> bool is_removing;
> >>> +bool fw_remove;
> >>> uint32_t ost_event;
> >>> uint32_t ost_status;
> >>> } AcpiCpuStatus;
> >>> @@ -50,6 +51,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object
> >>> *owner,
> >>> typedef struct CPUHotplugFeatures {
> >>> bool acpi_1_compatible;
> >>> bool has_legacy_cphp;
> >>> +bool fw_unplugs_cpu;
> >>> const char *smi_path;
> >>> } CPUHotplugFeatures;
> >>>
> >>> diff --git a/docs/specs/acpi_cpu_hotplug.txt
> >>> b/docs/specs/acpi_cpu_hotplug.txt
> >>> index 9bb22d1270..f68ef6e06c 100644
> >>> --- a/docs/specs/acpi_cpu_hotplug.txt
> >>> +++ b/docs/specs/acpi_cpu_hotplug.txt
> >>> @@ -57,7 +57,11 @@ read access:
> >>> It's valid only when bit 0 is set.
> >>> 2: Device remove event, used to distinguish device for which
> >>> no device eject request to OSPM was issued.
> >>> - 3-7: reserved and should be ignored by OSPM
> >>> + 3: reserved and should be ignored by OSPM
> >>> + 4: if set to 1, OSPM requests firmware to perform device
> >>> eject,
> >>> + firmware shall clear this event by writing 1 into it
> >>> before
> >>
> >> (1) s/clear this event/clear this event bit/
> >>
> >>> + performing device eject.
> >>
> >> (2) move the second and third lines ("firmware shall clear") over to
> >> the write documentation, below? In particular:
> >>
> >>> + 5-7: reserved and should be ignored by OSPM
> >>> [0x5-0x7] reserved
> >>> [0x8] Command data: (DWORD access)
> >>> contains 0 unless value last stored in 'Command field' is one
> >>> of:
> >>> @@ -82,7 +86,10 @@ write access:
> >>> selected CPU device
> >>> 3: if set to 1 initiates device eject, set by OSPM when it
> >>> triggers CPU device removal and calls _EJ0 method
> >>> -4-7: reserved, OSPM must clear them before writing to
> >>> register
> >>> +4: if set to 1 OSPM hands over device eject to firmware,
> >>> + Firmware shall issue device eject request as described
> >>> above
> >>> + (bit #3) and OSPM should not touch device eject bit (#3),
> >>>
> >>
> >> (3) it would be clearer if we documented the exact bit writing order
> >> here:
> >> - clear bit#4, *then* set bit#3 (two write accesses)
> >> - versus clear bit#4 *and* set bit#3 (single access)
> >
> > I was thinking that FW should not bother with clearing bit #4,
> > and QEMU should clear it when handling write to bit #3.
> > (it looks like I forgot to actually do that)
>
> Why involve the firmware with bit #3 at all? If the firmware only reads bit #4
> to detect fw_remove and then write (and thus reset) bit #4, isn't that
> good enough?
That would needlessly complicate code on QEMU side and I don't want to
overload bit #4 with another semantics, and we already have bit #3 that
does eject. So unless there are issues with that, I'd stick to using
bit #3 for eject.
>
>
> >
> >>
> >>
> >>
> >>> +5-7: reserved, OSPM must clear them before writing to
> >>> register
> >>> [0x5] Command field: (1 byte access)
> >>> value:
> >>> 0: selects a CPU device with inserting/removing events and
> >>> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> >>> index f099b50927..09d2f20dae 100644
> >>> --- a/hw/acpi/cpu.c
> >>> +++ b/hw/acpi/cpu.c
> >>> @@ -71,6 +71,7 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr
> >>> addr, unsigned size)
> >>> val |= cdev->cpu ?
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On Thu, 26 Nov 2020 19:35:30 -0800
Ankur Arora wrote:
> On 2020-11-26 4:46 a.m., Laszlo Ersek wrote:
> > On 11/26/20 11:24, Ankur Arora wrote:
> >> On 2020-11-24 4:25 a.m., Igor Mammedov wrote:
> >>> If firmware negotiates ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT feature,
> >>> OSPM on CPU eject will set bit #4 in CPU hotplug block for to be
> >>> ejected CPU to mark it for removal by firmware and trigger SMI
> >>> upcall to let firmware do actual eject.
> >>>
> >>> Signed-off-by: Igor Mammedov
> >>> ---
> >>> PS:
> >>> - abuse 5.1 machine type for now to turn off unplug feature
> >>> (it will be moved to 5.2 machine type once new merge window is open)
> >>> ---
> >>> include/hw/acpi/cpu.h | 2 ++
> >>> docs/specs/acpi_cpu_hotplug.txt | 11 +--
> >>> hw/acpi/cpu.c | 18 --
> >>> hw/i386/acpi-build.c | 5 +
> >>> hw/i386/pc.c | 1 +
> >>> hw/isa/lpc_ich9.c | 2 +-
> >>> 6 files changed, 34 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> >>> index 0eeedaa491..999caaf510 100644
> >>> --- a/include/hw/acpi/cpu.h
> >>> +++ b/include/hw/acpi/cpu.h
> >>> @@ -22,6 +22,7 @@ typedef struct AcpiCpuStatus {
> >>> uint64_t arch_id;
> >>> bool is_inserting;
> >>> bool is_removing;
> >>> + bool fw_remove;
> >>> uint32_t ost_event;
> >>> uint32_t ost_status;
> >>> } AcpiCpuStatus;
> >>> @@ -50,6 +51,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object
> >>> *owner,
> >>> typedef struct CPUHotplugFeatures {
> >>> bool acpi_1_compatible;
> >>> bool has_legacy_cphp;
> >>> + bool fw_unplugs_cpu;
> >>> const char *smi_path;
> >>> } CPUHotplugFeatures;
> >>> diff --git a/docs/specs/acpi_cpu_hotplug.txt
> >>> b/docs/specs/acpi_cpu_hotplug.txt
> >>> index 9bb22d1270..f68ef6e06c 100644
> >>> --- a/docs/specs/acpi_cpu_hotplug.txt
> >>> +++ b/docs/specs/acpi_cpu_hotplug.txt
> >>> @@ -57,7 +57,11 @@ read access:
> >>> It's valid only when bit 0 is set.
> >>> 2: Device remove event, used to distinguish device for which
> >>> no device eject request to OSPM was issued.
> >>> - 3-7: reserved and should be ignored by OSPM
> >>> + 3: reserved and should be ignored by OSPM
> >>> + 4: if set to 1, OSPM requests firmware to perform device
> >>> eject,
> >>> + firmware shall clear this event by writing 1 into it
> >>> before
> >>> + performing device eject> + 5-7: reserved and
> >>> should be ignored by OSPM
> >>> [0x5-0x7] reserved
> >>> [0x8] Command data: (DWORD access)
> >>> contains 0 unless value last stored in 'Command field' is
> >>> one of:
> >>> @@ -82,7 +86,10 @@ write access:
> >>> selected CPU device
> >>> 3: if set to 1 initiates device eject, set by OSPM when it
> >>> triggers CPU device removal and calls _EJ0 method
> >>> - 4-7: reserved, OSPM must clear them before writing to
> >>> register
> >>> + 4: if set to 1 OSPM hands over device eject to firmware,
> >>> + Firmware shall issue device eject request as described
> >>> above
> >>> + (bit #3) and OSPM should not touch device eject bit (#3),
> >>> + 5-7: reserved, OSPM must clear them before writing to
> >>> register
> >>> [0x5] Command field: (1 byte access)
> >>> value:
> >>> 0: selects a CPU device with inserting/removing events and
> >>> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> >>> index f099b50927..09d2f20dae 100644
> >>> --- a/hw/acpi/cpu.c
> >>> +++ b/hw/acpi/cpu.c
> >>> @@ -71,6 +71,7 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr
> >>> addr, unsigned size)
> >>> val |= cdev->cpu ? 1 : 0;
> >>> val |= cdev->is_inserting ? 2 : 0;
> >>> val |= cdev->is_removing ? 4 : 0;
> >>> + val |= cdev->fw_remove ? 16 : 0;
> >>
> >> I might be missing something but I don't see where cdev->fw_remove is being
> >> set.
> >
> > See just below, in the cpu_hotplug_wr() hunk. When bit#4 is written --
> > which happens through the ACPI code change --, fw_remove is inverted.
> Thanks that makes sense. I was reading the AML building code all wrong.
>
> >
> >
> >> We do set cdev->is_removing in acpi_cpu_unplug_request_cb() so AFAICS
> >> we would always end up setting this bit:
> >>> val |= cdev->is_removing ? 4 : 0;
> >>
> >> Also, if cdev->fw_remove and cdev->is_removing are both true, val would be
> >> (4 | 16). I'm guessing that in that case the AML determines which case gets
> >> handled but it might make sense to set just one of these?
> >
> > "is_removing" is set directly in response to the device_del QMP command.
> > That QMP command is asynchronous to the
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On 2020-11-26 12:38 p.m., Igor Mammedov wrote:
On Thu, 26 Nov 2020 12:17:27 +0100
Laszlo Ersek wrote:
On 11/24/20 13:25, Igor Mammedov wrote:
If firmware negotiates ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT feature,
OSPM on CPU eject will set bit #4 in CPU hotplug block for to be
ejected CPU to mark it for removal by firmware and trigger SMI
upcall to let firmware do actual eject.
Signed-off-by: Igor Mammedov
---
PS:
- abuse 5.1 machine type for now to turn off unplug feature
(it will be moved to 5.2 machine type once new merge window is open)
---
include/hw/acpi/cpu.h | 2 ++
docs/specs/acpi_cpu_hotplug.txt | 11 +--
hw/acpi/cpu.c | 18 --
hw/i386/acpi-build.c| 5 +
hw/i386/pc.c| 1 +
hw/isa/lpc_ich9.c | 2 +-
6 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
index 0eeedaa491..999caaf510 100644
--- a/include/hw/acpi/cpu.h
+++ b/include/hw/acpi/cpu.h
@@ -22,6 +22,7 @@ typedef struct AcpiCpuStatus {
uint64_t arch_id;
bool is_inserting;
bool is_removing;
+bool fw_remove;
uint32_t ost_event;
uint32_t ost_status;
} AcpiCpuStatus;
@@ -50,6 +51,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
typedef struct CPUHotplugFeatures {
bool acpi_1_compatible;
bool has_legacy_cphp;
+bool fw_unplugs_cpu;
const char *smi_path;
} CPUHotplugFeatures;
diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
index 9bb22d1270..f68ef6e06c 100644
--- a/docs/specs/acpi_cpu_hotplug.txt
+++ b/docs/specs/acpi_cpu_hotplug.txt
@@ -57,7 +57,11 @@ read access:
It's valid only when bit 0 is set.
2: Device remove event, used to distinguish device for which
no device eject request to OSPM was issued.
- 3-7: reserved and should be ignored by OSPM
+ 3: reserved and should be ignored by OSPM
+ 4: if set to 1, OSPM requests firmware to perform device eject,
+ firmware shall clear this event by writing 1 into it before
(1) s/clear this event/clear this event bit/
+ performing device eject.
(2) move the second and third lines ("firmware shall clear") over to
the write documentation, below? In particular:
+ 5-7: reserved and should be ignored by OSPM
[0x5-0x7] reserved
[0x8] Command data: (DWORD access)
contains 0 unless value last stored in 'Command field' is one of:
@@ -82,7 +86,10 @@ write access:
selected CPU device
3: if set to 1 initiates device eject, set by OSPM when it
triggers CPU device removal and calls _EJ0 method
-4-7: reserved, OSPM must clear them before writing to register
+4: if set to 1 OSPM hands over device eject to firmware,
+ Firmware shall issue device eject request as described above
+ (bit #3) and OSPM should not touch device eject bit (#3),
(3) it would be clearer if we documented the exact bit writing order
here:
- clear bit#4, *then* set bit#3 (two write accesses)
- versus clear bit#4 *and* set bit#3 (single access)
I was thinking that FW should not bother with clearing bit #4,
and QEMU should clear it when handling write to bit #3.
(it looks like I forgot to actually do that)
Why involve the firmware with bit #3 at all? If the firmware only reads bit #4
to detect fw_remove and then write (and thus reset) bit #4, isn't that
good enough?
+5-7: reserved, OSPM must clear them before writing to register
[0x5] Command field: (1 byte access)
value:
0: selects a CPU device with inserting/removing events and
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index f099b50927..09d2f20dae 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -71,6 +71,7 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr,
unsigned size)
val |= cdev->cpu ? 1 : 0;
val |= cdev->is_inserting ? 2 : 0;
val |= cdev->is_removing ? 4 : 0;
+val |= cdev->fw_remove ? 16 : 0;
trace_cpuhp_acpi_read_flags(cpu_st->selector, val);
break;
case ACPI_CPU_CMD_DATA_OFFSET_RW:
@@ -148,6 +149,8 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr,
uint64_t data,
hotplug_ctrl = qdev_get_hotplug_handler(dev);
hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
object_unparent(OBJECT(dev));
+} else if (data & 16) {
+cdev->fw_remove = !cdev->fw_remove;
hm... so I guess the ACPI code will first write bit#4 to flip
"fw_remove" from "off" to "on". Then the firmware will write bit#4 to
flip "fw_remove" back to "off". And finally, the firmware will write
bit#3 (strictly as a separate access) to unplug the CPU.
sorry for confusion in doc vs impl, FW should only read bit #4,
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On 2020-11-26 11:50 a.m., Igor Mammedov wrote:
On Thu, 26 Nov 2020 13:46:32 +0100
Laszlo Ersek wrote:
On 11/26/20 11:24, Ankur Arora wrote:
On 2020-11-24 4:25 a.m., Igor Mammedov wrote:
If firmware negotiates ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT feature,
OSPM on CPU eject will set bit #4 in CPU hotplug block for to be
ejected CPU to mark it for removal by firmware and trigger SMI
upcall to let firmware do actual eject.
Signed-off-by: Igor Mammedov
---
PS:
- abuse 5.1 machine type for now to turn off unplug feature
(it will be moved to 5.2 machine type once new merge window is open)
---
include/hw/acpi/cpu.h | 2 ++
docs/specs/acpi_cpu_hotplug.txt | 11 +--
hw/acpi/cpu.c | 18 --
hw/i386/acpi-build.c | 5 +
hw/i386/pc.c | 1 +
hw/isa/lpc_ich9.c | 2 +-
6 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
index 0eeedaa491..999caaf510 100644
--- a/include/hw/acpi/cpu.h
+++ b/include/hw/acpi/cpu.h
@@ -22,6 +22,7 @@ typedef struct AcpiCpuStatus {
uint64_t arch_id;
bool is_inserting;
bool is_removing;
+ bool fw_remove;
uint32_t ost_event;
uint32_t ost_status;
} AcpiCpuStatus;
@@ -50,6 +51,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object
*owner,
typedef struct CPUHotplugFeatures {
bool acpi_1_compatible;
bool has_legacy_cphp;
+ bool fw_unplugs_cpu;
const char *smi_path;
} CPUHotplugFeatures;
diff --git a/docs/specs/acpi_cpu_hotplug.txt
b/docs/specs/acpi_cpu_hotplug.txt
index 9bb22d1270..f68ef6e06c 100644
--- a/docs/specs/acpi_cpu_hotplug.txt
+++ b/docs/specs/acpi_cpu_hotplug.txt
@@ -57,7 +57,11 @@ read access:
It's valid only when bit 0 is set.
2: Device remove event, used to distinguish device for which
no device eject request to OSPM was issued.
- 3-7: reserved and should be ignored by OSPM
+ 3: reserved and should be ignored by OSPM
+ 4: if set to 1, OSPM requests firmware to perform device
eject,
+ firmware shall clear this event by writing 1 into it
before
+ performing device eject> + 5-7: reserved and
should be ignored by OSPM
[0x5-0x7] reserved
[0x8] Command data: (DWORD access)
contains 0 unless value last stored in 'Command field' is
one of:
@@ -82,7 +86,10 @@ write access:
selected CPU device
3: if set to 1 initiates device eject, set by OSPM when it
triggers CPU device removal and calls _EJ0 method
- 4-7: reserved, OSPM must clear them before writing to
register
+ 4: if set to 1 OSPM hands over device eject to firmware,
+ Firmware shall issue device eject request as described
above
+ (bit #3) and OSPM should not touch device eject bit (#3),
+ 5-7: reserved, OSPM must clear them before writing to
register
[0x5] Command field: (1 byte access)
value:
0: selects a CPU device with inserting/removing events and
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index f099b50927..09d2f20dae 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -71,6 +71,7 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr
addr, unsigned size)
val |= cdev->cpu ? 1 : 0;
val |= cdev->is_inserting ? 2 : 0;
val |= cdev->is_removing ? 4 : 0;
+ val |= cdev->fw_remove ? 16 : 0;
I might be missing something but I don't see where cdev->fw_remove is being
set.
See just below, in the cpu_hotplug_wr() hunk. When bit#4 is written --
which happens through the ACPI code change --, fw_remove is inverted.
We do set cdev->is_removing in acpi_cpu_unplug_request_cb() so AFAICS
we would always end up setting this bit:
val |= cdev->is_removing ? 4 : 0;
Also, if cdev->fw_remove and cdev->is_removing are both true, val would be
(4 | 16). I'm guessing that in that case the AML determines which case gets
handled but it might make sense to set just one of these?
"is_removing" is set directly in response to the device_del QMP command.
That QMP command is asynchronous to the execution of the guest OS.
its removing is notification to OSPM, which is cleared when ACPI scans
for events if I'm not mistaken.
Yeah, I think I finally have it on straight. Though, I have a couple of
questions
for you in my reply to Laszlo.
Thanks
Ankur
"fw_remove" is set (by virtue of inverting) by ACPI CEJ0, which is
executed by the guest OS's ACPI interpreter, after the guest OS has
de-scheduled all processes from the CPU being removed (= basically after
the OS has willfully forgotten about the CPU).
Therefore, considering the bitmask (is_removing, fw_remove), three
variations make sense:
#1 (is_removing=0, fw_remove=0) -- normal status; no unplug requested
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On 2020-11-26 4:46 a.m., Laszlo Ersek wrote:
On 11/26/20 11:24, Ankur Arora wrote:
On 2020-11-24 4:25 a.m., Igor Mammedov wrote:
If firmware negotiates ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT feature,
OSPM on CPU eject will set bit #4 in CPU hotplug block for to be
ejected CPU to mark it for removal by firmware and trigger SMI
upcall to let firmware do actual eject.
Signed-off-by: Igor Mammedov
---
PS:
- abuse 5.1 machine type for now to turn off unplug feature
(it will be moved to 5.2 machine type once new merge window is open)
---
include/hw/acpi/cpu.h | 2 ++
docs/specs/acpi_cpu_hotplug.txt | 11 +--
hw/acpi/cpu.c | 18 --
hw/i386/acpi-build.c | 5 +
hw/i386/pc.c | 1 +
hw/isa/lpc_ich9.c | 2 +-
6 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
index 0eeedaa491..999caaf510 100644
--- a/include/hw/acpi/cpu.h
+++ b/include/hw/acpi/cpu.h
@@ -22,6 +22,7 @@ typedef struct AcpiCpuStatus {
uint64_t arch_id;
bool is_inserting;
bool is_removing;
+ bool fw_remove;
uint32_t ost_event;
uint32_t ost_status;
} AcpiCpuStatus;
@@ -50,6 +51,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object
*owner,
typedef struct CPUHotplugFeatures {
bool acpi_1_compatible;
bool has_legacy_cphp;
+ bool fw_unplugs_cpu;
const char *smi_path;
} CPUHotplugFeatures;
diff --git a/docs/specs/acpi_cpu_hotplug.txt
b/docs/specs/acpi_cpu_hotplug.txt
index 9bb22d1270..f68ef6e06c 100644
--- a/docs/specs/acpi_cpu_hotplug.txt
+++ b/docs/specs/acpi_cpu_hotplug.txt
@@ -57,7 +57,11 @@ read access:
It's valid only when bit 0 is set.
2: Device remove event, used to distinguish device for which
no device eject request to OSPM was issued.
- 3-7: reserved and should be ignored by OSPM
+ 3: reserved and should be ignored by OSPM
+ 4: if set to 1, OSPM requests firmware to perform device
eject,
+ firmware shall clear this event by writing 1 into it
before
+ performing device eject> + 5-7: reserved and
should be ignored by OSPM
[0x5-0x7] reserved
[0x8] Command data: (DWORD access)
contains 0 unless value last stored in 'Command field' is
one of:
@@ -82,7 +86,10 @@ write access:
selected CPU device
3: if set to 1 initiates device eject, set by OSPM when it
triggers CPU device removal and calls _EJ0 method
- 4-7: reserved, OSPM must clear them before writing to
register
+ 4: if set to 1 OSPM hands over device eject to firmware,
+ Firmware shall issue device eject request as described
above
+ (bit #3) and OSPM should not touch device eject bit (#3),
+ 5-7: reserved, OSPM must clear them before writing to
register
[0x5] Command field: (1 byte access)
value:
0: selects a CPU device with inserting/removing events and
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index f099b50927..09d2f20dae 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -71,6 +71,7 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr
addr, unsigned size)
val |= cdev->cpu ? 1 : 0;
val |= cdev->is_inserting ? 2 : 0;
val |= cdev->is_removing ? 4 : 0;
+ val |= cdev->fw_remove ? 16 : 0;
I might be missing something but I don't see where cdev->fw_remove is being
set.
See just below, in the cpu_hotplug_wr() hunk. When bit#4 is written --
which happens through the ACPI code change --, fw_remove is inverted.
Thanks that makes sense. I was reading the AML building code all wrong.
We do set cdev->is_removing in acpi_cpu_unplug_request_cb() so AFAICS
we would always end up setting this bit:
val |= cdev->is_removing ? 4 : 0;
Also, if cdev->fw_remove and cdev->is_removing are both true, val would be
(4 | 16). I'm guessing that in that case the AML determines which case gets
handled but it might make sense to set just one of these?
"is_removing" is set directly in response to the device_del QMP command.
That QMP command is asynchronous to the execution of the guest OS.
j
"fw_remove" is set (by virtue of inverting) by ACPI CEJ0, which is
executed by the guest OS's ACPI interpreter, after the guest OS has
de-scheduled all processes from the CPU being removed (= basically after
the OS has willfully forgotten about the CPU).
Therefore, considering the bitmask (is_removing, fw_remove), three
variations make sense:
Just annotating these with the corresponding ACPI code to make sure
I have it straight. Please correct if my interpretation is wrong. Also,
a few questions inline:
#1 (is_removing=0, fw_remove=0) -- normal status; no unplug requested
#2 (is_removing=1, fw_remove=0) -- unplug requested via
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On Thu, 26 Nov 2020 12:17:27 +0100
Laszlo Ersek wrote:
> On 11/24/20 13:25, Igor Mammedov wrote:
> > If firmware negotiates ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT feature,
> > OSPM on CPU eject will set bit #4 in CPU hotplug block for to be
> > ejected CPU to mark it for removal by firmware and trigger SMI
> > upcall to let firmware do actual eject.
> >
> > Signed-off-by: Igor Mammedov
> > ---
> > PS:
> > - abuse 5.1 machine type for now to turn off unplug feature
> > (it will be moved to 5.2 machine type once new merge window is open)
> > ---
> > include/hw/acpi/cpu.h | 2 ++
> > docs/specs/acpi_cpu_hotplug.txt | 11 +--
> > hw/acpi/cpu.c | 18 --
> > hw/i386/acpi-build.c| 5 +
> > hw/i386/pc.c| 1 +
> > hw/isa/lpc_ich9.c | 2 +-
> > 6 files changed, 34 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> > index 0eeedaa491..999caaf510 100644
> > --- a/include/hw/acpi/cpu.h
> > +++ b/include/hw/acpi/cpu.h
> > @@ -22,6 +22,7 @@ typedef struct AcpiCpuStatus {
> > uint64_t arch_id;
> > bool is_inserting;
> > bool is_removing;
> > +bool fw_remove;
> > uint32_t ost_event;
> > uint32_t ost_status;
> > } AcpiCpuStatus;
> > @@ -50,6 +51,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
> > typedef struct CPUHotplugFeatures {
> > bool acpi_1_compatible;
> > bool has_legacy_cphp;
> > +bool fw_unplugs_cpu;
> > const char *smi_path;
> > } CPUHotplugFeatures;
> >
> > diff --git a/docs/specs/acpi_cpu_hotplug.txt
> > b/docs/specs/acpi_cpu_hotplug.txt
> > index 9bb22d1270..f68ef6e06c 100644
> > --- a/docs/specs/acpi_cpu_hotplug.txt
> > +++ b/docs/specs/acpi_cpu_hotplug.txt
> > @@ -57,7 +57,11 @@ read access:
> >It's valid only when bit 0 is set.
> > 2: Device remove event, used to distinguish device for which
> >no device eject request to OSPM was issued.
> > - 3-7: reserved and should be ignored by OSPM
> > + 3: reserved and should be ignored by OSPM
> > + 4: if set to 1, OSPM requests firmware to perform device eject,
> > + firmware shall clear this event by writing 1 into it before
>
> (1) s/clear this event/clear this event bit/
>
> > + performing device eject.
>
> (2) move the second and third lines ("firmware shall clear") over to
> the write documentation, below? In particular:
>
> > + 5-7: reserved and should be ignored by OSPM
> > [0x5-0x7] reserved
> > [0x8] Command data: (DWORD access)
> >contains 0 unless value last stored in 'Command field' is one of:
> > @@ -82,7 +86,10 @@ write access:
> > selected CPU device
> > 3: if set to 1 initiates device eject, set by OSPM when it
> > triggers CPU device removal and calls _EJ0 method
> > -4-7: reserved, OSPM must clear them before writing to register
> > +4: if set to 1 OSPM hands over device eject to firmware,
> > + Firmware shall issue device eject request as described above
> > + (bit #3) and OSPM should not touch device eject bit (#3),
>
> (3) it would be clearer if we documented the exact bit writing order
> here:
> - clear bit#4, *then* set bit#3 (two write accesses)
> - versus clear bit#4 *and* set bit#3 (single access)
I was thinking that FW should not bother with clearing bit #4,
and QEMU should clear it when handling write to bit #3.
(it looks like I forgot to actually do that)
>
>
>
> > +5-7: reserved, OSPM must clear them before writing to register
> > [0x5] Command field: (1 byte access)
> >value:
> > 0: selects a CPU device with inserting/removing events and
> > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> > index f099b50927..09d2f20dae 100644
> > --- a/hw/acpi/cpu.c
> > +++ b/hw/acpi/cpu.c
> > @@ -71,6 +71,7 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr,
> > unsigned size)
> > val |= cdev->cpu ? 1 : 0;
> > val |= cdev->is_inserting ? 2 : 0;
> > val |= cdev->is_removing ? 4 : 0;
> > +val |= cdev->fw_remove ? 16 : 0;
> > trace_cpuhp_acpi_read_flags(cpu_st->selector, val);
> > break;
> > case ACPI_CPU_CMD_DATA_OFFSET_RW:
> > @@ -148,6 +149,8 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr,
> > uint64_t data,
> > hotplug_ctrl = qdev_get_hotplug_handler(dev);
> > hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
> > object_unparent(OBJECT(dev));
> > +} else if (data & 16) {
> > +cdev->fw_remove = !cdev->fw_remove;
>
> hm... so I guess the ACPI code will first write bit#4 to flip
> "fw_remove" from "off" to "on". Then the firmware will write bit#4 to
> flip "fw_remove" back to "off". And finally, the firmware
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On Thu, 26 Nov 2020 02:24:27 -0800
Ankur Arora wrote:
> On 2020-11-24 4:25 a.m., Igor Mammedov wrote:
> > If firmware negotiates ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT feature,
> > OSPM on CPU eject will set bit #4 in CPU hotplug block for to be
> > ejected CPU to mark it for removal by firmware and trigger SMI
> > upcall to let firmware do actual eject.
> >
> > Signed-off-by: Igor Mammedov
> > ---
> > PS:
> >- abuse 5.1 machine type for now to turn off unplug feature
> > (it will be moved to 5.2 machine type once new merge window is open)
> > ---
> > include/hw/acpi/cpu.h | 2 ++
> > docs/specs/acpi_cpu_hotplug.txt | 11 +--
> > hw/acpi/cpu.c | 18 --
> > hw/i386/acpi-build.c| 5 +
> > hw/i386/pc.c| 1 +
> > hw/isa/lpc_ich9.c | 2 +-
> > 6 files changed, 34 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> > index 0eeedaa491..999caaf510 100644
> > --- a/include/hw/acpi/cpu.h
> > +++ b/include/hw/acpi/cpu.h
> > @@ -22,6 +22,7 @@ typedef struct AcpiCpuStatus {
> > uint64_t arch_id;
> > bool is_inserting;
> > bool is_removing;
> > +bool fw_remove;
> > uint32_t ost_event;
> > uint32_t ost_status;
> > } AcpiCpuStatus;
> > @@ -50,6 +51,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
> > typedef struct CPUHotplugFeatures {
> > bool acpi_1_compatible;
> > bool has_legacy_cphp;
> > +bool fw_unplugs_cpu;
> > const char *smi_path;
> > } CPUHotplugFeatures;
> >
> > diff --git a/docs/specs/acpi_cpu_hotplug.txt
> > b/docs/specs/acpi_cpu_hotplug.txt
> > index 9bb22d1270..f68ef6e06c 100644
> > --- a/docs/specs/acpi_cpu_hotplug.txt
> > +++ b/docs/specs/acpi_cpu_hotplug.txt
> > @@ -57,7 +57,11 @@ read access:
> > It's valid only when bit 0 is set.
> > 2: Device remove event, used to distinguish device for which
> > no device eject request to OSPM was issued.
> > - 3-7: reserved and should be ignored by OSPM
> > + 3: reserved and should be ignored by OSPM
> > + 4: if set to 1, OSPM requests firmware to perform device eject,
> > + firmware shall clear this event by writing 1 into it before
> > + performing device eject> + 5-7: reserved and
> > should be ignored by OSPM
> > [0x5-0x7] reserved
> > [0x8] Command data: (DWORD access)
> > contains 0 unless value last stored in 'Command field' is one
> > of:
> > @@ -82,7 +86,10 @@ write access:
> > selected CPU device
> > 3: if set to 1 initiates device eject, set by OSPM when it
> > triggers CPU device removal and calls _EJ0 method
> > -4-7: reserved, OSPM must clear them before writing to register
> > +4: if set to 1 OSPM hands over device eject to firmware,
> > + Firmware shall issue device eject request as described above
> > + (bit #3) and OSPM should not touch device eject bit (#3),
> > +5-7: reserved, OSPM must clear them before writing to register
> > [0x5] Command field: (1 byte access)
> > value:
> > 0: selects a CPU device with inserting/removing events and
> > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> > index f099b50927..09d2f20dae 100644
> > --- a/hw/acpi/cpu.c
> > +++ b/hw/acpi/cpu.c
> > @@ -71,6 +71,7 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr,
> > unsigned size)
> > val |= cdev->cpu ? 1 : 0;
> > val |= cdev->is_inserting ? 2 : 0;
> > val |= cdev->is_removing ? 4 : 0;
> > +val |= cdev->fw_remove ? 16 : 0;
>
> I might be missing something but I don't see where cdev->fw_remove is being
> set. We do set cdev->is_removing in acpi_cpu_unplug_request_cb() so AFAICS
> we would always end up setting this bit:
> > val |= cdev->is_removing ? 4 : 0;
>
> Also, if cdev->fw_remove and cdev->is_removing are both true, val would be
> (4 | 16). I'm guessing that in that case the AML determines which case gets
> handled but it might make sense to set just one of these?
cdev->fw_remove is set by AML when OSPM thinks it's ready to remove the CPU,
see "aml_append(method, aml_store(one, fw_ej_evt));" in this patch.
cdev->is_removing is set by QEMU's device_del command, and processed by AML
(which includes it being cleared before OSPM calls EJ0), it only serves
as flag for generating eject notification to OSPM.
>
>
> > trace_cpuhp_acpi_read_flags(cpu_st->selector, val);
> > break;
> > case ACPI_CPU_CMD_DATA_OFFSET_RW:
> > @@ -148,6 +149,8 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr,
> > uint64_t data,
> > hotplug_ctrl = qdev_get_hotplug_handler(dev);
> > hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
> >
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On Thu, 26 Nov 2020 13:46:32 +0100
Laszlo Ersek wrote:
> On 11/26/20 11:24, Ankur Arora wrote:
> > On 2020-11-24 4:25 a.m., Igor Mammedov wrote:
> >> If firmware negotiates ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT feature,
> >> OSPM on CPU eject will set bit #4 in CPU hotplug block for to be
> >> ejected CPU to mark it for removal by firmware and trigger SMI
> >> upcall to let firmware do actual eject.
> >>
> >> Signed-off-by: Igor Mammedov
> >> ---
> >> PS:
> >> - abuse 5.1 machine type for now to turn off unplug feature
> >> (it will be moved to 5.2 machine type once new merge window is open)
> >> ---
> >> include/hw/acpi/cpu.h | 2 ++
> >> docs/specs/acpi_cpu_hotplug.txt | 11 +--
> >> hw/acpi/cpu.c | 18 --
> >> hw/i386/acpi-build.c | 5 +
> >> hw/i386/pc.c | 1 +
> >> hw/isa/lpc_ich9.c | 2 +-
> >> 6 files changed, 34 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> >> index 0eeedaa491..999caaf510 100644
> >> --- a/include/hw/acpi/cpu.h
> >> +++ b/include/hw/acpi/cpu.h
> >> @@ -22,6 +22,7 @@ typedef struct AcpiCpuStatus {
> >> uint64_t arch_id;
> >> bool is_inserting;
> >> bool is_removing;
> >> + bool fw_remove;
> >> uint32_t ost_event;
> >> uint32_t ost_status;
> >> } AcpiCpuStatus;
> >> @@ -50,6 +51,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object
> >> *owner,
> >> typedef struct CPUHotplugFeatures {
> >> bool acpi_1_compatible;
> >> bool has_legacy_cphp;
> >> + bool fw_unplugs_cpu;
> >> const char *smi_path;
> >> } CPUHotplugFeatures;
> >> diff --git a/docs/specs/acpi_cpu_hotplug.txt
> >> b/docs/specs/acpi_cpu_hotplug.txt
> >> index 9bb22d1270..f68ef6e06c 100644
> >> --- a/docs/specs/acpi_cpu_hotplug.txt
> >> +++ b/docs/specs/acpi_cpu_hotplug.txt
> >> @@ -57,7 +57,11 @@ read access:
> >> It's valid only when bit 0 is set.
> >> 2: Device remove event, used to distinguish device for which
> >> no device eject request to OSPM was issued.
> >> - 3-7: reserved and should be ignored by OSPM
> >> + 3: reserved and should be ignored by OSPM
> >> + 4: if set to 1, OSPM requests firmware to perform device
> >> eject,
> >> + firmware shall clear this event by writing 1 into it
> >> before
> >> + performing device eject> + 5-7: reserved and
> >> should be ignored by OSPM
> >> [0x5-0x7] reserved
> >> [0x8] Command data: (DWORD access)
> >> contains 0 unless value last stored in 'Command field' is
> >> one of:
> >> @@ -82,7 +86,10 @@ write access:
> >> selected CPU device
> >> 3: if set to 1 initiates device eject, set by OSPM when it
> >> triggers CPU device removal and calls _EJ0 method
> >> - 4-7: reserved, OSPM must clear them before writing to
> >> register
> >> + 4: if set to 1 OSPM hands over device eject to firmware,
> >> + Firmware shall issue device eject request as described
> >> above
> >> + (bit #3) and OSPM should not touch device eject bit (#3),
> >> + 5-7: reserved, OSPM must clear them before writing to
> >> register
> >> [0x5] Command field: (1 byte access)
> >> value:
> >> 0: selects a CPU device with inserting/removing events and
> >> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> >> index f099b50927..09d2f20dae 100644
> >> --- a/hw/acpi/cpu.c
> >> +++ b/hw/acpi/cpu.c
> >> @@ -71,6 +71,7 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr
> >> addr, unsigned size)
> >> val |= cdev->cpu ? 1 : 0;
> >> val |= cdev->is_inserting ? 2 : 0;
> >> val |= cdev->is_removing ? 4 : 0;
> >> + val |= cdev->fw_remove ? 16 : 0;
> >
> > I might be missing something but I don't see where cdev->fw_remove is being
> > set.
>
> See just below, in the cpu_hotplug_wr() hunk. When bit#4 is written --
> which happens through the ACPI code change --, fw_remove is inverted.
>
>
> > We do set cdev->is_removing in acpi_cpu_unplug_request_cb() so AFAICS
> > we would always end up setting this bit:
> >> val |= cdev->is_removing ? 4 : 0;
> >
> > Also, if cdev->fw_remove and cdev->is_removing are both true, val would be
> > (4 | 16). I'm guessing that in that case the AML determines which case gets
> > handled but it might make sense to set just one of these?
>
> "is_removing" is set directly in response to the device_del QMP command.
> That QMP command is asynchronous to the execution of the guest OS.
its removing is notification to OSPM, which is cleared when ACPI scans
for events if I'm not mistaken.
>
> "fw_remove" is set (by virtue of inverting) by ACPI CEJ0, which is
> executed by the guest OS's ACPI interpreter, after the guest OS has
>
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On 2020-11-24 4:25 a.m., Igor Mammedov wrote:
If firmware negotiates ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT feature,
OSPM on CPU eject will set bit #4 in CPU hotplug block for to be
ejected CPU to mark it for removal by firmware and trigger SMI
upcall to let firmware do actual eject.
Signed-off-by: Igor Mammedov
---
PS:
- abuse 5.1 machine type for now to turn off unplug feature
(it will be moved to 5.2 machine type once new merge window is open)
---
include/hw/acpi/cpu.h | 2 ++
docs/specs/acpi_cpu_hotplug.txt | 11 +--
hw/acpi/cpu.c | 18 --
hw/i386/acpi-build.c| 5 +
hw/i386/pc.c| 1 +
hw/isa/lpc_ich9.c | 2 +-
6 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
index 0eeedaa491..999caaf510 100644
--- a/include/hw/acpi/cpu.h
+++ b/include/hw/acpi/cpu.h
@@ -22,6 +22,7 @@ typedef struct AcpiCpuStatus {
uint64_t arch_id;
bool is_inserting;
bool is_removing;
+bool fw_remove;
uint32_t ost_event;
uint32_t ost_status;
} AcpiCpuStatus;
@@ -50,6 +51,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
typedef struct CPUHotplugFeatures {
bool acpi_1_compatible;
bool has_legacy_cphp;
+bool fw_unplugs_cpu;
const char *smi_path;
} CPUHotplugFeatures;
diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
index 9bb22d1270..f68ef6e06c 100644
--- a/docs/specs/acpi_cpu_hotplug.txt
+++ b/docs/specs/acpi_cpu_hotplug.txt
@@ -57,7 +57,11 @@ read access:
It's valid only when bit 0 is set.
2: Device remove event, used to distinguish device for which
no device eject request to OSPM was issued.
- 3-7: reserved and should be ignored by OSPM
+ 3: reserved and should be ignored by OSPM
+ 4: if set to 1, OSPM requests firmware to perform device eject,
+ firmware shall clear this event by writing 1 into it before
+ performing device eject> + 5-7: reserved and should be
ignored by OSPM
[0x5-0x7] reserved
[0x8] Command data: (DWORD access)
contains 0 unless value last stored in 'Command field' is one of:
@@ -82,7 +86,10 @@ write access:
selected CPU device
3: if set to 1 initiates device eject, set by OSPM when it
triggers CPU device removal and calls _EJ0 method
-4-7: reserved, OSPM must clear them before writing to register
+4: if set to 1 OSPM hands over device eject to firmware,
+ Firmware shall issue device eject request as described above
+ (bit #3) and OSPM should not touch device eject bit (#3),
+5-7: reserved, OSPM must clear them before writing to register
[0x5] Command field: (1 byte access)
value:
0: selects a CPU device with inserting/removing events and
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index f099b50927..09d2f20dae 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -71,6 +71,7 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr,
unsigned size)
val |= cdev->cpu ? 1 : 0;
val |= cdev->is_inserting ? 2 : 0;
val |= cdev->is_removing ? 4 : 0;
+val |= cdev->fw_remove ? 16 : 0;
I might be missing something but I don't see where cdev->fw_remove is being
set. We do set cdev->is_removing in acpi_cpu_unplug_request_cb() so AFAICS
we would always end up setting this bit:
val |= cdev->is_removing ? 4 : 0;
Also, if cdev->fw_remove and cdev->is_removing are both true, val would be
(4 | 16). I'm guessing that in that case the AML determines which case gets
handled but it might make sense to set just one of these?
trace_cpuhp_acpi_read_flags(cpu_st->selector, val);
break;
case ACPI_CPU_CMD_DATA_OFFSET_RW:
@@ -148,6 +149,8 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr,
uint64_t data,
hotplug_ctrl = qdev_get_hotplug_handler(dev);
hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
object_unparent(OBJECT(dev));
+} else if (data & 16) {
+cdev->fw_remove = !cdev->fw_remove;
}
break;
case ACPI_CPU_CMD_OFFSET_WR:
@@ -332,6 +335,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
#define CPU_INSERT_EVENT "CINS"
#define CPU_REMOVE_EVENT "CRMV"
#define CPU_EJECT_EVENT "CEJ0"
+#define CPU_FW_EJECT_EVENT "CEJF"
void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
hwaddr io_base,
@@ -384,7 +388,10 @@ void build_cpus_aml(Aml *table, MachineState *machine,
CPUHotplugFeatures opts,
aml_append(field, aml_named_field(CPU_REMOVE_EVENT, 1));
/* initiates device eject, write only */
aml_append(field,
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On 11/26/20 11:24, Ankur Arora wrote:
> On 2020-11-24 4:25 a.m., Igor Mammedov wrote:
>> If firmware negotiates ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT feature,
>> OSPM on CPU eject will set bit #4 in CPU hotplug block for to be
>> ejected CPU to mark it for removal by firmware and trigger SMI
>> upcall to let firmware do actual eject.
>>
>> Signed-off-by: Igor Mammedov
>> ---
>> PS:
>> - abuse 5.1 machine type for now to turn off unplug feature
>> (it will be moved to 5.2 machine type once new merge window is open)
>> ---
>> include/hw/acpi/cpu.h | 2 ++
>> docs/specs/acpi_cpu_hotplug.txt | 11 +--
>> hw/acpi/cpu.c | 18 --
>> hw/i386/acpi-build.c | 5 +
>> hw/i386/pc.c | 1 +
>> hw/isa/lpc_ich9.c | 2 +-
>> 6 files changed, 34 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
>> index 0eeedaa491..999caaf510 100644
>> --- a/include/hw/acpi/cpu.h
>> +++ b/include/hw/acpi/cpu.h
>> @@ -22,6 +22,7 @@ typedef struct AcpiCpuStatus {
>> uint64_t arch_id;
>> bool is_inserting;
>> bool is_removing;
>> + bool fw_remove;
>> uint32_t ost_event;
>> uint32_t ost_status;
>> } AcpiCpuStatus;
>> @@ -50,6 +51,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object
>> *owner,
>> typedef struct CPUHotplugFeatures {
>> bool acpi_1_compatible;
>> bool has_legacy_cphp;
>> + bool fw_unplugs_cpu;
>> const char *smi_path;
>> } CPUHotplugFeatures;
>> diff --git a/docs/specs/acpi_cpu_hotplug.txt
>> b/docs/specs/acpi_cpu_hotplug.txt
>> index 9bb22d1270..f68ef6e06c 100644
>> --- a/docs/specs/acpi_cpu_hotplug.txt
>> +++ b/docs/specs/acpi_cpu_hotplug.txt
>> @@ -57,7 +57,11 @@ read access:
>> It's valid only when bit 0 is set.
>> 2: Device remove event, used to distinguish device for which
>> no device eject request to OSPM was issued.
>> - 3-7: reserved and should be ignored by OSPM
>> + 3: reserved and should be ignored by OSPM
>> + 4: if set to 1, OSPM requests firmware to perform device
>> eject,
>> + firmware shall clear this event by writing 1 into it
>> before
>> + performing device eject> + 5-7: reserved and
>> should be ignored by OSPM
>> [0x5-0x7] reserved
>> [0x8] Command data: (DWORD access)
>> contains 0 unless value last stored in 'Command field' is
>> one of:
>> @@ -82,7 +86,10 @@ write access:
>> selected CPU device
>> 3: if set to 1 initiates device eject, set by OSPM when it
>> triggers CPU device removal and calls _EJ0 method
>> - 4-7: reserved, OSPM must clear them before writing to
>> register
>> + 4: if set to 1 OSPM hands over device eject to firmware,
>> + Firmware shall issue device eject request as described
>> above
>> + (bit #3) and OSPM should not touch device eject bit (#3),
>> + 5-7: reserved, OSPM must clear them before writing to
>> register
>> [0x5] Command field: (1 byte access)
>> value:
>> 0: selects a CPU device with inserting/removing events and
>> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
>> index f099b50927..09d2f20dae 100644
>> --- a/hw/acpi/cpu.c
>> +++ b/hw/acpi/cpu.c
>> @@ -71,6 +71,7 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr
>> addr, unsigned size)
>> val |= cdev->cpu ? 1 : 0;
>> val |= cdev->is_inserting ? 2 : 0;
>> val |= cdev->is_removing ? 4 : 0;
>> + val |= cdev->fw_remove ? 16 : 0;
>
> I might be missing something but I don't see where cdev->fw_remove is being
> set.
See just below, in the cpu_hotplug_wr() hunk. When bit#4 is written --
which happens through the ACPI code change --, fw_remove is inverted.
> We do set cdev->is_removing in acpi_cpu_unplug_request_cb() so AFAICS
> we would always end up setting this bit:
>> val |= cdev->is_removing ? 4 : 0;
>
> Also, if cdev->fw_remove and cdev->is_removing are both true, val would be
> (4 | 16). I'm guessing that in that case the AML determines which case gets
> handled but it might make sense to set just one of these?
"is_removing" is set directly in response to the device_del QMP command.
That QMP command is asynchronous to the execution of the guest OS.
"fw_remove" is set (by virtue of inverting) by ACPI CEJ0, which is
executed by the guest OS's ACPI interpreter, after the guest OS has
de-scheduled all processes from the CPU being removed (= basically after
the OS has willfully forgotten about the CPU).
Therefore, considering the bitmask (is_removing, fw_remove), three
variations make sense:
#1 (is_removing=0, fw_remove=0) -- normal status; no unplug requested
#2 (is_removing=1, fw_remove=0) -- unplug requested via QMP, guest OS
is
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On 11/24/20 13:25, Igor Mammedov wrote:
> If firmware negotiates ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT feature,
> OSPM on CPU eject will set bit #4 in CPU hotplug block for to be
> ejected CPU to mark it for removal by firmware and trigger SMI
> upcall to let firmware do actual eject.
>
> Signed-off-by: Igor Mammedov
> ---
> PS:
> - abuse 5.1 machine type for now to turn off unplug feature
> (it will be moved to 5.2 machine type once new merge window is open)
> ---
> include/hw/acpi/cpu.h | 2 ++
> docs/specs/acpi_cpu_hotplug.txt | 11 +--
> hw/acpi/cpu.c | 18 --
> hw/i386/acpi-build.c| 5 +
> hw/i386/pc.c| 1 +
> hw/isa/lpc_ich9.c | 2 +-
> 6 files changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> index 0eeedaa491..999caaf510 100644
> --- a/include/hw/acpi/cpu.h
> +++ b/include/hw/acpi/cpu.h
> @@ -22,6 +22,7 @@ typedef struct AcpiCpuStatus {
> uint64_t arch_id;
> bool is_inserting;
> bool is_removing;
> +bool fw_remove;
> uint32_t ost_event;
> uint32_t ost_status;
> } AcpiCpuStatus;
> @@ -50,6 +51,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
> typedef struct CPUHotplugFeatures {
> bool acpi_1_compatible;
> bool has_legacy_cphp;
> +bool fw_unplugs_cpu;
> const char *smi_path;
> } CPUHotplugFeatures;
>
> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> index 9bb22d1270..f68ef6e06c 100644
> --- a/docs/specs/acpi_cpu_hotplug.txt
> +++ b/docs/specs/acpi_cpu_hotplug.txt
> @@ -57,7 +57,11 @@ read access:
>It's valid only when bit 0 is set.
> 2: Device remove event, used to distinguish device for which
>no device eject request to OSPM was issued.
> - 3-7: reserved and should be ignored by OSPM
> + 3: reserved and should be ignored by OSPM
> + 4: if set to 1, OSPM requests firmware to perform device eject,
> + firmware shall clear this event by writing 1 into it before
(1) s/clear this event/clear this event bit/
> + performing device eject.
(2) move the second and third lines ("firmware shall clear") over to
the write documentation, below? In particular:
> + 5-7: reserved and should be ignored by OSPM
> [0x5-0x7] reserved
> [0x8] Command data: (DWORD access)
>contains 0 unless value last stored in 'Command field' is one of:
> @@ -82,7 +86,10 @@ write access:
> selected CPU device
> 3: if set to 1 initiates device eject, set by OSPM when it
> triggers CPU device removal and calls _EJ0 method
> -4-7: reserved, OSPM must clear them before writing to register
> +4: if set to 1 OSPM hands over device eject to firmware,
> + Firmware shall issue device eject request as described above
> + (bit #3) and OSPM should not touch device eject bit (#3),
(3) it would be clearer if we documented the exact bit writing order
here:
- clear bit#4, *then* set bit#3 (two write accesses)
- versus clear bit#4 *and* set bit#3 (single access)
> +5-7: reserved, OSPM must clear them before writing to register
> [0x5] Command field: (1 byte access)
>value:
> 0: selects a CPU device with inserting/removing events and
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index f099b50927..09d2f20dae 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -71,6 +71,7 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr,
> unsigned size)
> val |= cdev->cpu ? 1 : 0;
> val |= cdev->is_inserting ? 2 : 0;
> val |= cdev->is_removing ? 4 : 0;
> +val |= cdev->fw_remove ? 16 : 0;
> trace_cpuhp_acpi_read_flags(cpu_st->selector, val);
> break;
> case ACPI_CPU_CMD_DATA_OFFSET_RW:
> @@ -148,6 +149,8 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr,
> uint64_t data,
> hotplug_ctrl = qdev_get_hotplug_handler(dev);
> hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
> object_unparent(OBJECT(dev));
> +} else if (data & 16) {
> +cdev->fw_remove = !cdev->fw_remove;
hm... so I guess the ACPI code will first write bit#4 to flip
"fw_remove" from "off" to "on". Then the firmware will write bit#4 to
flip "fw_remove" back to "off". And finally, the firmware will write
bit#3 (strictly as a separate access) to unplug the CPU.
(4) But anyway, taking a step back: what do we need the new bit for?
> }
> break;
> case ACPI_CPU_CMD_OFFSET_WR:
> @@ -332,6 +335,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
> #define CPU_INSERT_EVENT "CINS"
> #define CPU_REMOVE_EVENT "CRMV"
> #define CPU_EJECT_EVENT "CEJ0"
> +#define CPU_FW_EJECT_EVENT "CEJF"
>
> void build_cpus_aml(Aml
Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
On 11/24/20 13:25, Igor Mammedov wrote:
> If firmware negotiates ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT feature,
> OSPM on CPU eject will set bit #4 in CPU hotplug block for to be
> ejected CPU to mark it for removal by firmware and trigger SMI
> upcall to let firmware do actual eject.
>
> Signed-off-by: Igor Mammedov
> ---
> PS:
> - abuse 5.1 machine type for now to turn off unplug feature
> (it will be moved to 5.2 machine type once new merge window is open)
> ---
> include/hw/acpi/cpu.h | 2 ++
> docs/specs/acpi_cpu_hotplug.txt | 11 +--
> hw/acpi/cpu.c | 18 --
> hw/i386/acpi-build.c| 5 +
> hw/i386/pc.c| 1 +
> hw/isa/lpc_ich9.c | 2 +-
> 6 files changed, 34 insertions(+), 5 deletions(-)
Thanks -- I've tagged this for later; I can't tell when I'll come to it.
I'll have to re-read the previous discussion, from start of October, first.
Ankur -- please feel free to comment!
Thanks
Laszlo
>
> diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> index 0eeedaa491..999caaf510 100644
> --- a/include/hw/acpi/cpu.h
> +++ b/include/hw/acpi/cpu.h
> @@ -22,6 +22,7 @@ typedef struct AcpiCpuStatus {
> uint64_t arch_id;
> bool is_inserting;
> bool is_removing;
> +bool fw_remove;
> uint32_t ost_event;
> uint32_t ost_status;
> } AcpiCpuStatus;
> @@ -50,6 +51,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
> typedef struct CPUHotplugFeatures {
> bool acpi_1_compatible;
> bool has_legacy_cphp;
> +bool fw_unplugs_cpu;
> const char *smi_path;
> } CPUHotplugFeatures;
>
> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> index 9bb22d1270..f68ef6e06c 100644
> --- a/docs/specs/acpi_cpu_hotplug.txt
> +++ b/docs/specs/acpi_cpu_hotplug.txt
> @@ -57,7 +57,11 @@ read access:
>It's valid only when bit 0 is set.
> 2: Device remove event, used to distinguish device for which
>no device eject request to OSPM was issued.
> - 3-7: reserved and should be ignored by OSPM
> + 3: reserved and should be ignored by OSPM
> + 4: if set to 1, OSPM requests firmware to perform device eject,
> + firmware shall clear this event by writing 1 into it before
> + performing device eject.
> + 5-7: reserved and should be ignored by OSPM
> [0x5-0x7] reserved
> [0x8] Command data: (DWORD access)
>contains 0 unless value last stored in 'Command field' is one of:
> @@ -82,7 +86,10 @@ write access:
> selected CPU device
> 3: if set to 1 initiates device eject, set by OSPM when it
> triggers CPU device removal and calls _EJ0 method
> -4-7: reserved, OSPM must clear them before writing to register
> +4: if set to 1 OSPM hands over device eject to firmware,
> + Firmware shall issue device eject request as described above
> + (bit #3) and OSPM should not touch device eject bit (#3),
> +5-7: reserved, OSPM must clear them before writing to register
> [0x5] Command field: (1 byte access)
>value:
> 0: selects a CPU device with inserting/removing events and
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index f099b50927..09d2f20dae 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -71,6 +71,7 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr,
> unsigned size)
> val |= cdev->cpu ? 1 : 0;
> val |= cdev->is_inserting ? 2 : 0;
> val |= cdev->is_removing ? 4 : 0;
> +val |= cdev->fw_remove ? 16 : 0;
> trace_cpuhp_acpi_read_flags(cpu_st->selector, val);
> break;
> case ACPI_CPU_CMD_DATA_OFFSET_RW:
> @@ -148,6 +149,8 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr,
> uint64_t data,
> hotplug_ctrl = qdev_get_hotplug_handler(dev);
> hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
> object_unparent(OBJECT(dev));
> +} else if (data & 16) {
> +cdev->fw_remove = !cdev->fw_remove;
> }
> break;
> case ACPI_CPU_CMD_OFFSET_WR:
> @@ -332,6 +335,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
> #define CPU_INSERT_EVENT "CINS"
> #define CPU_REMOVE_EVENT "CRMV"
> #define CPU_EJECT_EVENT "CEJ0"
> +#define CPU_FW_EJECT_EVENT "CEJF"
>
> void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures
> opts,
> hwaddr io_base,
> @@ -384,7 +388,10 @@ void build_cpus_aml(Aml *table, MachineState *machine,
> CPUHotplugFeatures opts,
> aml_append(field, aml_named_field(CPU_REMOVE_EVENT, 1));
> /* initiates device eject, write only */
> aml_append(field, aml_named_field(CPU_EJECT_EVENT, 1));
> -aml_append(field, aml_reserved_field(4));
> +aml_append(field,
