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

[Laszlo Ersek]

On 10/10/19 15:04, Laszlo Ersek wrote:
> 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.)

After looking at cpu_hotplug_rd(), I think I know what's going on.

Every time command 0 is written, and there is no CPU with pending
events, the command data register will read as 0!

This seems like a core piece of information, and it's not documented in
the text file anywhere. It only says (with patch#1 applied),

  in case of error or unsupported command reads is 0x0

Command 0 is *not* unsupported. Therefore, this documentation is only
self-consistent if:

- selecting a non-existent (>=max_cpus) CPU via the selector register is
an *error*

- asking for a CPU with pending events (with command 0x0), and finding
none, is also an *error*.

Let me re-read the patch set with this information in mind.

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
>>
>