| Message ID | 20191009132252.17860-3-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface | expand |
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 <imammedo@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.) 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 >
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 <imammedo@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 >> >
On 10/09/19 15:22, Igor Mammedov wrote: > Clarify values of "CPU selector' register and add workflows for > * finding CPU with pending 'insert/remove' event > * enumerating present/non present CPUs > > Signed-off-by: Igor Mammedov <imammedo@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) > selects active CPU device. All following accesses to other > registers will read/store data from/to selected CPU. > + Valid values: [0 .. max_cpus) > [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) and event for selected CPU from 'CPU device status fields' > + register. If there aren't pending events, CPU selector value doesn't > + change and 'insert' and 'remove' bits are not set. Okay, so based on the "Command data" documentation I'm suggesting in <http://mid.mail-archive.com/cd0713b5-fd64-d3e1-7f83-3a0725b819a3@redhat.com>, I propose: 1. Store 0x0 to the 'CPU selector' register. 2. Store 0x0 to the 'Command field' register. 3. Read the 'CPU device status fields' register. 4. If both bit#1 and bit#2 are clear in the value read, there is no CPU with a pending event. 5. Otherwise, read the 'Command data' register. The value read is the selector of the CPU with the pending event (which is already selected). > + - Enumerate CPUs present/non present CPUs. > + 1. set iterator to 0x0 > + 2. write 0x0 into 'Command field' register and then iterator > + into 'CPU selector' register. > + 3. read 'enabled' flag for selected CPU from 'CPU device status fields' > + register > + 4. to continue to the next CPU, increment iterator and repeat step 2 > + 5. read 'Command data' register > + 5.1 if 'Command data' register matches iterator continue to step 3. > + (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 > How about: 01. Set the present CPU count to 0. 02. Set the iterator to 0. 03. Store 0x0 to the 'Command field' register. 04. Store 0x0 to the 'CPU selector' register. 05. Read the 'CPU device status fields' register. 06. If bit#0 is set, increment the present CPU count. 07. Increment the iterator. 08. Store the iterator to the 'CPU selector' register. 09. Read the 'Command data' register. 10. If the value read is zero, then the iterator equals "max_cpus"; exit now. 11. Goto 05. Thanks Laszlo
On Thu, 10 Oct 2019 16:13:22 +0200 Laszlo Ersek <lersek@redhat.com> wrote: > On 10/09/19 15:22, Igor Mammedov wrote: > > Clarify values of "CPU selector' register and add workflows for > > * finding CPU with pending 'insert/remove' event > > * enumerating present/non present CPUs > > > > Signed-off-by: Igor Mammedov <imammedo@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) > > selects active CPU device. All following accesses to other > > registers will read/store data from/to selected CPU. > > + Valid values: [0 .. max_cpus) > > [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) and event for selected CPU from 'CPU device status fields' > > + register. If there aren't pending events, CPU selector value doesn't > > + change and 'insert' and 'remove' bits are not set. > > Okay, so based on the "Command data" documentation I'm suggesting in > <http://mid.mail-archive.com/cd0713b5-fd64-d3e1-7f83-3a0725b819a3@redhat.com>, > I propose: > > 1. Store 0x0 to the 'CPU selector' register. With ACPI code being the only user it was not necessary as value val always correct. But if someone wrote invalid selector value it would break CPHP_GET_NEXT_CPU_WITH_EVENT_CMD command. I shall update AML code to include this step as well so it would be more robust. > 2. Store 0x0 to the 'Command field' register. > 3. Read the 'CPU device status fields' register. > 4. If both bit#1 and bit#2 are clear in the value read, there is no CPU > with a pending event. > 5. Otherwise, read the 'Command data' register. The value read is the > selector of the CPU with the pending event (which is already > selected). > > > + - Enumerate CPUs present/non present CPUs. > > + 1. set iterator to 0x0 > > + 2. write 0x0 into 'Command field' register and then iterator > > + into 'CPU selector' register. > > + 3. read 'enabled' flag for selected CPU from 'CPU device status fields' > > + register > > + 4. to continue to the next CPU, increment iterator and repeat step 2 > > + 5. read 'Command data' register > > + 5.1 if 'Command data' register matches iterator continue to step 3. > > + (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 > > > > How about: > > 01. Set the present CPU count to 0. > 02. Set the iterator to 0. > 03. Store 0x0 to the 'Command field' register. > 04. Store 0x0 to the 'CPU selector' register. > 05. Read the 'CPU device status fields' register. > 06. If bit#0 is set, increment the present CPU count. > 07. Increment the iterator. > 08. Store the iterator to the 'CPU selector' register. > 09. Read the 'Command data' register. > 10. If the value read is zero, then the iterator equals "max_cpus"; > exit now. > 11. Goto 05. Looks good, I'll use both suggestions, thanks! > > Thanks > Laszlo
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) selects active CPU device. All following accesses to other registers will read/store data from/to selected CPU. + Valid values: [0 .. max_cpus) [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) and event for selected CPU from 'CPU device status fields' + register. If there aren't pending events, CPU selector value doesn't + change and 'insert' and 'remove' bits are not set. + - Enumerate CPUs present/non present CPUs. + 1. set iterator to 0x0 + 2. write 0x0 into 'Command field' register and then iterator + into 'CPU selector' register. + 3. read 'enabled' flag for selected CPU from 'CPU device status fields' + register + 4. to continue to the next CPU, increment iterator and repeat step 2 + 5. read 'Command data' register + 5.1 if 'Command data' register matches iterator continue to step 3. + (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
Clarify values of "CPU selector' register and add workflows for * finding CPU with pending 'insert/remove' event * enumerating present/non present CPUs Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- docs/specs/acpi_cpu_hotplug.txt | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)