Re: [PATCH for-5.0 8/8] acpi: cpuhp: spec: document procedure for enabling modern CPU hotplug

[Laszlo Ersek <lersek@redhat.com>]

On 12/04/19 18:05, Igor Mammedov wrote:
> Describe how to enable and detect modern CPU hotplug interface.
> Detection part is based on new CPHP_GET_CPU_ID_CMD command,
> introduced by "acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command" patch.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  docs/specs/acpi_cpu_hotplug.txt | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)

Could we make this usecase / workflow independent of the new
CPHP_GET_CPU_ID_CMD command please?

I'd like to suggest the following:

> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> index bb33144..667b264 100644
> --- a/docs/specs/acpi_cpu_hotplug.txt
> +++ b/docs/specs/acpi_cpu_hotplug.txt
> @@ -15,14 +15,14 @@ CPU present bitmap for:
>    PIIX-PM  (IO port 0xaf00-0xaf1f, 1-byte access)
>    One bit per CPU. Bit position reflects corresponding CPU APIC ID. Read-only.
>    The first DWORD in bitmap is used in write mode to switch from legacy
> -  to new CPU hotplug interface, write 0 into it to do switch.
> +  to modern CPU hotplug interface, write 0 into it to do switch.
>  ---------------------------------------------------------------
>  QEMU sets corresponding CPU bit on hot-add event and issues SCI
>  with GPE.2 event set. CPU present map is read by ACPI BIOS GPE.2 handler
>  to notify OS about CPU hot-add events. CPU hot-remove isn't supported.
>
>  =====================================
> -ACPI CPU hotplug interface registers:
> +Modern ACPI CPU hotplug interface registers:
>  -------------------------------------
>  Register block base address:
>      ICH9-LPC IO port 0x0cd8
> @@ -105,6 +105,24 @@ write access:
>                other values: reserved
>
>      Typical usecases:
> +        - (x86) Detecting and enabling modern CPU hotplug interface.

(1) I think we can drop the (x86) restriction. (Because, we don't need
to depend on APIC ID specifics; see below.)

> +          QEMU starts with legacy CPU hotplug interface enabled. Detecting and
> +          switching to modern interface is based on the 2 legacy CPU hotplug features:
> +            1. Writes into CPU bitmap are ignored.
> +            2. CPU bitmap always has bit#0 set, corresponding to boot CPU.
> +
> +          Use following steps to detect and enable modern CPU hotplug interface:
> +            1. Store 0x0 to the 'CPU selector' register,
> +               attempting to switch to modern mode
> +            2. Store 0x0 to the 'CPU selector' register,
> +               to ensure valid selector value

OK thus far.

> +            3. Store 0x3 to the 'Command field' register,
> +               sets the 'Command data 2' register into architecture specific
> +               CPU identifier mode

(2) Can we please store command 0 here
(CPHP_GET_NEXT_CPU_WITH_EVENT_CMD)?

That might change the selector value, yes. (Even if that happens, the
new selector will be *valid*.)

But the main point is that, with 0 stored to the command register, one
of the following *four* cases will hold, subsequently:

(2a) if register block is totally missing:

--> Offset#0 will read as all-bits-one (unassigned read)

(2b) if register block is legacy-only:

--> Offset#0 will read as nonzero, due to CPU#0 being always present

(2c) if the modern register block is active, but the "Command data 2"
register is *not* yet described in the spec file:

--> Offset#0 will read as zero, because it is *reserved*:

> read access:
>     offset:
>     [0x0-0x3] reserved <---- HERE

(2d) if the modern register block is active, and the "Command data 2"
register *is* described in the spec file:

--> the "Command data 2" register (at offset#0) will read as zero,
because:

> read access:
>     offset:
>     [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 <------ HERE

and then step#4 applies just the same:

On 12/04/19 18:05, Igor Mammedov wrote:
> +            4. Read the 'Command data 2' register.
> +               If read value is 0x0, the modern interface is enabled.
> +               Otherwise legacy or no CPU hotplug interface available
> +

because "read value is 0x0" corresponds to the *union* of cases (2c) and
(2d) -- namely, "the modern register block is active".

My proposal above is what I implemented for OVMF in October:

  [edk2-devel] [PATCH v2 3/3]
  OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug

  http://mid.mail-archive.com/20191022221554.14963-4-lersek@redhat.com

and it works very well.

So the benefits would be:

- I wouldn't have to rewrite that (complex!) patch :) ,

- the logic would not store the new CPHP_GET_CPU_ID_CMD command, only
  read offset#0,

- the logic would not be x86 specific (it would not have to rely on the
  most significant 32 bits of the 64-bit arch-specific CPU identifier --
  here: the APIC ID -- being zero).

Furthermore,

(3) In step#4, I suggest replacing 'Command data 2' with "offset 0",

(4) finally, I'd suggest squashing this patch (updated as requested
above) into patch "acpi: cpuhp: spec: add typical usecases".

Using my suggestion (2), you can define the "modern interface
enablement" workflow as well, without any dependency on
CPHP_GET_CPU_ID_CMD. The only thing that's necessary is the small update
from (3), and then you can describe all three important use cases in one
go, in patch#6.

And then you can introduce CPHP_GET_CPU_ID_CMD in the last patch
(patch#7), while staying compatible with the previously-documented
workflows.

Pretty please? :)

Thanks!
Laszlo

>          - Get a cpu with pending event
>            1. Store 0x0 to the 'CPU selector' register.
>            2. Store 0x0 to the 'Command field' register.
>