Re: [PATCH] acpi: cpuhp: document how to use CPHP_GET_CPU_ID_CMD command

[Laszlo Ersek]

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 <address@hidden>
> ---
>  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 <address@hidden>

(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