| Message ID | 1575479147-6641-8-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | q35: CPU hotplug with secure boot, part 1+2 | expand |
On 12/04/19 18:05, Igor Mammedov wrote: > Extend CPU hotplug interface to return architecture specific > identifier for current CPU in 2 registers: > - lower 32 bits existing ACPI_CPU_CMD_DATA_OFFSET_RW > - upper 32 bits in new ACPI_CPU_CMD_DATA2_OFFSET_R at > offset 0. OK. > Target user is UEFI firmware, which needs a way to enumerate > all CPUs (including possible CPUs) to allocate and initialize > CPU structures on boot. (1) This is correct in general, but if we want to keep this description, then it should be moved to the commit message of the previous patch. CPHP_GET_CPU_ID_CMD is not needed for the purpose described above -- it will be necessary for handling the hotplug SMI. For the boot time allocation / initialization, the "enumerating present and possible CPUs" workflow is necessary, and that is documented in the previous patch in this series. So if we want to keep this paragraph, we should move it to the previous patch's commit message. > (for x86: it needs APIC ID and later command will be used to > retrieve ARM's MPIDR which serves the similar to APIC ID purpose) (2) I would suggest some punctuation, to make this clearer. How about: > On x86, guest UEFI firmware will use CPHP_GET_CPU_ID_CMD for fetching > the APIC ID when handling the hotplug SMI. > > Later, CPHP_GET_CPU_ID_CMD will be used on ARM to retrieve MPIDR, > which serves a purpose similar to the x86 APIC ID. Back to the patch: On 12/04/19 18:05, Igor Mammedov wrote: > > The new ACPI_CPU_CMD_DATA2_OFFSET_R register will also be used > to detect presence of modern CPU hotplug, which will be described > in follow up patch. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > v1: > - s/ACPI_CPU_CMD_DATA2_OFFSET_RW/ACPI_CPU_CMD_DATA2_OFFSET_R/. > --- > docs/specs/acpi_cpu_hotplug.txt | 10 ++++++++-- > hw/acpi/cpu.c | 15 +++++++++++++++ > hw/acpi/trace-events | 1 + > 3 files changed, 24 insertions(+), 2 deletions(-) > > diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt > index 58c16c6..bb33144 100644 > --- a/docs/specs/acpi_cpu_hotplug.txt > +++ b/docs/specs/acpi_cpu_hotplug.txt > @@ -44,7 +44,11 @@ keeps the current value. > > read access: > offset: > - [0x0-0x3] reserved > + [0x0-0x3] Command data 2: (DWORD access) > + if last stored 'Command field' value: > + 3: upper 32 bits of architecture specific identifying CPU value > + (n x86 case: 0x0) > + other values: reserved > [0x4] CPU device status fields: (1 byte access) > bits: > 0: Device is enabled and may be used by guest OK. > @@ -96,7 +100,9 @@ write access: > 2: stores value into OST status register, triggers > ACPI_DEVICE_OST QMP event from QEMU to external applications > with current values of OST event and status registers. > - other values: reserved > + 3: lower 32 bit of architecture specific identifier > + (in x86 case: APIC ID) > + other values: reserved > > Typical usecases: > - Get a cpu with pending event (3) This fragment has been pasted in the wrong spot. We should add this under: > read access: > ... > [0x8] Command data: (DWORD access) > contains 0 unless last stored in 'Command field' value is one of: > 0: contains 'CPU selector' value of a CPU with pending event[s] But right now, the addition is in the context of: > write access: > ... > [0x8] Command data: (DWORD access) > if last stored 'Command field' value: > 1: stores value into OST event register > 2: stores value into OST status register, triggers > ACPI_DEVICE_OST QMP event from QEMU to external applications > with current values of OST event and status registers. > 3: lower 32 bit of architecture specific identifier > (in x86 case: APIC ID) > other values: reserved and it does not make sense there. (4) Furthermore, for consistency, please write "32 bits" (plural). Back to the patch: On 12/04/19 18:05, Igor Mammedov wrote: > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c > index 87f30a3..87813ce 100644 > --- a/hw/acpi/cpu.c > +++ b/hw/acpi/cpu.c > @@ -12,11 +12,13 @@ > #define ACPI_CPU_FLAGS_OFFSET_RW 4 > #define ACPI_CPU_CMD_OFFSET_WR 5 > #define ACPI_CPU_CMD_DATA_OFFSET_RW 8 > +#define ACPI_CPU_CMD_DATA2_OFFSET_R 0 > > enum { > CPHP_GET_NEXT_CPU_WITH_EVENT_CMD = 0, > CPHP_OST_EVENT_CMD = 1, > CPHP_OST_STATUS_CMD = 2, > + CPHP_GET_CPU_ID_CMD = 3, > CPHP_CMD_MAX > }; > > @@ -74,11 +76,24 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size) > case CPHP_GET_NEXT_CPU_WITH_EVENT_CMD: > val = cpu_st->selector; > break; > + case CPHP_GET_CPU_ID_CMD: > + val = cdev->arch_id & 0xFFFFFFFF; > + break; > default: > break; > } > trace_cpuhp_acpi_read_cmd_data(cpu_st->selector, val); > break; > + case ACPI_CPU_CMD_DATA2_OFFSET_R: > + switch (cpu_st->command) { > + case CPHP_GET_CPU_ID_CMD: > + val = cdev->arch_id >> 32; > + break; > + default: > + break; > + } > + trace_cpuhp_acpi_read_cmd_data2(cpu_st->selector, val); > + break; > default: > break; > } > diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events > index 96b8273..afbc77d 100644 > --- a/hw/acpi/trace-events > +++ b/hw/acpi/trace-events > @@ -23,6 +23,7 @@ cpuhp_acpi_read_flags(uint32_t idx, uint8_t flags) "idx[0x%"PRIx32"] flags: 0x%" > cpuhp_acpi_write_idx(uint32_t idx) "set active cpu idx: 0x%"PRIx32 > cpuhp_acpi_write_cmd(uint32_t idx, uint8_t cmd) "idx[0x%"PRIx32"] cmd: 0x%"PRIx8 > cpuhp_acpi_read_cmd_data(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32 > +cpuhp_acpi_read_cmd_data2(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32 > cpuhp_acpi_cpu_has_events(uint32_t idx, bool ins, bool rm) "idx[0x%"PRIx32"] inserting: %d, removing: %d" > cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]" > cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]" > The code looks good to me. Thanks! Laszlo
On Thu, 5 Dec 2019 14:03:01 +0100 Laszlo Ersek <lersek@redhat.com> wrote: > On 12/04/19 18:05, Igor Mammedov wrote: > > Extend CPU hotplug interface to return architecture specific > > identifier for current CPU in 2 registers: > > - lower 32 bits existing ACPI_CPU_CMD_DATA_OFFSET_RW > > - upper 32 bits in new ACPI_CPU_CMD_DATA2_OFFSET_R at > > offset 0. > > OK. > > > Target user is UEFI firmware, which needs a way to enumerate > > all CPUs (including possible CPUs) to allocate and initialize > > CPU structures on boot. > > (1) This is correct in general, but if we want to keep this description, > then it should be moved to the commit message of the previous patch. > CPHP_GET_CPU_ID_CMD is not needed for the purpose described above -- it > will be necessary for handling the hotplug SMI. > > For the boot time allocation / initialization, the "enumerating present > and possible CPUs" workflow is necessary, and that is documented in the > previous patch in this series. > > So if we want to keep this paragraph, we should move it to the previous > patch's commit message. > > > (for x86: it needs APIC ID and later command will be used to > > retrieve ARM's MPIDR which serves the similar to APIC ID purpose) > > (2) I would suggest some punctuation, to make this clearer. How about: > > > On x86, guest UEFI firmware will use CPHP_GET_CPU_ID_CMD for fetching > > the APIC ID when handling the hotplug SMI. > > > > Later, CPHP_GET_CPU_ID_CMD will be used on ARM to retrieve MPIDR, > > which serves a purpose similar to the x86 APIC ID. How about following commit message: Firmware can enumerate present at boot APs by broadcasting wakeup IPI, so that woken up secondary CPUs could register them-selves. However in CPU hotplug case, it would need to know architecture specific CPU IDs for possible and hotplugged CPUs so it could prepare enviroment for and wake hotplugged AP. Reuse and extend existing CPU hotplug interface to return architecture specific ID for currently selected CPU in 2 registers: - lower 32 bits in ACPI_CPU_CMD_DATA_OFFSET_RW - upper 32 bits in ACPI_CPU_CMD_DATA2_OFFSET_R On x86, firmware will use CPHP_GET_CPU_ID_CMD for fetching the APIC ID when handling hotplug SMI. Later, CPHP_GET_CPU_ID_CMD will be used on ARM to retrieve MPIDR, which serves the similar to APIC ID purpose. [...]
On 12/06/19 16:15, Igor Mammedov wrote: > On Thu, 5 Dec 2019 14:03:01 +0100 > Laszlo Ersek <lersek@redhat.com> wrote: > >> On 12/04/19 18:05, Igor Mammedov wrote: >>> Extend CPU hotplug interface to return architecture specific >>> identifier for current CPU in 2 registers: >>> - lower 32 bits existing ACPI_CPU_CMD_DATA_OFFSET_RW >>> - upper 32 bits in new ACPI_CPU_CMD_DATA2_OFFSET_R at >>> offset 0. >> >> OK. >> >>> Target user is UEFI firmware, which needs a way to enumerate >>> all CPUs (including possible CPUs) to allocate and initialize >>> CPU structures on boot. >> >> (1) This is correct in general, but if we want to keep this description, >> then it should be moved to the commit message of the previous patch. >> CPHP_GET_CPU_ID_CMD is not needed for the purpose described above -- it >> will be necessary for handling the hotplug SMI. >> >> For the boot time allocation / initialization, the "enumerating present >> and possible CPUs" workflow is necessary, and that is documented in the >> previous patch in this series. >> >> So if we want to keep this paragraph, we should move it to the previous >> patch's commit message. >> >>> (for x86: it needs APIC ID and later command will be used to >>> retrieve ARM's MPIDR which serves the similar to APIC ID purpose) >> >> (2) I would suggest some punctuation, to make this clearer. How about: >> >>> On x86, guest UEFI firmware will use CPHP_GET_CPU_ID_CMD for fetching >>> the APIC ID when handling the hotplug SMI. >>> >>> Later, CPHP_GET_CPU_ID_CMD will be used on ARM to retrieve MPIDR, >>> which serves a purpose similar to the x86 APIC ID. > > How about following commit message: > > Firmware can enumerate present at boot APs by broadcasting wakeup IPI, > so that woken up secondary CPUs could register them-selves. > However in CPU hotplug case, it would need to know architecture > specific CPU IDs for possible and hotplugged CPUs so it could > prepare enviroment for and wake hotplugged AP. > > Reuse and extend existing CPU hotplug interface to return architecture > specific ID for currently selected CPU in 2 registers: > - lower 32 bits in ACPI_CPU_CMD_DATA_OFFSET_RW > - upper 32 bits in ACPI_CPU_CMD_DATA2_OFFSET_R > > On x86, firmware will use CPHP_GET_CPU_ID_CMD for fetching the APIC ID > when handling hotplug SMI. > > Later, CPHP_GET_CPU_ID_CMD will be used on ARM to retrieve MPIDR, > which serves the similar to APIC ID purpose. > > [...] > Looks fine to me, thank you! Laszlo
diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt index 58c16c6..bb33144 100644 --- a/docs/specs/acpi_cpu_hotplug.txt +++ b/docs/specs/acpi_cpu_hotplug.txt @@ -44,7 +44,11 @@ keeps the current value. read access: offset: - [0x0-0x3] reserved + [0x0-0x3] Command data 2: (DWORD access) + if last stored 'Command field' value: + 3: upper 32 bits of architecture specific identifying CPU value + (n x86 case: 0x0) + other values: reserved [0x4] CPU device status fields: (1 byte access) bits: 0: Device is enabled and may be used by guest @@ -96,7 +100,9 @@ write access: 2: stores value into OST status register, triggers ACPI_DEVICE_OST QMP event from QEMU to external applications with current values of OST event and status registers. - other values: reserved + 3: lower 32 bit of architecture specific identifier + (in x86 case: APIC ID) + other values: reserved Typical usecases: - Get a cpu with pending event diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index 87f30a3..87813ce 100644 --- a/hw/acpi/cpu.c +++ b/hw/acpi/cpu.c @@ -12,11 +12,13 @@ #define ACPI_CPU_FLAGS_OFFSET_RW 4 #define ACPI_CPU_CMD_OFFSET_WR 5 #define ACPI_CPU_CMD_DATA_OFFSET_RW 8 +#define ACPI_CPU_CMD_DATA2_OFFSET_R 0 enum { CPHP_GET_NEXT_CPU_WITH_EVENT_CMD = 0, CPHP_OST_EVENT_CMD = 1, CPHP_OST_STATUS_CMD = 2, + CPHP_GET_CPU_ID_CMD = 3, CPHP_CMD_MAX }; @@ -74,11 +76,24 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size) case CPHP_GET_NEXT_CPU_WITH_EVENT_CMD: val = cpu_st->selector; break; + case CPHP_GET_CPU_ID_CMD: + val = cdev->arch_id & 0xFFFFFFFF; + break; default: break; } trace_cpuhp_acpi_read_cmd_data(cpu_st->selector, val); break; + case ACPI_CPU_CMD_DATA2_OFFSET_R: + switch (cpu_st->command) { + case CPHP_GET_CPU_ID_CMD: + val = cdev->arch_id >> 32; + break; + default: + break; + } + trace_cpuhp_acpi_read_cmd_data2(cpu_st->selector, val); + break; default: break; } diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events index 96b8273..afbc77d 100644 --- a/hw/acpi/trace-events +++ b/hw/acpi/trace-events @@ -23,6 +23,7 @@ cpuhp_acpi_read_flags(uint32_t idx, uint8_t flags) "idx[0x%"PRIx32"] flags: 0x%" cpuhp_acpi_write_idx(uint32_t idx) "set active cpu idx: 0x%"PRIx32 cpuhp_acpi_write_cmd(uint32_t idx, uint8_t cmd) "idx[0x%"PRIx32"] cmd: 0x%"PRIx8 cpuhp_acpi_read_cmd_data(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32 +cpuhp_acpi_read_cmd_data2(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32 cpuhp_acpi_cpu_has_events(uint32_t idx, bool ins, bool rm) "idx[0x%"PRIx32"] inserting: %d, removing: %d" cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]" cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]"
Extend CPU hotplug interface to return architecture specific identifier for current CPU in 2 registers: - lower 32 bits existing ACPI_CPU_CMD_DATA_OFFSET_RW - upper 32 bits in new ACPI_CPU_CMD_DATA2_OFFSET_R at offset 0. Target user is UEFI firmware, which needs a way to enumerate all CPUs (including possible CPUs) to allocate and initialize CPU structures on boot. (for x86: it needs APIC ID and later command will be used to retrieve ARM's MPIDR which serves the similar to APIC ID purpose) The new ACPI_CPU_CMD_DATA2_OFFSET_R register will also be used to detect presence of modern CPU hotplug, which will be described in follow up patch. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- v1: - s/ACPI_CPU_CMD_DATA2_OFFSET_RW/ACPI_CPU_CMD_DATA2_OFFSET_R/. --- docs/specs/acpi_cpu_hotplug.txt | 10 ++++++++-- hw/acpi/cpu.c | 15 +++++++++++++++ hw/acpi/trace-events | 1 + 3 files changed, 24 insertions(+), 2 deletions(-)