Hi Igor,
On 01/24/20 16:17, Igor Mammedov wrote:
> Commit 3a61c8db9d25 introduced CPHP_GET_CPU_ID_CMD command but
> did not sufficiently described how to use it. Fix it by adding
> missing command documentation and suggested work-flow to enumerate
> possible architecture specific CPU IDs.
>
> Fixes: 3a61c8db9d25 ("acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command")
> Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> ---
> docs/specs/acpi_cpu_hotplug.txt | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> index a8ce5e7..81b4534 100644
> --- a/docs/specs/acpi_cpu_hotplug.txt
> +++ b/docs/specs/acpi_cpu_hotplug.txt
> @@ -94,6 +94,8 @@ write access:
> register in QEMU
> 2: following writes to 'Command data' register set OST status
> register in QEMU
> + 3: following reads from 'Command data' and 'Command data 2'
> return
> + architecture specific CPU ID value for currently selected CPU.
> other values: reserved
> [0x6-0x7] reserved
> [0x8] Command data: (DWORD access)
Looks good.
> @@ -147,3 +149,16 @@ Typical usecases:
> 11. Otherwise store 0x0 to the 'CPU selector' register, to put it
> into a valid state and exit.
> The iterator at this point equals "max_cpus".
> +
> + - Enumerate present/non present CPUs architecture specific IDs
> + (in case of x86: ACPIC IDs)
> + 01: Use "Enumerate CPUs present/non present CPUs" to get max_cpus
OK, this includes the last step of that procedure too, i.e.:
11. Otherwise store 0x0 to the 'CPU selector' register, to put it
into a valid state and exit.
[...]
> + 02: Store 0x3 in the 'Command field' register
> + 03: Set 'current cpu selector' iterator to 0x0
> + 04: Store the iterator to the 'CPU selector' register
> + 05: Read from registers 'Command data' and 'Command data 2' parts of
> ID,
> + combine them into ID like following:
> + 'Command data 2' << 32 | 'Command data'
> + and store pair 'current cpu selector' : ID for further processing
> + 06: Increment the iterator and if the iterator < max_cpus go to step 4
> + 07: Otherwise store 0x0 to the 'CPU selector' register and exit.
>
This looks good as well.
I'm happy to R-b this patch, with one caveat: in edk2, I might not want
to -- or have to -- fetch the full array of arch-specific CPU IDs in
advance. That's one possibility, yes -- but it's also possible that I'll
only fetch the arch CPU ID for the freshly hotplugged CPU in the SMI
handler. I don't know yet.
So, as long as OVMF is not expected to *only* implement the typical use
case here, I'm OK with this algorithm, because it looks valid to me.
I'd just like to keep the option open to use the CPHP_GET_CPU_ID_CMD
command for such a CPU as well that has been selected with -- say --
CPHP_GET_NEXT_CPU_WITH_EVENT_CMD.
With that:
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
(If you like, you could also split this patch in two -- repost the first
half (with the register documentation update) with my R-b at once, and
delay the second half (the typical use case for CPHP_GET_CPU_ID_CMD)
until I "get there" in edk2. Up to you.)
Thanks!
Laszlo
