Re: [RFC 2/3] acpi: cpuhp: add typical usecases into spec

[Laszlo Ersek]

On 10/09/19 15:22, Igor Mammedov wrote:
> Clarify values of "CPU selector' register and add workflows for

mismatched quotes (double vs. single)

>   * finding CPU with pending 'insert/remove' event
>   * enumerating present/non present CPUs
> 
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
>  docs/specs/acpi_cpu_hotplug.txt | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> index ac5903b2b1..43c5a193f0 100644
> --- a/docs/specs/acpi_cpu_hotplug.txt
> +++ b/docs/specs/acpi_cpu_hotplug.txt
> @@ -54,6 +54,7 @@ write access:
>      [0x0-0x3] CPU selector: (DWORD access)

Please clarify the endianness.

>                selects active CPU device. All following accesses to other
>                registers will read/store data from/to selected CPU.
> +              Valid values: [0 .. max_cpus)

Nice; appreciate the bracket on the left side vs. the paren on the right
side!

>      [0x4] CPU device control fields: (1 byte access)
>          bits:
>              0: reserved, OSPM must clear it before writing to register.
> @@ -93,3 +94,24 @@ Selecting CPU device beyond possible range has no effect 
> on platform:
>       ignored
>     - read accesses to CPU hot-plug registers not documented above return
>       all bits set to 0.
> +
> +Typical usecases:
> +   - Get a cpu with pending event
> +     1. write 0x0 into 'Command field' register
> +     2. read from 'Command data' register, CPU selector value (CPU's UID in 
> ACPI
> +        tables)

OK.

I suggest putting this as: "read the CPU selector value (the CPU's UID
in the ACPI tables) from the 'Command data' register"

> and event for selected CPU from 'CPU device status fields'

OK.

> +        register. If there aren't pending events, CPU selector value doesn't

OK.

I suggest s/aren't/are no/

> +        change

So this feels important: *change* is relative to a previous value. In
order to determine change, I have to

- either read the "command data" register before writing 0x0 to
"command", and then compare the old value against the new value

- or even set "command data" to a bogus value myself (against which I
can compare the new value, after writing the command register).

So, what is the previous selector value that the change is relative to?

> and 'insert' and 'remove' bits are not set.

Ah, so is the order of steps actually this:

1. write 0x0 to command

2. read device status field

3. if bit#1 or bit#2 is set (insert or remove event), read CPU selector
affected by those event(s) from the command data field

4. otherwise, no pending event

?

> +   - Enumerate CPUs present/non present CPUs.
> +     1. set iterator to 0x0

OK

> +     2. write 0x0 into 'Command field' register

... this may update the device status field, and the command data field
(to a selector with pending events)

> and then iterator
> +        into 'CPU selector' register.

... so in case command 0x0 selected a CPU with pending events, we ignore
that, and select our iterator anyway. OK.

> +     3. read 'enabled' flag for selected CPU from 'CPU device status fields'
> +        register

OK

> +     4. to continue to the next CPU, increment iterator

OK

> and repeat step 2

not sure why writing 0x0 to "command" again is useful, but I'll see it
below; OK

> +     5. read 'Command data' register

oookay... so if writing 0x0 to command selected a CPU with pending
events, we get the selector of *that* CPU (regardless of what iterator
we have presently)

Otherwise we get an indeterminate value.

> +     5.1 if 'Command data' register matches iterator continue to step 3.

uhhh... what? :) At this point, the command data register can be in two
states:

- if the last 0x0 command selected a CPU with events pending, then that
selector is available in the command data register.

I don't understand why comparing that against the iterator is helpful.

- If there was no CPU with pending events, we're comparing an
indeterminate value against the iterator. Why?

I think the "command data" field must change under some circumstances
that are currently not documented. (I.e. it seems like "command data"
does not *only* change when command 0x0 can find a CPU with pending events.)

Thanks
Laszlo

> +         (read presence bit for the next CPU)
> +     5.2 if 'Command data' register has not changed, there is not CPU
> +         corresponding to iterator value and the last valid iterator value
> +         equals to 'max_cpus' + 1
>