Re: [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec
(I've been dropped from the address list, not sure why) On 10/22/19 19:17, Christophe de Dinechin wrote: > > Laszlo Ersek writes: > >> On 10/10/19 15:31, Laszlo Ersek wrote: >>> 2nd round: >>> >>> On 10/09/19 15:22, Igor Mammedov wrote: QEMU returns 0, in case of erro or invalid value in 'Command field', make spec match reality, i.e. >>> >>> AHA! so this is exactly where you meant to list the particular cases >>> when "command data" reads as 0: >>> - CPU >= max_cpus selected, >>> - CPU with pending events asked for, but none found >>> Also fix returned value description in case 'Command field' == 0x0, it's in not PXM but CPU selector value with pending event Signed-off-by: Igor Mammedov --- docs/specs/acpi_cpu_hotplug.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt index ee219c8358..ac5903b2b1 100644 --- a/docs/specs/acpi_cpu_hotplug.txt +++ b/docs/specs/acpi_cpu_hotplug.txt @@ -44,9 +44,10 @@ read access: 3-7: reserved and should be ignored by OSPM [0x5-0x7] reserved [0x8] Command data: (DWORD access) - in case of error or unsupported command reads is 0x + in case of error or unsupported command reads is 0x0 current 'Command field' value: - 0: returns PXM value corresponding to device + 0: returns CPU selector value corresponding to a CPU with + pending event. write access: offset: >>> >>> How about: >>> >>> [0x8] Command data: (DWORD access, little endian) >>> If the "CPU selector" value last stored by the guest refers to >>> an impossible CPU, then 0. >>> Otherwise, if the "Command field" value last stored by the >>> guest differs from 0, then 0. >>> Otherwise, if there is at least one CPU with a pending event, >>> then that CPU has been selected; the command data register >>> returns that selector. >>> Otherwise, 0. >> >> Hmmm not exactly. Let me try again. >> >> [0x8] Command data: (DWORD access, little endian) >> If the "CPU selector" value last stored by the guest refers to >> an impossible CPU, then 0. >> Otherwise, if the "Command field" value last stored by the >> guest differs from 0, then 0. >> Otherwise, the currently selected CPU. > > How about phrasing it to put the more general case first, e.g. > > [0x8] Command data: (DWORD access, little endian) > > The currently selected CPU, unless: > - The "CPU selector" value refers to an impossible CPU > - The "Command field" value last stored by the guest differs > from 0 > In these last two cases, the value is 0. I prefer my proposal because it translates to source code more directly (a ladder of "if / else" pairs, or similar). (I know that some programming languages support "unless"; I could never wrap my brain around that idea! :) ) Thanks Laszlo
Re: [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec
Laszlo Ersek writes: > On 10/10/19 15:31, Laszlo Ersek wrote: >> 2nd round: >> >> On 10/09/19 15:22, Igor Mammedov wrote: >>> QEMU returns 0, in case of erro or invalid value in 'Command field', >>> make spec match reality, i.e. >> >> AHA! so this is exactly where you meant to list the particular cases >> when "command data" reads as 0: >> - CPU >= max_cpus selected, >> - CPU with pending events asked for, but none found >> >>> Also fix returned value description in case 'Command field' == 0x0, >>> it's in not PXM but CPU selector value with pending event >>> >>> Signed-off-by: Igor Mammedov >>> --- >>> docs/specs/acpi_cpu_hotplug.txt | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/docs/specs/acpi_cpu_hotplug.txt >>> b/docs/specs/acpi_cpu_hotplug.txt >>> index ee219c8358..ac5903b2b1 100644 >>> --- a/docs/specs/acpi_cpu_hotplug.txt >>> +++ b/docs/specs/acpi_cpu_hotplug.txt >>> @@ -44,9 +44,10 @@ read access: >>> 3-7: reserved and should be ignored by OSPM >>> [0x5-0x7] reserved >>> [0x8] Command data: (DWORD access) >>> - in case of error or unsupported command reads is 0x >>> + in case of error or unsupported command reads is 0x0 >>>current 'Command field' value: >>> - 0: returns PXM value corresponding to device >>> + 0: returns CPU selector value corresponding to a CPU with >>> + pending event. >>> >>> write access: >>> offset: >>> >> >> How about: >> >> [0x8] Command data: (DWORD access, little endian) >> If the "CPU selector" value last stored by the guest refers to >> an impossible CPU, then 0. >> Otherwise, if the "Command field" value last stored by the >> guest differs from 0, then 0. >> Otherwise, if there is at least one CPU with a pending event, >> then that CPU has been selected; the command data register >> returns that selector. >> Otherwise, 0. > > Hmmm not exactly. Let me try again. > > [0x8] Command data: (DWORD access, little endian) > If the "CPU selector" value last stored by the guest refers to > an impossible CPU, then 0. > Otherwise, if the "Command field" value last stored by the > guest differs from 0, then 0. > Otherwise, the currently selected CPU. How about phrasing it to put the more general case first, e.g. [0x8] Command data: (DWORD access, little endian) The currently selected CPU, unless: - The "CPU selector" value refers to an impossible CPU - The "Command field" value last stored by the guest differs from 0 In these last two cases, the value is 0. > > Thanks, > Laszlo -- Cheers, Christophe de Dinechin (IRC c3d)
Re: [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec
On 10/17/19 17:41, Igor Mammedov wrote: > On Thu, 10 Oct 2019 14:33:19 +0200 > Laszlo Ersek wrote: > >> On 10/09/19 15:22, Igor Mammedov wrote: >>> QEMU returns 0, in case of erro or invalid value in 'Command field', >>> make spec match reality, i.e. >> >> Unfinished thought? > yep, I'll fix it up. > > [...] >>> index ee219c8358..ac5903b2b1 100644 >>> --- a/docs/specs/acpi_cpu_hotplug.txt >>> +++ b/docs/specs/acpi_cpu_hotplug.txt >>> @@ -44,9 +44,10 @@ read access: >>> 3-7: reserved and should be ignored by OSPM >>> [0x5-0x7] reserved >>> [0x8] Command data: (DWORD access) >> >> since we're fixing this dword field description, can you spell out the >> endianness too? > Since it's ACPI oriented interface (i.e. guest AML LE access implied), > I'd prefer to spell it out once in spec so it would cover all integer > fields vs doing it per filed. (less chance to make mistake later) Makes sense, thanks! Laszlo >> (I think endianness is relevant here, because patch#2 suggests selectors >> can be looped over. So selectors are actual integers, not just 32-bit >> opaque blobs.) >> >>> - in case of error or unsupported command reads is 0x >>> + in case of error or unsupported command reads is 0x0 >>>current 'Command field' value: >>> - 0: returns PXM value corresponding to device >>> + 0: returns CPU selector value corresponding to a CPU with >>> + pending event. >>> >>> write access: >>> offset: >>> >> >> Thanks! >> Laszlo >> >
Re: [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec
On Thu, 10 Oct 2019 14:33:19 +0200 Laszlo Ersek wrote: > On 10/09/19 15:22, Igor Mammedov wrote: > > QEMU returns 0, in case of erro or invalid value in 'Command field', > > make spec match reality, i.e. > > Unfinished thought? yep, I'll fix it up. [...] > > index ee219c8358..ac5903b2b1 100644 > > --- a/docs/specs/acpi_cpu_hotplug.txt > > +++ b/docs/specs/acpi_cpu_hotplug.txt > > @@ -44,9 +44,10 @@ read access: > > 3-7: reserved and should be ignored by OSPM > > [0x5-0x7] reserved > > [0x8] Command data: (DWORD access) > > since we're fixing this dword field description, can you spell out the > endianness too? Since it's ACPI oriented interface (i.e. guest AML LE access implied), I'd prefer to spell it out once in spec so it would cover all integer fields vs doing it per filed. (less chance to make mistake later) > (I think endianness is relevant here, because patch#2 suggests selectors > can be looped over. So selectors are actual integers, not just 32-bit > opaque blobs.) > > > - in case of error or unsupported command reads is 0x > > + in case of error or unsupported command reads is 0x0 > >current 'Command field' value: > > - 0: returns PXM value corresponding to device > > + 0: returns CPU selector value corresponding to a CPU with > > + pending event. > > > > write access: > > offset: > > > > Thanks! > Laszlo >
Re: [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec
2nd round: On 10/09/19 15:22, Igor Mammedov wrote: > QEMU returns 0, in case of erro or invalid value in 'Command field', > make spec match reality, i.e. AHA! so this is exactly where you meant to list the particular cases when "command data" reads as 0: - CPU >= max_cpus selected, - CPU with pending events asked for, but none found > Also fix returned value description in case 'Command field' == 0x0, > it's in not PXM but CPU selector value with pending event > > Signed-off-by: Igor Mammedov > --- > docs/specs/acpi_cpu_hotplug.txt | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt > index ee219c8358..ac5903b2b1 100644 > --- a/docs/specs/acpi_cpu_hotplug.txt > +++ b/docs/specs/acpi_cpu_hotplug.txt > @@ -44,9 +44,10 @@ read access: > 3-7: reserved and should be ignored by OSPM > [0x5-0x7] reserved > [0x8] Command data: (DWORD access) > - in case of error or unsupported command reads is 0x > + in case of error or unsupported command reads is 0x0 >current 'Command field' value: > - 0: returns PXM value corresponding to device > + 0: returns CPU selector value corresponding to a CPU with > + pending event. > > write access: > offset: > How about: [0x8] Command data: (DWORD access, little endian) If the "CPU selector" value last stored by the guest refers to an impossible CPU, then 0. Otherwise, if the "Command field" value last stored by the guest differs from 0, then 0. Otherwise, if there is at least one CPU with a pending event, then that CPU has been selected; the command data register returns that selector. Otherwise, 0. Thanks Laszlo
Re: [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec
On 10/10/19 15:31, Laszlo Ersek wrote: > 2nd round: > > On 10/09/19 15:22, Igor Mammedov wrote: >> QEMU returns 0, in case of erro or invalid value in 'Command field', >> make spec match reality, i.e. > > AHA! so this is exactly where you meant to list the particular cases > when "command data" reads as 0: > - CPU >= max_cpus selected, > - CPU with pending events asked for, but none found > >> Also fix returned value description in case 'Command field' == 0x0, >> it's in not PXM but CPU selector value with pending event >> >> Signed-off-by: Igor Mammedov >> --- >> docs/specs/acpi_cpu_hotplug.txt | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/docs/specs/acpi_cpu_hotplug.txt >> b/docs/specs/acpi_cpu_hotplug.txt >> index ee219c8358..ac5903b2b1 100644 >> --- a/docs/specs/acpi_cpu_hotplug.txt >> +++ b/docs/specs/acpi_cpu_hotplug.txt >> @@ -44,9 +44,10 @@ read access: >> 3-7: reserved and should be ignored by OSPM >> [0x5-0x7] reserved >> [0x8] Command data: (DWORD access) >> - in case of error or unsupported command reads is 0x >> + in case of error or unsupported command reads is 0x0 >>current 'Command field' value: >> - 0: returns PXM value corresponding to device >> + 0: returns CPU selector value corresponding to a CPU with >> + pending event. >> >> write access: >> offset: >> > > How about: > > [0x8] Command data: (DWORD access, little endian) > If the "CPU selector" value last stored by the guest refers to > an impossible CPU, then 0. > Otherwise, if the "Command field" value last stored by the > guest differs from 0, then 0. > Otherwise, if there is at least one CPU with a pending event, > then that CPU has been selected; the command data register > returns that selector. > Otherwise, 0. Hmmm not exactly. Let me try again. [0x8] Command data: (DWORD access, little endian) If the "CPU selector" value last stored by the guest refers to an impossible CPU, then 0. Otherwise, if the "Command field" value last stored by the guest differs from 0, then 0. Otherwise, the currently selected CPU. Thanks, Laszlo
Re: [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec
On 10/09/19 15:22, Igor Mammedov wrote: > QEMU returns 0, in case of erro or invalid value in 'Command field', > make spec match reality, i.e. Unfinished thought? > > Also fix returned value description in case 'Command field' == 0x0, double space > it's in not PXM but CPU selector value with pending event suggest dropping "in" > > Signed-off-by: Igor Mammedov > --- > docs/specs/acpi_cpu_hotplug.txt | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt > index ee219c8358..ac5903b2b1 100644 > --- a/docs/specs/acpi_cpu_hotplug.txt > +++ b/docs/specs/acpi_cpu_hotplug.txt > @@ -44,9 +44,10 @@ read access: > 3-7: reserved and should be ignored by OSPM > [0x5-0x7] reserved > [0x8] Command data: (DWORD access) since we're fixing this dword field description, can you spell out the endianness too? (I think endianness is relevant here, because patch#2 suggests selectors can be looped over. So selectors are actual integers, not just 32-bit opaque blobs.) > - in case of error or unsupported command reads is 0x > + in case of error or unsupported command reads is 0x0 >current 'Command field' value: > - 0: returns PXM value corresponding to device > + 0: returns CPU selector value corresponding to a CPU with > + pending event. > > write access: > offset: > Thanks! Laszlo
