| Message ID | 20201204170939.1815522-3-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add support for cpu hot-unplug with SMI broadcast enabled | expand |
On 2020-12-04 9:09 a.m., Igor Mammedov wrote: > Adds bit #4 to status/control field of CPU hotplug MMIO interface. > New bit will be used OSPM to mark CPUs as pending for removal by firmware, > when it calls _EJ0 method on CPU device node. Later on, when firmware > sees this bit set, it will perform CPU eject which will clear bit #4 > as well. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > v1: > - rearrange status/control bits description (Laszlo) > - add clear bit #4 on eject > - drop toggling logic from bit #4, it can be only set by guest > and clear as part of cpu eject > - exclude boot CPU from remove request > - add trace events for new bit > --- > include/hw/acpi/cpu.h | 1 + > docs/specs/acpi_cpu_hotplug.txt | 19 ++++++++++++++----- > hw/acpi/cpu.c | 9 +++++++++ > hw/acpi/trace-events | 2 ++ > 4 files changed, 26 insertions(+), 5 deletions(-) > > diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h > index 0eeedaa491..d71edde456 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; > diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt > index 9bb22d1270..9bd59ae0da 100644 > --- a/docs/specs/acpi_cpu_hotplug.txt > +++ b/docs/specs/acpi_cpu_hotplug.txt > @@ -56,8 +56,11 @@ read access: > no device check event to OSPM was issued. > 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 > + no device eject request to OSPM was issued. Firmware must > + ignore this bit. > + 3: reserved and should be ignored by OSPM > + 4: if set to 1, OSPM requests firmware to perform 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: > @@ -79,10 +82,16 @@ write access: > selected CPU device > 2: if set to 1 clears device remove event, set by OSPM > after it has emitted device eject request for the > - selected CPU device > + 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 > + triggers CPU device removal and calls _EJ0 method or by firmware > + when bit #4 is set. In case bit #4 were set, it's cleared as > + part of device eject. > + 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) in case > + it's asked firmware to perform CPU device 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..811218f673 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,14 @@ 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)); > + cdev->fw_remove = false; > + } else if (data & 16) { > + if (!cdev->cpu || cdev->cpu == first_cpu) { > + trace_cpuhp_acpi_fw_remove_invalid_cpu(cpu_st->selector); > + break; > + } > + trace_cpuhp_acpi_fw_remove_cpu(cpu_st->selector); > + cdev->fw_remove = true; > } > break; By the time the firmware gets the MMI, cdev->is_removing == 0. So we probably need the cdev->fw_remove clause as well. @@ -168,7 +193,7 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr, uint64_t data, do { cdev = &cpu_st->devs[iter]; - if (cdev->is_inserting || cdev->is_removing) { + if (cdev->is_inserting || cdev->is_removing || cdev->fw_remove) { cpu_st->selector = iter; trace_cpuhp_acpi_cpu_has_events(cpu_st->selector, cdev->is_inserting, cdev->is_removing); Ankur > case ACPI_CPU_CMD_OFFSET_WR: > diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events > index afbc77de1c..f91ced477d 100644 > --- a/hw/acpi/trace-events > +++ b/hw/acpi/trace-events > @@ -29,6 +29,8 @@ cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]" > cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]" > cpuhp_acpi_ejecting_invalid_cpu(uint32_t idx) "0x%"PRIx32 > cpuhp_acpi_ejecting_cpu(uint32_t idx) "0x%"PRIx32 > +cpuhp_acpi_fw_remove_invalid_cpu(uint32_t idx) "0x%"PRIx32 > +cpuhp_acpi_fw_remove_cpu(uint32_t idx) "0x%"PRIx32 > cpuhp_acpi_write_ost_ev(uint32_t slot, uint32_t ev) "idx[0x%"PRIx32"] OST EVENT: 0x%"PRIx32 > cpuhp_acpi_write_ost_status(uint32_t slot, uint32_t st) "idx[0x%"PRIx32"] OST STATUS: 0x%"PRIx32 > >
On 2020-12-06 10:31 p.m., Ankur Arora wrote: > On 2020-12-04 9:09 a.m., Igor Mammedov wrote: >> Adds bit #4 to status/control field of CPU hotplug MMIO interface. >> New bit will be used OSPM to mark CPUs as pending for removal by firmware, >> when it calls _EJ0 method on CPU device node. Later on, when firmware >> sees this bit set, it will perform CPU eject which will clear bit #4 >> as well. >> >> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >> --- >> v1: >> - rearrange status/control bits description (Laszlo) >> - add clear bit #4 on eject >> - drop toggling logic from bit #4, it can be only set by guest >> and clear as part of cpu eject >> - exclude boot CPU from remove request >> - add trace events for new bit >> --- >> include/hw/acpi/cpu.h | 1 + >> docs/specs/acpi_cpu_hotplug.txt | 19 ++++++++++++++----- >> hw/acpi/cpu.c | 9 +++++++++ >> hw/acpi/trace-events | 2 ++ >> 4 files changed, 26 insertions(+), 5 deletions(-) >> >> diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h >> index 0eeedaa491..d71edde456 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; >> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt >> index 9bb22d1270..9bd59ae0da 100644 >> --- a/docs/specs/acpi_cpu_hotplug.txt >> +++ b/docs/specs/acpi_cpu_hotplug.txt >> @@ -56,8 +56,11 @@ read access: >> no device check event to OSPM was issued. >> 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 >> + no device eject request to OSPM was issued. Firmware must >> + ignore this bit. >> + 3: reserved and should be ignored by OSPM >> + 4: if set to 1, OSPM requests firmware to perform 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: >> @@ -79,10 +82,16 @@ write access: >> selected CPU device >> 2: if set to 1 clears device remove event, set by OSPM >> after it has emitted device eject request for the >> - selected CPU device >> + 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 >> + triggers CPU device removal and calls _EJ0 method or by firmware >> + when bit #4 is set. In case bit #4 were set, it's cleared as >> + part of device eject. So I spent some time testing my OVMF series alongside this. Just sent out the code on the EDK2 list. To do the eject, I'm setting bit#3 after queuing up the processor for removal (via RemoveProcessor()). This means, however, that the unplug in QEMU would happen before the real firmware unplug (which'll happen in SmmCpuUpdate()). Clearly, the right place for eject would be either in the appropriate APHandler() or in the BSPHandler() after all the APs have been waited for but from my reading of related code I don't see any infrastructure for doing this (admittedly, I don't know the EDK2 source well enough so it's likely I missed something.) The other possibility might be to do it in the _EJ0 method itself after we return from the SMI: @@ -479,9 +515,8 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts, 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_store(one, ej_evt)); aml_append(method, aml_release(ctrl_lock)); This seems to work but it is possible I'm missing something here. Opinions? Ankur >> + 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) in case >> + it's asked firmware to perform CPU device 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..811218f673 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,14 @@ 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)); >> + cdev->fw_remove = false; >> + } else if (data & 16) { >> + if (!cdev->cpu || cdev->cpu == first_cpu) { >> + trace_cpuhp_acpi_fw_remove_invalid_cpu(cpu_st->selector); >> + break; >> + } >> + trace_cpuhp_acpi_fw_remove_cpu(cpu_st->selector); >> + cdev->fw_remove = true; >> } >> break; > > By the time the firmware gets the MMI, cdev->is_removing == 0. So we probably > need the cdev->fw_remove clause as well. > > @@ -168,7 +193,7 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr, uint64_t data, > > do { > cdev = &cpu_st->devs[iter]; > - if (cdev->is_inserting || cdev->is_removing) { > + if (cdev->is_inserting || cdev->is_removing || cdev->fw_remove) { > cpu_st->selector = iter; > trace_cpuhp_acpi_cpu_has_events(cpu_st->selector, > cdev->is_inserting, cdev->is_removing); > > > Ankur > >> case ACPI_CPU_CMD_OFFSET_WR: >> diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events >> index afbc77de1c..f91ced477d 100644 >> --- a/hw/acpi/trace-events >> +++ b/hw/acpi/trace-events >> @@ -29,6 +29,8 @@ cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]" >> cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]" >> cpuhp_acpi_ejecting_invalid_cpu(uint32_t idx) "0x%"PRIx32 >> cpuhp_acpi_ejecting_cpu(uint32_t idx) "0x%"PRIx32 >> +cpuhp_acpi_fw_remove_invalid_cpu(uint32_t idx) "0x%"PRIx32 >> +cpuhp_acpi_fw_remove_cpu(uint32_t idx) "0x%"PRIx32 >> cpuhp_acpi_write_ost_ev(uint32_t slot, uint32_t ev) "idx[0x%"PRIx32"] OST EVENT: 0x%"PRIx32 >> cpuhp_acpi_write_ost_status(uint32_t slot, uint32_t st) "idx[0x%"PRIx32"] OST STATUS: 0x%"PRIx32 >>
On Sun, 6 Dec 2020 22:31:50 -0800 Ankur Arora <ankur.a.arora@oracle.com> wrote: > On 2020-12-04 9:09 a.m., Igor Mammedov wrote: > > Adds bit #4 to status/control field of CPU hotplug MMIO interface. > > New bit will be used OSPM to mark CPUs as pending for removal by firmware, > > when it calls _EJ0 method on CPU device node. Later on, when firmware > > sees this bit set, it will perform CPU eject which will clear bit #4 > > as well. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > v1: > > - rearrange status/control bits description (Laszlo) > > - add clear bit #4 on eject > > - drop toggling logic from bit #4, it can be only set by guest > > and clear as part of cpu eject > > - exclude boot CPU from remove request > > - add trace events for new bit > > --- > > include/hw/acpi/cpu.h | 1 + > > docs/specs/acpi_cpu_hotplug.txt | 19 ++++++++++++++----- > > hw/acpi/cpu.c | 9 +++++++++ > > hw/acpi/trace-events | 2 ++ > > 4 files changed, 26 insertions(+), 5 deletions(-) > > > > diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h > > index 0eeedaa491..d71edde456 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; > > diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt > > index 9bb22d1270..9bd59ae0da 100644 > > --- a/docs/specs/acpi_cpu_hotplug.txt > > +++ b/docs/specs/acpi_cpu_hotplug.txt > > @@ -56,8 +56,11 @@ read access: > > no device check event to OSPM was issued. > > 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 > > + no device eject request to OSPM was issued. Firmware must > > + ignore this bit. > > + 3: reserved and should be ignored by OSPM > > + 4: if set to 1, OSPM requests firmware to perform 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: > > @@ -79,10 +82,16 @@ write access: > > selected CPU device > > 2: if set to 1 clears device remove event, set by OSPM > > after it has emitted device eject request for the > > - selected CPU device > > + 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 > > + triggers CPU device removal and calls _EJ0 method or by firmware > > + when bit #4 is set. In case bit #4 were set, it's cleared as > > + part of device eject. > > + 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) in case > > + it's asked firmware to perform CPU device 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..811218f673 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,14 @@ 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)); > > + cdev->fw_remove = false; > > + } else if (data & 16) { > > + if (!cdev->cpu || cdev->cpu == first_cpu) { > > + trace_cpuhp_acpi_fw_remove_invalid_cpu(cpu_st->selector); > > + break; > > + } > > + trace_cpuhp_acpi_fw_remove_cpu(cpu_st->selector); > > + cdev->fw_remove = true; > > } > > break; > > By the time the firmware gets the MMI, cdev->is_removing == 0. So we probably > need the cdev->fw_remove clause as well. indeed, I'll fix in in v2 > > @@ -168,7 +193,7 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr, uint64_t data, > > do { > cdev = &cpu_st->devs[iter]; > - if (cdev->is_inserting || cdev->is_removing) { > + if (cdev->is_inserting || cdev->is_removing || cdev->fw_remove) { > cpu_st->selector = iter; > trace_cpuhp_acpi_cpu_has_events(cpu_st->selector, > cdev->is_inserting, cdev->is_removing); > > > Ankur > > > case ACPI_CPU_CMD_OFFSET_WR: > > diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events > > index afbc77de1c..f91ced477d 100644 > > --- a/hw/acpi/trace-events > > +++ b/hw/acpi/trace-events > > @@ -29,6 +29,8 @@ cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]" > > cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]" > > cpuhp_acpi_ejecting_invalid_cpu(uint32_t idx) "0x%"PRIx32 > > cpuhp_acpi_ejecting_cpu(uint32_t idx) "0x%"PRIx32 > > +cpuhp_acpi_fw_remove_invalid_cpu(uint32_t idx) "0x%"PRIx32 > > +cpuhp_acpi_fw_remove_cpu(uint32_t idx) "0x%"PRIx32 > > cpuhp_acpi_write_ost_ev(uint32_t slot, uint32_t ev) "idx[0x%"PRIx32"] OST EVENT: 0x%"PRIx32 > > cpuhp_acpi_write_ost_status(uint32_t slot, uint32_t st) "idx[0x%"PRIx32"] OST STATUS: 0x%"PRIx32 > > > > >
On Mon, 7 Dec 2020 00:47:13 -0800 Ankur Arora <ankur.a.arora@oracle.com> wrote: > On 2020-12-06 10:31 p.m., Ankur Arora wrote: > > On 2020-12-04 9:09 a.m., Igor Mammedov wrote: > >> Adds bit #4 to status/control field of CPU hotplug MMIO interface. > >> New bit will be used OSPM to mark CPUs as pending for removal by firmware, > >> when it calls _EJ0 method on CPU device node. Later on, when firmware > >> sees this bit set, it will perform CPU eject which will clear bit #4 > >> as well. > >> > >> Signed-off-by: Igor Mammedov <imammedo@redhat.com> > >> --- > >> v1: > >> - rearrange status/control bits description (Laszlo) > >> - add clear bit #4 on eject > >> - drop toggling logic from bit #4, it can be only set by guest > >> and clear as part of cpu eject > >> - exclude boot CPU from remove request > >> - add trace events for new bit > >> --- > >> include/hw/acpi/cpu.h | 1 + > >> docs/specs/acpi_cpu_hotplug.txt | 19 ++++++++++++++----- > >> hw/acpi/cpu.c | 9 +++++++++ > >> hw/acpi/trace-events | 2 ++ > >> 4 files changed, 26 insertions(+), 5 deletions(-) > >> > >> diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h > >> index 0eeedaa491..d71edde456 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; > >> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt > >> index 9bb22d1270..9bd59ae0da 100644 > >> --- a/docs/specs/acpi_cpu_hotplug.txt > >> +++ b/docs/specs/acpi_cpu_hotplug.txt > >> @@ -56,8 +56,11 @@ read access: > >> no device check event to OSPM was issued. > >> 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 > >> + no device eject request to OSPM was issued. Firmware must > >> + ignore this bit. > >> + 3: reserved and should be ignored by OSPM > >> + 4: if set to 1, OSPM requests firmware to perform 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: > >> @@ -79,10 +82,16 @@ write access: > >> selected CPU device > >> 2: if set to 1 clears device remove event, set by OSPM > >> after it has emitted device eject request for the > >> - selected CPU device > >> + 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 > >> + triggers CPU device removal and calls _EJ0 method or by firmware > >> + when bit #4 is set. In case bit #4 were set, it's cleared as > >> + part of device eject. > > So I spent some time testing my OVMF series alongside this. > Just sent out the code on the EDK2 list. > > To do the eject, I'm setting bit#3 after queuing up the processor > for removal (via RemoveProcessor()). > > This means, however, that the unplug in QEMU would happen before the > real firmware unplug (which'll happen in SmmCpuUpdate()). > > Clearly, the right place for eject would be either in the appropriate > APHandler() or in the BSPHandler() after all the APs have been waited > for but from my reading of related code I don't see any infrastructure > for doing this (admittedly, I don't know the EDK2 source well enough > so it's likely I missed something.) > > The other possibility might be to do it in the _EJ0 method itself > after we return from the SMI: > > @@ -479,9 +515,8 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts, > 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_store(one, ej_evt)); > aml_append(method, aml_release(ctrl_lock)); > > This seems to work but it is possible I'm missing something here. theoretically this leaves unaccounted CPU on fw side, what if SMM is entered again before CPU is ejected or OS doesn't eject it on purpose? I'd prefer firmware to remove CPU before returning from SMM. > > > Opinions? > > Ankur > > >> + 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) in case > >> + it's asked firmware to perform CPU device 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..811218f673 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,14 @@ 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)); > >> + cdev->fw_remove = false; > >> + } else if (data & 16) { > >> + if (!cdev->cpu || cdev->cpu == first_cpu) { > >> + trace_cpuhp_acpi_fw_remove_invalid_cpu(cpu_st->selector); > >> + break; > >> + } > >> + trace_cpuhp_acpi_fw_remove_cpu(cpu_st->selector); > >> + cdev->fw_remove = true; > >> } > >> break; > > > > By the time the firmware gets the MMI, cdev->is_removing == 0. So we probably > > need the cdev->fw_remove clause as well. > > > > @@ -168,7 +193,7 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr, uint64_t data, > > > > do { > > cdev = &cpu_st->devs[iter]; > > - if (cdev->is_inserting || cdev->is_removing) { > > + if (cdev->is_inserting || cdev->is_removing || cdev->fw_remove) { > > cpu_st->selector = iter; > > trace_cpuhp_acpi_cpu_has_events(cpu_st->selector, > > cdev->is_inserting, cdev->is_removing); > > > > > > Ankur > > > >> case ACPI_CPU_CMD_OFFSET_WR: > >> diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events > >> index afbc77de1c..f91ced477d 100644 > >> --- a/hw/acpi/trace-events > >> +++ b/hw/acpi/trace-events > >> @@ -29,6 +29,8 @@ cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]" > >> cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]" > >> cpuhp_acpi_ejecting_invalid_cpu(uint32_t idx) "0x%"PRIx32 > >> cpuhp_acpi_ejecting_cpu(uint32_t idx) "0x%"PRIx32 > >> +cpuhp_acpi_fw_remove_invalid_cpu(uint32_t idx) "0x%"PRIx32 > >> +cpuhp_acpi_fw_remove_cpu(uint32_t idx) "0x%"PRIx32 > >> cpuhp_acpi_write_ost_ev(uint32_t slot, uint32_t ev) "idx[0x%"PRIx32"] OST EVENT: 0x%"PRIx32 > >> cpuhp_acpi_write_ost_status(uint32_t slot, uint32_t st) "idx[0x%"PRIx32"] OST STATUS: 0x%"PRIx32 > >> >
On 2020-12-07 4:48 a.m., Igor Mammedov wrote: > On Mon, 7 Dec 2020 00:47:13 -0800 > Ankur Arora <ankur.a.arora@oracle.com> wrote: > >> On 2020-12-06 10:31 p.m., Ankur Arora wrote: >>> On 2020-12-04 9:09 a.m., Igor Mammedov wrote: >>>> Adds bit #4 to status/control field of CPU hotplug MMIO interface. >>>> New bit will be used OSPM to mark CPUs as pending for removal by firmware, >>>> when it calls _EJ0 method on CPU device node. Later on, when firmware >>>> sees this bit set, it will perform CPU eject which will clear bit #4 >>>> as well. >>>> >>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >>>> --- >>>> v1: >>>> - rearrange status/control bits description (Laszlo) >>>> - add clear bit #4 on eject >>>> - drop toggling logic from bit #4, it can be only set by guest >>>> and clear as part of cpu eject >>>> - exclude boot CPU from remove request >>>> - add trace events for new bit >>>> --- >>>> include/hw/acpi/cpu.h | 1 + >>>> docs/specs/acpi_cpu_hotplug.txt | 19 ++++++++++++++----- >>>> hw/acpi/cpu.c | 9 +++++++++ >>>> hw/acpi/trace-events | 2 ++ >>>> 4 files changed, 26 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h >>>> index 0eeedaa491..d71edde456 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; >>>> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt >>>> index 9bb22d1270..9bd59ae0da 100644 >>>> --- a/docs/specs/acpi_cpu_hotplug.txt >>>> +++ b/docs/specs/acpi_cpu_hotplug.txt >>>> @@ -56,8 +56,11 @@ read access: >>>> no device check event to OSPM was issued. >>>> 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 >>>> + no device eject request to OSPM was issued. Firmware must >>>> + ignore this bit. >>>> + 3: reserved and should be ignored by OSPM >>>> + 4: if set to 1, OSPM requests firmware to perform 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: >>>> @@ -79,10 +82,16 @@ write access: >>>> selected CPU device >>>> 2: if set to 1 clears device remove event, set by OSPM >>>> after it has emitted device eject request for the >>>> - selected CPU device >>>> + 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 >>>> + triggers CPU device removal and calls _EJ0 method or by firmware >>>> + when bit #4 is set. In case bit #4 were set, it's cleared as >>>> + part of device eject. >> >> So I spent some time testing my OVMF series alongside this. >> Just sent out the code on the EDK2 list. >> >> To do the eject, I'm setting bit#3 after queuing up the processor >> for removal (via RemoveProcessor()). >> >> This means, however, that the unplug in QEMU would happen before the >> real firmware unplug (which'll happen in SmmCpuUpdate()). >> >> Clearly, the right place for eject would be either in the appropriate >> APHandler() or in the BSPHandler() after all the APs have been waited >> for but from my reading of related code I don't see any infrastructure >> for doing this (admittedly, I don't know the EDK2 source well enough >> so it's likely I missed something.) >> >> The other possibility might be to do it in the _EJ0 method itself >> after we return from the SMI: >> >> @@ -479,9 +515,8 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts, >> 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_store(one, ej_evt)); >> aml_append(method, aml_release(ctrl_lock)); >> >> This seems to work but it is possible I'm missing something here. > > theoretically this leaves unaccounted CPU on fw side, On the guest side right? As in the firmware has marked it gone but it's not really gone. > what if SMM is entered again before CPU is ejected or OS doesn't eject it on purpose? Yeah both of those things would be a mess. I'm not even sure it would enter the SMM again -- given that the SMM has marked it gone. > > I'd prefer firmware to remove CPU before returning from SMM. That would be ideal though I don't yet see a mechanism for doing that. Laszlo might have better ideas though. Another possibility that might address your concerns would be deferred removal. Need to work out the details, but here's a sketch of what I'm thinking: When the firmware writes to bit#3, QEMU marks the CPU for deferred removal. And, when the firmware exits from the SMI handler, we force a VMEXIT before returning to the guest. And, as part of this VMEXIT, QEMU does the actual unplug. Ankur > > >> >> >> Opinions? >> >> Ankur >> >>>> + 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) in case >>>> + it's asked firmware to perform CPU device 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..811218f673 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,14 @@ 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)); >>>> + cdev->fw_remove = false; >>>> + } else if (data & 16) { >>>> + if (!cdev->cpu || cdev->cpu == first_cpu) { >>>> + trace_cpuhp_acpi_fw_remove_invalid_cpu(cpu_st->selector); >>>> + break; >>>> + } >>>> + trace_cpuhp_acpi_fw_remove_cpu(cpu_st->selector); >>>> + cdev->fw_remove = true; >>>> } >>>> break; >>> >>> By the time the firmware gets the MMI, cdev->is_removing == 0. So we probably >>> need the cdev->fw_remove clause as well. >>> >>> @@ -168,7 +193,7 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr, uint64_t data, >>> >>> do { >>> cdev = &cpu_st->devs[iter]; >>> - if (cdev->is_inserting || cdev->is_removing) { >>> + if (cdev->is_inserting || cdev->is_removing || cdev->fw_remove) { >>> cpu_st->selector = iter; >>> trace_cpuhp_acpi_cpu_has_events(cpu_st->selector, >>> cdev->is_inserting, cdev->is_removing); >>> >>> >>> Ankur >>> >>>> case ACPI_CPU_CMD_OFFSET_WR: >>>> diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events >>>> index afbc77de1c..f91ced477d 100644 >>>> --- a/hw/acpi/trace-events >>>> +++ b/hw/acpi/trace-events >>>> @@ -29,6 +29,8 @@ cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]" >>>> cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]" >>>> cpuhp_acpi_ejecting_invalid_cpu(uint32_t idx) "0x%"PRIx32 >>>> cpuhp_acpi_ejecting_cpu(uint32_t idx) "0x%"PRIx32 >>>> +cpuhp_acpi_fw_remove_invalid_cpu(uint32_t idx) "0x%"PRIx32 >>>> +cpuhp_acpi_fw_remove_cpu(uint32_t idx) "0x%"PRIx32 >>>> cpuhp_acpi_write_ost_ev(uint32_t slot, uint32_t ev) "idx[0x%"PRIx32"] OST EVENT: 0x%"PRIx32 >>>> cpuhp_acpi_write_ost_status(uint32_t slot, uint32_t st) "idx[0x%"PRIx32"] OST STATUS: 0x%"PRIx32 >>>> >> >
diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h index 0eeedaa491..d71edde456 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; diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt index 9bb22d1270..9bd59ae0da 100644 --- a/docs/specs/acpi_cpu_hotplug.txt +++ b/docs/specs/acpi_cpu_hotplug.txt @@ -56,8 +56,11 @@ read access: no device check event to OSPM was issued. 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 + no device eject request to OSPM was issued. Firmware must + ignore this bit. + 3: reserved and should be ignored by OSPM + 4: if set to 1, OSPM requests firmware to perform 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: @@ -79,10 +82,16 @@ write access: selected CPU device 2: if set to 1 clears device remove event, set by OSPM after it has emitted device eject request for the - selected CPU device + 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 + triggers CPU device removal and calls _EJ0 method or by firmware + when bit #4 is set. In case bit #4 were set, it's cleared as + part of device eject. + 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) in case + it's asked firmware to perform CPU device 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..811218f673 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,14 @@ 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)); + cdev->fw_remove = false; + } else if (data & 16) { + if (!cdev->cpu || cdev->cpu == first_cpu) { + trace_cpuhp_acpi_fw_remove_invalid_cpu(cpu_st->selector); + break; + } + trace_cpuhp_acpi_fw_remove_cpu(cpu_st->selector); + cdev->fw_remove = true; } break; case ACPI_CPU_CMD_OFFSET_WR: diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events index afbc77de1c..f91ced477d 100644 --- a/hw/acpi/trace-events +++ b/hw/acpi/trace-events @@ -29,6 +29,8 @@ cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]" cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]" cpuhp_acpi_ejecting_invalid_cpu(uint32_t idx) "0x%"PRIx32 cpuhp_acpi_ejecting_cpu(uint32_t idx) "0x%"PRIx32 +cpuhp_acpi_fw_remove_invalid_cpu(uint32_t idx) "0x%"PRIx32 +cpuhp_acpi_fw_remove_cpu(uint32_t idx) "0x%"PRIx32 cpuhp_acpi_write_ost_ev(uint32_t slot, uint32_t ev) "idx[0x%"PRIx32"] OST EVENT: 0x%"PRIx32 cpuhp_acpi_write_ost_status(uint32_t slot, uint32_t st) "idx[0x%"PRIx32"] OST STATUS: 0x%"PRIx32
Adds bit #4 to status/control field of CPU hotplug MMIO interface. New bit will be used OSPM to mark CPUs as pending for removal by firmware, when it calls _EJ0 method on CPU device node. Later on, when firmware sees this bit set, it will perform CPU eject which will clear bit #4 as well. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- v1: - rearrange status/control bits description (Laszlo) - add clear bit #4 on eject - drop toggling logic from bit #4, it can be only set by guest and clear as part of cpu eject - exclude boot CPU from remove request - add trace events for new bit --- include/hw/acpi/cpu.h | 1 + docs/specs/acpi_cpu_hotplug.txt | 19 ++++++++++++++----- hw/acpi/cpu.c | 9 +++++++++ hw/acpi/trace-events | 2 ++ 4 files changed, 26 insertions(+), 5 deletions(-)