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 <imamm...@redhat.com> >> --- >> 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 >> >
