* [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface @ 2019-10-09 13:22 Igor Mammedov 2019-10-09 13:22 ` [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec Igor Mammedov ` (3 more replies) 0 siblings, 4 replies; 43+ messages in thread From: Igor Mammedov @ 2019-10-09 13:22 UTC (permalink / raw) To: qemu-devel Cc: Eduardo Habkost, Michael S. Tsirkin, Laszlo Ersek, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson As an alternative to passing to firmware topology info via new fwcfg files so it could recreate APIC IDs based on it and order CPUs are enumerated, extend CPU hotplug interface to return APIC ID as response to the new command CPHP_GET_CPU_ID_CMD. CC: Laszlo Ersek <lersek@redhat.com> CC: Eduardo Habkost <ehabkost@redhat.com> CC: "Michael S. Tsirkin" <mst@redhat.com> CC: Gerd Hoffmann <kraxel@redhat.com> CC: Paolo Bonzini <pbonzini@redhat.com> CC: Philippe Mathieu-Daudé <philmd@redhat.com> CC: Richard Henderson <rth@twiddle.net> Igor Mammedov (3): acpi: cpuhp: fix 'Command data' description is spec acpi: cpuhp: add typical usecases into spec acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command docs/specs/acpi_cpu_hotplug.txt | 37 ++++++++++++++++++++++++++++++--- hw/acpi/cpu.c | 15 +++++++++++++ hw/acpi/trace-events | 1 + 3 files changed, 50 insertions(+), 3 deletions(-) -- 2.18.1 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec 2019-10-09 13:22 [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface Igor Mammedov @ 2019-10-09 13:22 ` Igor Mammedov 2019-10-10 12:33 ` Laszlo Ersek 2019-10-10 13:31 ` Laszlo Ersek 2019-10-09 13:22 ` [RFC 2/3] acpi: cpuhp: add typical usecases into spec Igor Mammedov ` (2 subsequent siblings) 3 siblings, 2 replies; 43+ messages in thread From: Igor Mammedov @ 2019-10-09 13:22 UTC (permalink / raw) To: qemu-devel Cc: Eduardo Habkost, Michael S. Tsirkin, Laszlo Ersek, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson QEMU returns 0, in case of erro or invalid value in 'Command field', make spec match reality, i.e. 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 <imammedo@redhat.com> --- 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 0xFFFFFFFF + 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: -- 2.18.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec 2019-10-09 13:22 ` [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec Igor Mammedov @ 2019-10-10 12:33 ` Laszlo Ersek 2019-10-17 15:41 ` Igor Mammedov 2019-10-10 13:31 ` Laszlo Ersek 1 sibling, 1 reply; 43+ messages in thread From: Laszlo Ersek @ 2019-10-10 12:33 UTC (permalink / raw) To: Igor Mammedov, qemu-devel Cc: Eduardo Habkost, Michael S. Tsirkin, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson 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 <imammedo@redhat.com> > --- > 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 0xFFFFFFFF > + 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec 2019-10-10 12:33 ` Laszlo Ersek @ 2019-10-17 15:41 ` Igor Mammedov 2019-10-18 13:24 ` Laszlo Ersek 0 siblings, 1 reply; 43+ messages in thread From: Igor Mammedov @ 2019-10-17 15:41 UTC (permalink / raw) To: Laszlo Ersek Cc: Eduardo Habkost, Michael S. Tsirkin, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On Thu, 10 Oct 2019 14:33:19 +0200 Laszlo Ersek <lersek@redhat.com> 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 0xFFFFFFFF > > + 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 > ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec 2019-10-17 15:41 ` Igor Mammedov @ 2019-10-18 13:24 ` Laszlo Ersek 0 siblings, 0 replies; 43+ messages in thread From: Laszlo Ersek @ 2019-10-18 13:24 UTC (permalink / raw) To: Igor Mammedov Cc: Eduardo Habkost, Michael S. Tsirkin, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On 10/17/19 17:41, Igor Mammedov wrote: > On Thu, 10 Oct 2019 14:33:19 +0200 > Laszlo Ersek <lersek@redhat.com> 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 0xFFFFFFFF >>> + 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 >> > ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec 2019-10-09 13:22 ` [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec Igor Mammedov 2019-10-10 12:33 ` Laszlo Ersek @ 2019-10-10 13:31 ` Laszlo Ersek 2019-10-10 13:36 ` Laszlo Ersek 1 sibling, 1 reply; 43+ messages in thread From: Laszlo Ersek @ 2019-10-10 13:31 UTC (permalink / raw) To: Igor Mammedov, qemu-devel Cc: Eduardo Habkost, Michael S. Tsirkin, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson 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 <imammedo@redhat.com> > --- > 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 0xFFFFFFFF > + 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec 2019-10-10 13:31 ` Laszlo Ersek @ 2019-10-10 13:36 ` Laszlo Ersek 2019-10-22 17:17 ` Christophe de Dinechin 0 siblings, 1 reply; 43+ messages in thread From: Laszlo Ersek @ 2019-10-10 13:36 UTC (permalink / raw) To: Igor Mammedov, qemu-devel Cc: Eduardo Habkost, Michael S. Tsirkin, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson 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 <imammedo@redhat.com> >> --- >> 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 0xFFFFFFFF >> + 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec 2019-10-10 13:36 ` Laszlo Ersek @ 2019-10-22 17:17 ` Christophe de Dinechin 2019-10-22 17:37 ` Laszlo Ersek 0 siblings, 1 reply; 43+ messages in thread From: Christophe de Dinechin @ 2019-10-22 17:17 UTC (permalink / raw) To: qemu-devel Cc: Eduardo Habkost, Michael S. Tsirkin, Gerd Hoffmann, Paolo Bonzini, Igor Mammedov, Philippe Mathieu-Daudé, Richard Henderson 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 <imammedo@redhat.com> >>> --- >>> 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 0xFFFFFFFF >>> + 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) ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec 2019-10-22 17:17 ` Christophe de Dinechin @ 2019-10-22 17:37 ` Laszlo Ersek 0 siblings, 0 replies; 43+ messages in thread From: Laszlo Ersek @ 2019-10-22 17:37 UTC (permalink / raw) To: Christophe de Dinechin, qemu-devel Cc: Eduardo Habkost, Michael S. Tsirkin, Gerd Hoffmann, Igor Mammedov, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson (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 <imammedo@redhat.com> >>>> --- >>>> 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 0xFFFFFFFF >>>> + 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC 2/3] acpi: cpuhp: add typical usecases into spec 2019-10-09 13:22 [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface Igor Mammedov 2019-10-09 13:22 ` [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec Igor Mammedov @ 2019-10-09 13:22 ` Igor Mammedov 2019-10-10 13:04 ` Laszlo Ersek 2019-10-10 14:13 ` Laszlo Ersek 2019-10-09 13:22 ` [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command Igor Mammedov 2019-10-10 9:56 ` [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface Michael S. Tsirkin 3 siblings, 2 replies; 43+ messages in thread From: Igor Mammedov @ 2019-10-09 13:22 UTC (permalink / raw) To: qemu-devel Cc: Eduardo Habkost, Michael S. Tsirkin, Laszlo Ersek, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson 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. + - 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 -- 2.18.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [RFC 2/3] acpi: cpuhp: add typical usecases into spec 2019-10-09 13:22 ` [RFC 2/3] acpi: cpuhp: add typical usecases into spec Igor Mammedov @ 2019-10-10 13:04 ` Laszlo Ersek 2019-10-10 13:15 ` Laszlo Ersek 2019-10-10 14:13 ` Laszlo Ersek 1 sibling, 1 reply; 43+ messages in thread From: Laszlo Ersek @ 2019-10-10 13:04 UTC (permalink / raw) To: Igor Mammedov, qemu-devel Cc: Eduardo Habkost, Michael S. Tsirkin, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson 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 > ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 2/3] acpi: cpuhp: add typical usecases into spec 2019-10-10 13:04 ` Laszlo Ersek @ 2019-10-10 13:15 ` Laszlo Ersek 0 siblings, 0 replies; 43+ messages in thread From: Laszlo Ersek @ 2019-10-10 13:15 UTC (permalink / raw) To: Igor Mammedov, qemu-devel Cc: Eduardo Habkost, Michael S. Tsirkin, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson 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 >> > ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 2/3] acpi: cpuhp: add typical usecases into spec 2019-10-09 13:22 ` [RFC 2/3] acpi: cpuhp: add typical usecases into spec Igor Mammedov 2019-10-10 13:04 ` Laszlo Ersek @ 2019-10-10 14:13 ` Laszlo Ersek 2019-10-18 14:45 ` Igor Mammedov 1 sibling, 1 reply; 43+ messages in thread From: Laszlo Ersek @ 2019-10-10 14:13 UTC (permalink / raw) To: Igor Mammedov, qemu-devel Cc: Eduardo Habkost, Michael S. Tsirkin, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 2/3] acpi: cpuhp: add typical usecases into spec 2019-10-10 14:13 ` Laszlo Ersek @ 2019-10-18 14:45 ` Igor Mammedov 0 siblings, 0 replies; 43+ messages in thread From: Igor Mammedov @ 2019-10-18 14:45 UTC (permalink / raw) To: Laszlo Ersek Cc: Eduardo Habkost, Michael S. Tsirkin, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command 2019-10-09 13:22 [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface Igor Mammedov 2019-10-09 13:22 ` [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec Igor Mammedov 2019-10-09 13:22 ` [RFC 2/3] acpi: cpuhp: add typical usecases into spec Igor Mammedov @ 2019-10-09 13:22 ` Igor Mammedov 2019-10-10 14:56 ` Laszlo Ersek 2019-10-24 15:07 ` Philippe Mathieu-Daudé 2019-10-10 9:56 ` [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface Michael S. Tsirkin 3 siblings, 2 replies; 43+ messages in thread From: Igor Mammedov @ 2019-10-09 13:22 UTC (permalink / raw) To: qemu-devel Cc: Eduardo Habkost, Michael S. Tsirkin, Laszlo Ersek, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson Extend CPU hotplug interface to return architecture specific identifier for current CPU (in case of x86, it's APIC ID). Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- TODO: * cripple it to behave old way on old machine types so that new firmware started on new QEMU won't see a difference when migrated to an old QEMU (i.e. QEMU that doesn't support this command) --- docs/specs/acpi_cpu_hotplug.txt | 10 +++++++++- hw/acpi/cpu.c | 15 +++++++++++++++ hw/acpi/trace-events | 1 + 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt index 43c5a193f0..0438678249 100644 --- a/docs/specs/acpi_cpu_hotplug.txt +++ b/docs/specs/acpi_cpu_hotplug.txt @@ -32,7 +32,9 @@ Register block size: read access: offset: - [0x0-0x3] reserved + [0x0-0x3] Command data 2: (DWORD access) + upper 32 bit of 'Command data' if returned data value is 64 bit. + in case of error or unsupported command reads is 0x0 [0x4] CPU device status fields: (1 byte access) bits: 0: Device is enabled and may be used by guest @@ -87,6 +89,8 @@ write access: 2: stores value into OST status register, triggers ACPI_DEVICE_OST QMP event from QEMU to external applications with current values of OST event and status registers. + 3: OSPM reads architecture specific value identifying CPU + (x86: APIC ID) other values: reserved Selecting CPU device beyond possible range has no effect on platform: @@ -115,3 +119,7 @@ Typical usecases: 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 + - Get architecture specific id for a CPU + 1. pick a CPU to read from using 'CPU selector' register + 2. write 0x3 int0 'Command field' register + 3. read architecture specific id from 'Command data' register diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index 87f30a31d7..701542d860 100644 --- a/hw/acpi/cpu.c +++ b/hw/acpi/cpu.c @@ -12,11 +12,13 @@ #define ACPI_CPU_FLAGS_OFFSET_RW 4 #define ACPI_CPU_CMD_OFFSET_WR 5 #define ACPI_CPU_CMD_DATA_OFFSET_RW 8 +#define ACPI_CPU_CMD_DATA2_OFFSET_RW 0 enum { CPHP_GET_NEXT_CPU_WITH_EVENT_CMD = 0, CPHP_OST_EVENT_CMD = 1, CPHP_OST_STATUS_CMD = 2, + CPHP_GET_CPU_ID_CMD = 3, CPHP_CMD_MAX }; @@ -74,11 +76,24 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size) case CPHP_GET_NEXT_CPU_WITH_EVENT_CMD: val = cpu_st->selector; break; + case CPHP_GET_CPU_ID_CMD: + val = cpu_to_le64(cdev->arch_id) & 0xFFFFFFFF; + break; default: break; } trace_cpuhp_acpi_read_cmd_data(cpu_st->selector, val); break; + case ACPI_CPU_CMD_DATA2_OFFSET_RW: + switch (cpu_st->command) { + case CPHP_GET_CPU_ID_CMD: + val = cpu_to_le64(cdev->arch_id) >> 32; + break; + default: + break; + } + trace_cpuhp_acpi_read_cmd_data2(cpu_st->selector, val); + break; default: break; } diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events index 96b8273297..afbc77de1c 100644 --- a/hw/acpi/trace-events +++ b/hw/acpi/trace-events @@ -23,6 +23,7 @@ cpuhp_acpi_read_flags(uint32_t idx, uint8_t flags) "idx[0x%"PRIx32"] flags: 0x%" cpuhp_acpi_write_idx(uint32_t idx) "set active cpu idx: 0x%"PRIx32 cpuhp_acpi_write_cmd(uint32_t idx, uint8_t cmd) "idx[0x%"PRIx32"] cmd: 0x%"PRIx8 cpuhp_acpi_read_cmd_data(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32 +cpuhp_acpi_read_cmd_data2(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32 cpuhp_acpi_cpu_has_events(uint32_t idx, bool ins, bool rm) "idx[0x%"PRIx32"] inserting: %d, removing: %d" cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]" cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]" -- 2.18.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command 2019-10-09 13:22 ` [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command Igor Mammedov @ 2019-10-10 14:56 ` Laszlo Ersek 2019-10-10 15:06 ` Michael S. Tsirkin 2019-10-18 16:18 ` Igor Mammedov 2019-10-24 15:07 ` Philippe Mathieu-Daudé 1 sibling, 2 replies; 43+ messages in thread From: Laszlo Ersek @ 2019-10-10 14:56 UTC (permalink / raw) To: Igor Mammedov, qemu-devel Cc: Eduardo Habkost, Michael S. Tsirkin, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On 10/09/19 15:22, Igor Mammedov wrote: > Extend CPU hotplug interface to return architecture specific > identifier for current CPU (in case of x86, it's APIC ID). > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > TODO: > * cripple it to behave old way on old machine types so that > new firmware started on new QEMU won't see a difference > when migrated to an old QEMU (i.e. QEMU that doesn't support > this command) > --- > docs/specs/acpi_cpu_hotplug.txt | 10 +++++++++- > hw/acpi/cpu.c | 15 +++++++++++++++ > hw/acpi/trace-events | 1 + > 3 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt > index 43c5a193f0..0438678249 100644 > --- a/docs/specs/acpi_cpu_hotplug.txt > +++ b/docs/specs/acpi_cpu_hotplug.txt > @@ -32,7 +32,9 @@ Register block size: > > read access: > offset: > - [0x0-0x3] reserved > + [0x0-0x3] Command data 2: (DWORD access) > + upper 32 bit of 'Command data' if returned data value is 64 bit. > + in case of error or unsupported command reads is 0x0 How about [0x0] Command data 2: (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 3, then 0. Otherwise, the most significant 32 bits of the selected CPU's architecture specific ID. [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 is 0, then the selector of the currently selected CPU; - if the "Command field" value last stored by the guest is 3, then the least significant 32 bits of the selected CPU's architecture specific ID; - otherwise, 0. > [0x4] CPU device status fields: (1 byte access) > bits: > 0: Device is enabled and may be used by guest > @@ -87,6 +89,8 @@ write access: > 2: stores value into OST status register, triggers > ACPI_DEVICE_OST QMP event from QEMU to external applications > with current values of OST event and status registers. > + 3: OSPM reads architecture specific value identifying CPU > + (x86: APIC ID) > other values: reserved > Seems OK. > Selecting CPU device beyond possible range has no effect on platform: > @@ -115,3 +119,7 @@ Typical usecases: > 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 > + - Get architecture specific id for a CPU > + 1. pick a CPU to read from using 'CPU selector' register > + 2. write 0x3 int0 'Command field' register > + 3. read architecture specific id from 'Command data' register Looks good, except for: - typo: "int0" - in step 3, we should reference 'Command data 2' as well. In fact, in <http://mid.mail-archive.com/2b10ca48-c734-4f41-9521-136c44060812@redhat.com>, I wrote, for the "Get a cpu with pending event" use case: > 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). and your steps #2 and #3, for getting the arch specific ID, can be directly appended as steps 6. and 7.! > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c > index 87f30a31d7..701542d860 100644 > --- a/hw/acpi/cpu.c > +++ b/hw/acpi/cpu.c > @@ -12,11 +12,13 @@ > #define ACPI_CPU_FLAGS_OFFSET_RW 4 > #define ACPI_CPU_CMD_OFFSET_WR 5 > #define ACPI_CPU_CMD_DATA_OFFSET_RW 8 > +#define ACPI_CPU_CMD_DATA2_OFFSET_RW 0 > > enum { > CPHP_GET_NEXT_CPU_WITH_EVENT_CMD = 0, > CPHP_OST_EVENT_CMD = 1, > CPHP_OST_STATUS_CMD = 2, > + CPHP_GET_CPU_ID_CMD = 3, > CPHP_CMD_MAX > }; > > @@ -74,11 +76,24 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size) > case CPHP_GET_NEXT_CPU_WITH_EVENT_CMD: > val = cpu_st->selector; > break; > + case CPHP_GET_CPU_ID_CMD: > + val = cpu_to_le64(cdev->arch_id) & 0xFFFFFFFF; > + break; > default: > break; > } > trace_cpuhp_acpi_read_cmd_data(cpu_st->selector, val); > break; > + case ACPI_CPU_CMD_DATA2_OFFSET_RW: > + switch (cpu_st->command) { > + case CPHP_GET_CPU_ID_CMD: > + val = cpu_to_le64(cdev->arch_id) >> 32; > + break; > + default: > + break; > + } > + trace_cpuhp_acpi_read_cmd_data2(cpu_st->selector, val); > + break; > default: > break; > } > diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events > index 96b8273297..afbc77de1c 100644 > --- a/hw/acpi/trace-events > +++ b/hw/acpi/trace-events > @@ -23,6 +23,7 @@ cpuhp_acpi_read_flags(uint32_t idx, uint8_t flags) "idx[0x%"PRIx32"] flags: 0x%" > cpuhp_acpi_write_idx(uint32_t idx) "set active cpu idx: 0x%"PRIx32 > cpuhp_acpi_write_cmd(uint32_t idx, uint8_t cmd) "idx[0x%"PRIx32"] cmd: 0x%"PRIx8 > cpuhp_acpi_read_cmd_data(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32 > +cpuhp_acpi_read_cmd_data2(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32 > cpuhp_acpi_cpu_has_events(uint32_t idx, bool ins, bool rm) "idx[0x%"PRIx32"] inserting: %d, removing: %d" > cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]" > cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]" > Looks plausible to me, thanks (discounting the TODO item). Right now, I can't offer testing for patch#3 (I'm quite far from the point where I'll be actually looking for a hotplugged CPU :) ), but based on the docs patches #1 and #2, and my proposed updates, I can rework my "possible CPU count detection" in OVMF. Do I need to check in OVMF specifically whether the "modern" CPU hotplug register block is available? Can you tell me what the oldest machine types are that support the modern interface? Hmm... Commit abd49bc2ed2f ("docs: update ACPI CPU hotplug spec with new protocol", 2016-06-24) seems relevant. First released in v2.7.0. I think I should detect whether this interface is available. Can I use the following sequence to detect whether the interface is available? 1. Store 0x0 to command register. 2. Store 0x0 to selector register. 3. Read 'command data' register. 4. If value read is 0, the interface is available. (Because I assume that unmapped IO ports read as all-bits-one. Is that right?) BTW, can I dynamically detect support for the GET_CPU_ID command too? I'm thinking, when I enumerate / count all possible CPUs, I can at once fetch the arch IDs for all of them. If I only get zeros from the command data registers, across all CPUs, in response to GET_CPU_ID, then the command is not available. Thanks Laszlo ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command 2019-10-10 14:56 ` Laszlo Ersek @ 2019-10-10 15:06 ` Michael S. Tsirkin 2019-10-10 17:23 ` Igor Mammedov ` (2 more replies) 2019-10-18 16:18 ` Igor Mammedov 1 sibling, 3 replies; 43+ messages in thread From: Michael S. Tsirkin @ 2019-10-10 15:06 UTC (permalink / raw) To: Laszlo Ersek Cc: Eduardo Habkost, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Igor Mammedov, Philippe Mathieu-Daudé, Richard Henderson On Thu, Oct 10, 2019 at 04:56:18PM +0200, Laszlo Ersek wrote: > On 10/09/19 15:22, Igor Mammedov wrote: > > Extend CPU hotplug interface to return architecture specific > > identifier for current CPU (in case of x86, it's APIC ID). > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > TODO: > > * cripple it to behave old way on old machine types so that > > new firmware started on new QEMU won't see a difference > > when migrated to an old QEMU (i.e. QEMU that doesn't support > > this command) > > --- > > docs/specs/acpi_cpu_hotplug.txt | 10 +++++++++- > > hw/acpi/cpu.c | 15 +++++++++++++++ > > hw/acpi/trace-events | 1 + > > 3 files changed, 25 insertions(+), 1 deletion(-) > > > > diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt > > index 43c5a193f0..0438678249 100644 > > --- a/docs/specs/acpi_cpu_hotplug.txt > > +++ b/docs/specs/acpi_cpu_hotplug.txt > > @@ -32,7 +32,9 @@ Register block size: > > > > read access: > > offset: > > - [0x0-0x3] reserved > > + [0x0-0x3] Command data 2: (DWORD access) > > + upper 32 bit of 'Command data' if returned data value is 64 bit. > > + in case of error or unsupported command reads is 0x0 > > How about > > [0x0] Command data 2: (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 3, then 0. > Otherwise, the most significant 32 bits of the selected CPU's > architecture specific ID. > > [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 is 0, > then the selector of the currently selected CPU; > - if the "Command field" value last stored by the guest is 3, > then the least significant 32 bits of the selected CPU's > architecture specific ID; > - otherwise, 0. > > > [0x4] CPU device status fields: (1 byte access) > > bits: > > 0: Device is enabled and may be used by guest > > @@ -87,6 +89,8 @@ write access: > > 2: stores value into OST status register, triggers > > ACPI_DEVICE_OST QMP event from QEMU to external applications > > with current values of OST event and status registers. > > + 3: OSPM reads architecture specific value identifying CPU > > + (x86: APIC ID) > > other values: reserved > > > > Seems OK. > > > Selecting CPU device beyond possible range has no effect on platform: > > @@ -115,3 +119,7 @@ Typical usecases: > > 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 > > + - Get architecture specific id for a CPU > > + 1. pick a CPU to read from using 'CPU selector' register > > + 2. write 0x3 int0 'Command field' register > > + 3. read architecture specific id from 'Command data' register > > Looks good, except for: > > - typo: "int0" > > - in step 3, we should reference 'Command data 2' as well. > > > In fact, in > <http://mid.mail-archive.com/2b10ca48-c734-4f41-9521-136c44060812@redhat.com>, > I wrote, for the "Get a cpu with pending event" use case: > > > 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). > > and your steps #2 and #3, for getting the arch specific ID, can be > directly appended as steps 6. and 7.! > > > > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c > > index 87f30a31d7..701542d860 100644 > > --- a/hw/acpi/cpu.c > > +++ b/hw/acpi/cpu.c > > @@ -12,11 +12,13 @@ > > #define ACPI_CPU_FLAGS_OFFSET_RW 4 > > #define ACPI_CPU_CMD_OFFSET_WR 5 > > #define ACPI_CPU_CMD_DATA_OFFSET_RW 8 > > +#define ACPI_CPU_CMD_DATA2_OFFSET_RW 0 > > > > enum { > > CPHP_GET_NEXT_CPU_WITH_EVENT_CMD = 0, > > CPHP_OST_EVENT_CMD = 1, > > CPHP_OST_STATUS_CMD = 2, > > + CPHP_GET_CPU_ID_CMD = 3, > > CPHP_CMD_MAX > > }; > > > > @@ -74,11 +76,24 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size) > > case CPHP_GET_NEXT_CPU_WITH_EVENT_CMD: > > val = cpu_st->selector; > > break; > > + case CPHP_GET_CPU_ID_CMD: > > + val = cpu_to_le64(cdev->arch_id) & 0xFFFFFFFF; > > + break; > > default: > > break; > > } > > trace_cpuhp_acpi_read_cmd_data(cpu_st->selector, val); > > break; > > + case ACPI_CPU_CMD_DATA2_OFFSET_RW: > > + switch (cpu_st->command) { > > + case CPHP_GET_CPU_ID_CMD: > > + val = cpu_to_le64(cdev->arch_id) >> 32; > > + break; > > + default: > > + break; > > + } > > + trace_cpuhp_acpi_read_cmd_data2(cpu_st->selector, val); > > + break; > > default: > > break; > > } > > diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events > > index 96b8273297..afbc77de1c 100644 > > --- a/hw/acpi/trace-events > > +++ b/hw/acpi/trace-events > > @@ -23,6 +23,7 @@ cpuhp_acpi_read_flags(uint32_t idx, uint8_t flags) "idx[0x%"PRIx32"] flags: 0x%" > > cpuhp_acpi_write_idx(uint32_t idx) "set active cpu idx: 0x%"PRIx32 > > cpuhp_acpi_write_cmd(uint32_t idx, uint8_t cmd) "idx[0x%"PRIx32"] cmd: 0x%"PRIx8 > > cpuhp_acpi_read_cmd_data(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32 > > +cpuhp_acpi_read_cmd_data2(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32 > > cpuhp_acpi_cpu_has_events(uint32_t idx, bool ins, bool rm) "idx[0x%"PRIx32"] inserting: %d, removing: %d" > > cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]" > > cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]" > > > > Looks plausible to me, thanks (discounting the TODO item). > > Right now, I can't offer testing for patch#3 (I'm quite far from the > point where I'll be actually looking for a hotplugged CPU :) ), but > based on the docs patches #1 and #2, and my proposed updates, I can > rework my "possible CPU count detection" in OVMF. > > Do I need to check in OVMF specifically whether the "modern" CPU hotplug > register block is available? Can you tell me what the oldest machine > types are that support the modern interface? > > Hmm... Commit abd49bc2ed2f ("docs: update ACPI CPU hotplug spec with new > protocol", 2016-06-24) seems relevant. First released in v2.7.0. I think > I should detect whether this interface is available. > > Can I use the following sequence to detect whether the interface is > available? > > 1. Store 0x0 to command register. > 2. Store 0x0 to selector register. > 3. Read 'command data' register. > 4. If value read is 0, the interface is available. > > (Because I assume that unmapped IO ports read as all-bits-one. Is that > right?) > > BTW, can I dynamically detect support for the GET_CPU_ID command too? > I'm thinking, when I enumerate / count all possible CPUs, I can at once > fetch the arch IDs for all of them. If I only get zeros from the command > data registers, across all CPUs, in response to GET_CPU_ID, then the > command is not available. > > Thanks > Laszlo Laszlo, won't we need to add topology info anyway? if yes then this patch is just a stopgap, so let's do fw cfg and be done with it? -- MST ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command 2019-10-10 15:06 ` Michael S. Tsirkin @ 2019-10-10 17:23 ` Igor Mammedov 2019-10-10 17:53 ` Laszlo Ersek 2019-10-10 19:26 ` Eduardo Habkost 2 siblings, 0 replies; 43+ messages in thread From: Igor Mammedov @ 2019-10-10 17:23 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Eduardo Habkost, Laszlo Ersek, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On Thu, 10 Oct 2019 11:06:29 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, Oct 10, 2019 at 04:56:18PM +0200, Laszlo Ersek wrote: > > On 10/09/19 15:22, Igor Mammedov wrote: > > > Extend CPU hotplug interface to return architecture specific > > > identifier for current CPU (in case of x86, it's APIC ID). > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > TODO: > > > * cripple it to behave old way on old machine types so that > > > new firmware started on new QEMU won't see a difference > > > when migrated to an old QEMU (i.e. QEMU that doesn't support > > > this command) > > > --- > > > docs/specs/acpi_cpu_hotplug.txt | 10 +++++++++- > > > hw/acpi/cpu.c | 15 +++++++++++++++ > > > hw/acpi/trace-events | 1 + > > > 3 files changed, 25 insertions(+), 1 deletion(-) > > > > > > diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt > > > index 43c5a193f0..0438678249 100644 > > > --- a/docs/specs/acpi_cpu_hotplug.txt > > > +++ b/docs/specs/acpi_cpu_hotplug.txt > > > @@ -32,7 +32,9 @@ Register block size: > > > > > > read access: > > > offset: > > > - [0x0-0x3] reserved > > > + [0x0-0x3] Command data 2: (DWORD access) > > > + upper 32 bit of 'Command data' if returned data value is 64 bit. > > > + in case of error or unsupported command reads is 0x0 > > > > How about > > > > [0x0] Command data 2: (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 3, then 0. > > Otherwise, the most significant 32 bits of the selected CPU's > > architecture specific ID. > > > > [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 is 0, > > then the selector of the currently selected CPU; > > - if the "Command field" value last stored by the guest is 3, > > then the least significant 32 bits of the selected CPU's > > architecture specific ID; > > - otherwise, 0. > > > > > [0x4] CPU device status fields: (1 byte access) > > > bits: > > > 0: Device is enabled and may be used by guest > > > @@ -87,6 +89,8 @@ write access: > > > 2: stores value into OST status register, triggers > > > ACPI_DEVICE_OST QMP event from QEMU to external applications > > > with current values of OST event and status registers. > > > + 3: OSPM reads architecture specific value identifying CPU > > > + (x86: APIC ID) > > > other values: reserved > > > > > > > Seems OK. > > > > > Selecting CPU device beyond possible range has no effect on platform: > > > @@ -115,3 +119,7 @@ Typical usecases: > > > 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 > > > + - Get architecture specific id for a CPU > > > + 1. pick a CPU to read from using 'CPU selector' register > > > + 2. write 0x3 int0 'Command field' register > > > + 3. read architecture specific id from 'Command data' register > > > > Looks good, except for: > > > > - typo: "int0" > > > > - in step 3, we should reference 'Command data 2' as well. > > > > > > In fact, in > > <http://mid.mail-archive.com/2b10ca48-c734-4f41-9521-136c44060812@redhat.com>, > > I wrote, for the "Get a cpu with pending event" use case: > > > > > 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). > > > > and your steps #2 and #3, for getting the arch specific ID, can be > > directly appended as steps 6. and 7.! > > > > > > > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c > > > index 87f30a31d7..701542d860 100644 > > > --- a/hw/acpi/cpu.c > > > +++ b/hw/acpi/cpu.c > > > @@ -12,11 +12,13 @@ > > > #define ACPI_CPU_FLAGS_OFFSET_RW 4 > > > #define ACPI_CPU_CMD_OFFSET_WR 5 > > > #define ACPI_CPU_CMD_DATA_OFFSET_RW 8 > > > +#define ACPI_CPU_CMD_DATA2_OFFSET_RW 0 > > > > > > enum { > > > CPHP_GET_NEXT_CPU_WITH_EVENT_CMD = 0, > > > CPHP_OST_EVENT_CMD = 1, > > > CPHP_OST_STATUS_CMD = 2, > > > + CPHP_GET_CPU_ID_CMD = 3, > > > CPHP_CMD_MAX > > > }; > > > > > > @@ -74,11 +76,24 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size) > > > case CPHP_GET_NEXT_CPU_WITH_EVENT_CMD: > > > val = cpu_st->selector; > > > break; > > > + case CPHP_GET_CPU_ID_CMD: > > > + val = cpu_to_le64(cdev->arch_id) & 0xFFFFFFFF; > > > + break; > > > default: > > > break; > > > } > > > trace_cpuhp_acpi_read_cmd_data(cpu_st->selector, val); > > > break; > > > + case ACPI_CPU_CMD_DATA2_OFFSET_RW: > > > + switch (cpu_st->command) { > > > + case CPHP_GET_CPU_ID_CMD: > > > + val = cpu_to_le64(cdev->arch_id) >> 32; > > > + break; > > > + default: > > > + break; > > > + } > > > + trace_cpuhp_acpi_read_cmd_data2(cpu_st->selector, val); > > > + break; > > > default: > > > break; > > > } > > > diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events > > > index 96b8273297..afbc77de1c 100644 > > > --- a/hw/acpi/trace-events > > > +++ b/hw/acpi/trace-events > > > @@ -23,6 +23,7 @@ cpuhp_acpi_read_flags(uint32_t idx, uint8_t flags) "idx[0x%"PRIx32"] flags: 0x%" > > > cpuhp_acpi_write_idx(uint32_t idx) "set active cpu idx: 0x%"PRIx32 > > > cpuhp_acpi_write_cmd(uint32_t idx, uint8_t cmd) "idx[0x%"PRIx32"] cmd: 0x%"PRIx8 > > > cpuhp_acpi_read_cmd_data(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32 > > > +cpuhp_acpi_read_cmd_data2(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32 > > > cpuhp_acpi_cpu_has_events(uint32_t idx, bool ins, bool rm) "idx[0x%"PRIx32"] inserting: %d, removing: %d" > > > cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]" > > > cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]" > > > > > > > Looks plausible to me, thanks (discounting the TODO item). > > > > Right now, I can't offer testing for patch#3 (I'm quite far from the > > point where I'll be actually looking for a hotplugged CPU :) ), but > > based on the docs patches #1 and #2, and my proposed updates, I can > > rework my "possible CPU count detection" in OVMF. > > > > Do I need to check in OVMF specifically whether the "modern" CPU hotplug > > register block is available? Can you tell me what the oldest machine > > types are that support the modern interface? > > > > Hmm... Commit abd49bc2ed2f ("docs: update ACPI CPU hotplug spec with new > > protocol", 2016-06-24) seems relevant. First released in v2.7.0. I think > > I should detect whether this interface is available. > > > > Can I use the following sequence to detect whether the interface is > > available? > > > > 1. Store 0x0 to command register. > > 2. Store 0x0 to selector register. > > 3. Read 'command data' register. > > 4. If value read is 0, the interface is available. > > > > (Because I assume that unmapped IO ports read as all-bits-one. Is that > > right?) > > > > BTW, can I dynamically detect support for the GET_CPU_ID command too? > > I'm thinking, when I enumerate / count all possible CPUs, I can at once > > fetch the arch IDs for all of them. If I only get zeros from the command > > data registers, across all CPUs, in response to GET_CPU_ID, then the > > command is not available. > > > > Thanks > > Laszlo > > Laszlo, won't we need to add topology info anyway? > if yes then this patch is just a stopgap, so let's do > fw cfg and be done with it? answer to your question and suggestion is in following thread "Re: [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command" " In fact, it seems like OVMF will have to synthesize the new (hot-plugged) VCPU's *APIC-ID* from the following information sources: - the topology information described above (die / core / thread counts), and - the "modern" CPU hotplug interface (docs/specs/acpi_cpu_hotplug.txt). " ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command 2019-10-10 15:06 ` Michael S. Tsirkin 2019-10-10 17:23 ` Igor Mammedov @ 2019-10-10 17:53 ` Laszlo Ersek 2019-10-10 19:26 ` Eduardo Habkost 2 siblings, 0 replies; 43+ messages in thread From: Laszlo Ersek @ 2019-10-10 17:53 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Eduardo Habkost, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Igor Mammedov, Philippe Mathieu-Daudé, Richard Henderson On 10/10/19 17:06, Michael S. Tsirkin wrote: > On Thu, Oct 10, 2019 at 04:56:18PM +0200, Laszlo Ersek wrote: >> On 10/09/19 15:22, Igor Mammedov wrote: >>> Extend CPU hotplug interface to return architecture specific >>> identifier for current CPU (in case of x86, it's APIC ID). >>> >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >>> --- >>> TODO: >>> * cripple it to behave old way on old machine types so that >>> new firmware started on new QEMU won't see a difference >>> when migrated to an old QEMU (i.e. QEMU that doesn't support >>> this command) >>> --- >>> docs/specs/acpi_cpu_hotplug.txt | 10 +++++++++- >>> hw/acpi/cpu.c | 15 +++++++++++++++ >>> hw/acpi/trace-events | 1 + >>> 3 files changed, 25 insertions(+), 1 deletion(-) >>> >>> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt >>> index 43c5a193f0..0438678249 100644 >>> --- a/docs/specs/acpi_cpu_hotplug.txt >>> +++ b/docs/specs/acpi_cpu_hotplug.txt >>> @@ -32,7 +32,9 @@ Register block size: >>> >>> read access: >>> offset: >>> - [0x0-0x3] reserved >>> + [0x0-0x3] Command data 2: (DWORD access) >>> + upper 32 bit of 'Command data' if returned data value is 64 bit. >>> + in case of error or unsupported command reads is 0x0 >> >> How about >> >> [0x0] Command data 2: (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 3, then 0. >> Otherwise, the most significant 32 bits of the selected CPU's >> architecture specific ID. >> >> [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 is 0, >> then the selector of the currently selected CPU; >> - if the "Command field" value last stored by the guest is 3, >> then the least significant 32 bits of the selected CPU's >> architecture specific ID; >> - otherwise, 0. >> >>> [0x4] CPU device status fields: (1 byte access) >>> bits: >>> 0: Device is enabled and may be used by guest >>> @@ -87,6 +89,8 @@ write access: >>> 2: stores value into OST status register, triggers >>> ACPI_DEVICE_OST QMP event from QEMU to external applications >>> with current values of OST event and status registers. >>> + 3: OSPM reads architecture specific value identifying CPU >>> + (x86: APIC ID) >>> other values: reserved >>> >> >> Seems OK. >> >>> Selecting CPU device beyond possible range has no effect on platform: >>> @@ -115,3 +119,7 @@ Typical usecases: >>> 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 >>> + - Get architecture specific id for a CPU >>> + 1. pick a CPU to read from using 'CPU selector' register >>> + 2. write 0x3 int0 'Command field' register >>> + 3. read architecture specific id from 'Command data' register >> >> Looks good, except for: >> >> - typo: "int0" >> >> - in step 3, we should reference 'Command data 2' as well. >> >> >> In fact, in >> <http://mid.mail-archive.com/2b10ca48-c734-4f41-9521-136c44060812@redhat.com>, >> I wrote, for the "Get a cpu with pending event" use case: >> >>> 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). >> >> and your steps #2 and #3, for getting the arch specific ID, can be >> directly appended as steps 6. and 7.! >> >> >>> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c >>> index 87f30a31d7..701542d860 100644 >>> --- a/hw/acpi/cpu.c >>> +++ b/hw/acpi/cpu.c >>> @@ -12,11 +12,13 @@ >>> #define ACPI_CPU_FLAGS_OFFSET_RW 4 >>> #define ACPI_CPU_CMD_OFFSET_WR 5 >>> #define ACPI_CPU_CMD_DATA_OFFSET_RW 8 >>> +#define ACPI_CPU_CMD_DATA2_OFFSET_RW 0 >>> >>> enum { >>> CPHP_GET_NEXT_CPU_WITH_EVENT_CMD = 0, >>> CPHP_OST_EVENT_CMD = 1, >>> CPHP_OST_STATUS_CMD = 2, >>> + CPHP_GET_CPU_ID_CMD = 3, >>> CPHP_CMD_MAX >>> }; >>> >>> @@ -74,11 +76,24 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size) >>> case CPHP_GET_NEXT_CPU_WITH_EVENT_CMD: >>> val = cpu_st->selector; >>> break; >>> + case CPHP_GET_CPU_ID_CMD: >>> + val = cpu_to_le64(cdev->arch_id) & 0xFFFFFFFF; >>> + break; >>> default: >>> break; >>> } >>> trace_cpuhp_acpi_read_cmd_data(cpu_st->selector, val); >>> break; >>> + case ACPI_CPU_CMD_DATA2_OFFSET_RW: >>> + switch (cpu_st->command) { >>> + case CPHP_GET_CPU_ID_CMD: >>> + val = cpu_to_le64(cdev->arch_id) >> 32; >>> + break; >>> + default: >>> + break; >>> + } >>> + trace_cpuhp_acpi_read_cmd_data2(cpu_st->selector, val); >>> + break; >>> default: >>> break; >>> } >>> diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events >>> index 96b8273297..afbc77de1c 100644 >>> --- a/hw/acpi/trace-events >>> +++ b/hw/acpi/trace-events >>> @@ -23,6 +23,7 @@ cpuhp_acpi_read_flags(uint32_t idx, uint8_t flags) "idx[0x%"PRIx32"] flags: 0x%" >>> cpuhp_acpi_write_idx(uint32_t idx) "set active cpu idx: 0x%"PRIx32 >>> cpuhp_acpi_write_cmd(uint32_t idx, uint8_t cmd) "idx[0x%"PRIx32"] cmd: 0x%"PRIx8 >>> cpuhp_acpi_read_cmd_data(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32 >>> +cpuhp_acpi_read_cmd_data2(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32 >>> cpuhp_acpi_cpu_has_events(uint32_t idx, bool ins, bool rm) "idx[0x%"PRIx32"] inserting: %d, removing: %d" >>> cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]" >>> cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]" >>> >> >> Looks plausible to me, thanks (discounting the TODO item). >> >> Right now, I can't offer testing for patch#3 (I'm quite far from the >> point where I'll be actually looking for a hotplugged CPU :) ), but >> based on the docs patches #1 and #2, and my proposed updates, I can >> rework my "possible CPU count detection" in OVMF. >> >> Do I need to check in OVMF specifically whether the "modern" CPU hotplug >> register block is available? Can you tell me what the oldest machine >> types are that support the modern interface? >> >> Hmm... Commit abd49bc2ed2f ("docs: update ACPI CPU hotplug spec with new >> protocol", 2016-06-24) seems relevant. First released in v2.7.0. I think >> I should detect whether this interface is available. >> >> Can I use the following sequence to detect whether the interface is >> available? >> >> 1. Store 0x0 to command register. >> 2. Store 0x0 to selector register. >> 3. Read 'command data' register. >> 4. If value read is 0, the interface is available. >> >> (Because I assume that unmapped IO ports read as all-bits-one. Is that >> right?) >> >> BTW, can I dynamically detect support for the GET_CPU_ID command too? >> I'm thinking, when I enumerate / count all possible CPUs, I can at once >> fetch the arch IDs for all of them. If I only get zeros from the command >> data registers, across all CPUs, in response to GET_CPU_ID, then the >> command is not available. >> >> Thanks >> Laszlo > > Laszlo, won't we need to add topology info anyway? > if yes then this patch is just a stopgap, so let's do > fw cfg and be done with it? No, CPU topology is not relevant *per se* for this OVMF feature (CPU hotplug with SMM). CPU topology is only interesting as a building block. What OVMF really needs, for the hotplug SMI handler, is the ability to: - learn a QEMU-specific unique identifier for the CPU that has just been hot-plugged, - translate said QEMU-specific unique CPU identifier to the new CPU's APIC-ID. So how do we get there? - If the hotplug register block (the hotplug hardware interface) lets me fetch the APIC-ID for the new CPU immediately, in the hotplug SMI handler, that's best (most convenient). - If the QEMU-specific unique identifier fetched from the register block is a sequential CPU index (or "selector"), then I *could* use the CPU topology as a building block, for constructing the APIC-ID myself. I would decompose the CPU index into thread / core / die / socket offsets, based on the topology, and re-compose those offsets into an APIC-ID. - If the QEMU-specific unique identifier fetched from the register block is a sequential CPU index (or "selector"), then I could *also* use a complete mapping table (index to APIC-ID), fetched earlier from fw_cfg. I would use the CPU index as a subscript into that table. What really matters for OVMF is the APIC-ID of the just-plugged CPU. I definitely have to access the CPU hotplug register block in the SMI handler, because that's where I can get *any kind* of identifier for the new CPU. (Unless we want to implement a dedicated CPU hotplug controller for OVMF, that is.) Thanks Laszlo ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command 2019-10-10 15:06 ` Michael S. Tsirkin 2019-10-10 17:23 ` Igor Mammedov 2019-10-10 17:53 ` Laszlo Ersek @ 2019-10-10 19:26 ` Eduardo Habkost 2019-10-11 8:07 ` Laszlo Ersek 2 siblings, 1 reply; 43+ messages in thread From: Eduardo Habkost @ 2019-10-10 19:26 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Laszlo Ersek, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Igor Mammedov, Philippe Mathieu-Daudé, Richard Henderson On Thu, Oct 10, 2019 at 11:06:29AM -0400, Michael S. Tsirkin wrote: > On Thu, Oct 10, 2019 at 04:56:18PM +0200, Laszlo Ersek wrote: > > On 10/09/19 15:22, Igor Mammedov wrote: > > > Extend CPU hotplug interface to return architecture specific > > > identifier for current CPU (in case of x86, it's APIC ID). > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > TODO: > > > * cripple it to behave old way on old machine types so that > > > new firmware started on new QEMU won't see a difference > > > when migrated to an old QEMU (i.e. QEMU that doesn't support > > > this command) > > > --- > > > docs/specs/acpi_cpu_hotplug.txt | 10 +++++++++- > > > hw/acpi/cpu.c | 15 +++++++++++++++ > > > hw/acpi/trace-events | 1 + > > > 3 files changed, 25 insertions(+), 1 deletion(-) > > > > > > diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt > > > index 43c5a193f0..0438678249 100644 > > > --- a/docs/specs/acpi_cpu_hotplug.txt > > > +++ b/docs/specs/acpi_cpu_hotplug.txt > > > @@ -32,7 +32,9 @@ Register block size: > > > > > > read access: > > > offset: > > > - [0x0-0x3] reserved > > > + [0x0-0x3] Command data 2: (DWORD access) > > > + upper 32 bit of 'Command data' if returned data value is 64 bit. > > > + in case of error or unsupported command reads is 0x0 > > > > How about > > > > [0x0] Command data 2: (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 3, then 0. > > Otherwise, the most significant 32 bits of the selected CPU's > > architecture specific ID. > > > > [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 is 0, > > then the selector of the currently selected CPU; > > - if the "Command field" value last stored by the guest is 3, > > then the least significant 32 bits of the selected CPU's > > architecture specific ID; > > - otherwise, 0. > > > > > [0x4] CPU device status fields: (1 byte access) > > > bits: > > > 0: Device is enabled and may be used by guest > > > @@ -87,6 +89,8 @@ write access: > > > 2: stores value into OST status register, triggers > > > ACPI_DEVICE_OST QMP event from QEMU to external applications > > > with current values of OST event and status registers. > > > + 3: OSPM reads architecture specific value identifying CPU > > > + (x86: APIC ID) > > > other values: reserved > > > > > > > Seems OK. > > > > > Selecting CPU device beyond possible range has no effect on platform: > > > @@ -115,3 +119,7 @@ Typical usecases: > > > 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 > > > + - Get architecture specific id for a CPU > > > + 1. pick a CPU to read from using 'CPU selector' register > > > + 2. write 0x3 int0 'Command field' register > > > + 3. read architecture specific id from 'Command data' register > > > > Looks good, except for: > > > > - typo: "int0" > > > > - in step 3, we should reference 'Command data 2' as well. > > > > > > In fact, in > > <http://mid.mail-archive.com/2b10ca48-c734-4f41-9521-136c44060812@redhat.com>, > > I wrote, for the "Get a cpu with pending event" use case: > > > > > 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). > > > > and your steps #2 and #3, for getting the arch specific ID, can be > > directly appended as steps 6. and 7.! > > > > > > > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c > > > index 87f30a31d7..701542d860 100644 > > > --- a/hw/acpi/cpu.c > > > +++ b/hw/acpi/cpu.c > > > @@ -12,11 +12,13 @@ > > > #define ACPI_CPU_FLAGS_OFFSET_RW 4 > > > #define ACPI_CPU_CMD_OFFSET_WR 5 > > > #define ACPI_CPU_CMD_DATA_OFFSET_RW 8 > > > +#define ACPI_CPU_CMD_DATA2_OFFSET_RW 0 > > > > > > enum { > > > CPHP_GET_NEXT_CPU_WITH_EVENT_CMD = 0, > > > CPHP_OST_EVENT_CMD = 1, > > > CPHP_OST_STATUS_CMD = 2, > > > + CPHP_GET_CPU_ID_CMD = 3, > > > CPHP_CMD_MAX > > > }; > > > > > > @@ -74,11 +76,24 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size) > > > case CPHP_GET_NEXT_CPU_WITH_EVENT_CMD: > > > val = cpu_st->selector; > > > break; > > > + case CPHP_GET_CPU_ID_CMD: > > > + val = cpu_to_le64(cdev->arch_id) & 0xFFFFFFFF; > > > + break; > > > default: > > > break; > > > } > > > trace_cpuhp_acpi_read_cmd_data(cpu_st->selector, val); > > > break; > > > + case ACPI_CPU_CMD_DATA2_OFFSET_RW: > > > + switch (cpu_st->command) { > > > + case CPHP_GET_CPU_ID_CMD: > > > + val = cpu_to_le64(cdev->arch_id) >> 32; > > > + break; > > > + default: > > > + break; > > > + } > > > + trace_cpuhp_acpi_read_cmd_data2(cpu_st->selector, val); > > > + break; > > > default: > > > break; > > > } > > > diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events > > > index 96b8273297..afbc77de1c 100644 > > > --- a/hw/acpi/trace-events > > > +++ b/hw/acpi/trace-events > > > @@ -23,6 +23,7 @@ cpuhp_acpi_read_flags(uint32_t idx, uint8_t flags) "idx[0x%"PRIx32"] flags: 0x%" > > > cpuhp_acpi_write_idx(uint32_t idx) "set active cpu idx: 0x%"PRIx32 > > > cpuhp_acpi_write_cmd(uint32_t idx, uint8_t cmd) "idx[0x%"PRIx32"] cmd: 0x%"PRIx8 > > > cpuhp_acpi_read_cmd_data(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32 > > > +cpuhp_acpi_read_cmd_data2(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32 > > > cpuhp_acpi_cpu_has_events(uint32_t idx, bool ins, bool rm) "idx[0x%"PRIx32"] inserting: %d, removing: %d" > > > cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]" > > > cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]" > > > > > > > Looks plausible to me, thanks (discounting the TODO item). > > > > Right now, I can't offer testing for patch#3 (I'm quite far from the > > point where I'll be actually looking for a hotplugged CPU :) ), but > > based on the docs patches #1 and #2, and my proposed updates, I can > > rework my "possible CPU count detection" in OVMF. > > > > Do I need to check in OVMF specifically whether the "modern" CPU hotplug > > register block is available? Can you tell me what the oldest machine > > types are that support the modern interface? > > > > Hmm... Commit abd49bc2ed2f ("docs: update ACPI CPU hotplug spec with new > > protocol", 2016-06-24) seems relevant. First released in v2.7.0. I think > > I should detect whether this interface is available. > > > > Can I use the following sequence to detect whether the interface is > > available? > > > > 1. Store 0x0 to command register. > > 2. Store 0x0 to selector register. > > 3. Read 'command data' register. > > 4. If value read is 0, the interface is available. > > > > (Because I assume that unmapped IO ports read as all-bits-one. Is that > > right?) > > > > BTW, can I dynamically detect support for the GET_CPU_ID command too? > > I'm thinking, when I enumerate / count all possible CPUs, I can at once > > fetch the arch IDs for all of them. If I only get zeros from the command > > data registers, across all CPUs, in response to GET_CPU_ID, then the > > command is not available. > > > > Thanks > > Laszlo > > Laszlo, won't we need to add topology info anyway? > if yes then this patch is just a stopgap, so let's do > fw cfg and be done with it? Topology info is already available on CPUID. -- Eduardo ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command 2019-10-10 19:26 ` Eduardo Habkost @ 2019-10-11 8:07 ` Laszlo Ersek 0 siblings, 0 replies; 43+ messages in thread From: Laszlo Ersek @ 2019-10-11 8:07 UTC (permalink / raw) To: Eduardo Habkost, Michael S. Tsirkin Cc: qemu-devel, Gerd Hoffmann, Paolo Bonzini, Igor Mammedov, Philippe Mathieu-Daudé, Richard Henderson On 10/10/19 21:26, Eduardo Habkost wrote: > Topology info is already available on CPUID. Independently of everything else, thanks for pointing this out. The edk2 library called "LocalApicLib" has two relevant functions: > /** > Get Package ID/Core ID/Thread ID of a processor. > > The algorithm assumes the target system has symmetry across physical > package boundaries with respect to the number of logical processors > per package, number of cores per package. > > @param[in] InitialApicId Initial APIC ID of the target logical processor. > @param[out] Package Returns the processor package ID. > @param[out] Core Returns the processor core ID. > @param[out] Thread Returns the processor thread ID. > **/ > VOID > EFIAPI > GetProcessorLocationByApicId ( > IN UINT32 InitialApicId, > OUT UINT32 *Package OPTIONAL, > OUT UINT32 *Core OPTIONAL, > OUT UINT32 *Thread OPTIONAL > ); > > /** > Get Package ID/Module ID/Tile ID/Die ID/Core ID/Thread ID of a processor. > > The algorithm assumes the target system has symmetry across physical > package boundaries with respect to the number of threads per core, number of > cores per module, number of modules per tile, number of tiles per die, number > of dies per package. > > @param[in] InitialApicId Initial APIC ID of the target logical processor. > @param[out] Package Returns the processor package ID. > @param[out] Die Returns the processor die ID. > @param[out] Tile Returns the processor tile ID. > @param[out] Module Returns the processor module ID. > @param[out] Core Returns the processor core ID. > @param[out] Thread Returns the processor thread ID. > **/ > VOID > EFIAPI > GetProcessorLocation2ByApicId ( > IN UINT32 InitialApicId, > OUT UINT32 *Package OPTIONAL, > OUT UINT32 *Die OPTIONAL, > OUT UINT32 *Tile OPTIONAL, > OUT UINT32 *Module OPTIONAL, > OUT UINT32 *Core OPTIONAL, > OUT UINT32 *Thread OPTIONAL > ); They are implemented with heavy CPUID usage. So... just give me the APIC-ID. That's the primary key in edk2 for identifying x86 processors. Thanks Laszlo ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command 2019-10-10 14:56 ` Laszlo Ersek 2019-10-10 15:06 ` Michael S. Tsirkin @ 2019-10-18 16:18 ` Igor Mammedov 2019-10-21 13:06 ` Laszlo Ersek 1 sibling, 1 reply; 43+ messages in thread From: Igor Mammedov @ 2019-10-18 16:18 UTC (permalink / raw) To: Laszlo Ersek Cc: Eduardo Habkost, Michael S. Tsirkin, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On Thu, 10 Oct 2019 16:56:18 +0200 Laszlo Ersek <lersek@redhat.com> wrote: > On 10/09/19 15:22, Igor Mammedov wrote: > > Extend CPU hotplug interface to return architecture specific > > identifier for current CPU (in case of x86, it's APIC ID). > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > TODO: > > * cripple it to behave old way on old machine types so that > > new firmware started on new QEMU won't see a difference > > when migrated to an old QEMU (i.e. QEMU that doesn't support > > this command) > > --- > > docs/specs/acpi_cpu_hotplug.txt | 10 +++++++++- > > hw/acpi/cpu.c | 15 +++++++++++++++ > > hw/acpi/trace-events | 1 + > > 3 files changed, 25 insertions(+), 1 deletion(-) > > > > diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt > > index 43c5a193f0..0438678249 100644 > > --- a/docs/specs/acpi_cpu_hotplug.txt > > +++ b/docs/specs/acpi_cpu_hotplug.txt > > @@ -32,7 +32,9 @@ Register block size: > > > > read access: > > offset: > > - [0x0-0x3] reserved > > + [0x0-0x3] Command data 2: (DWORD access) > > + upper 32 bit of 'Command data' if returned data value is 64 bit. > > + in case of error or unsupported command reads is 0x0 > > How about > > [0x0] Command data 2: (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 3, then 0. > Otherwise, the most significant 32 bits of the selected CPU's > architecture specific ID. > > [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 is 0, > then the selector of the currently selected CPU; > - if the "Command field" value last stored by the guest is 3, > then the least significant 32 bits of the selected CPU's > architecture specific ID; > - otherwise, 0. this format is easier to read comparing to suggestion on [1/3] https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg02256.html So I'll use it in [1/3] and then just extend it here > > > [0x4] CPU device status fields: (1 byte access) > > bits: > > 0: Device is enabled and may be used by guest > > @@ -87,6 +89,8 @@ write access: > > 2: stores value into OST status register, triggers > > ACPI_DEVICE_OST QMP event from QEMU to external applications > > with current values of OST event and status registers. > > + 3: OSPM reads architecture specific value identifying CPU > > + (x86: APIC ID) > > other values: reserved > > > > Seems OK. > > > Selecting CPU device beyond possible range has no effect on platform: > > @@ -115,3 +119,7 @@ Typical usecases: > > 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 > > + - Get architecture specific id for a CPU > > + 1. pick a CPU to read from using 'CPU selector' register > > + 2. write 0x3 int0 'Command field' register > > + 3. read architecture specific id from 'Command data' register > > Looks good, except for: > > - typo: "int0" > > - in step 3, we should reference 'Command data 2' as well. > > > In fact, in > <http://mid.mail-archive.com/2b10ca48-c734-4f41-9521-136c44060812@redhat.com>, > I wrote, for the "Get a cpu with pending event" use case: > > > 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). > > and your steps #2 and #3, for getting the arch specific ID, can be > directly appended as steps 6. and 7.! > > > > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c > > index 87f30a31d7..701542d860 100644 > > --- a/hw/acpi/cpu.c > > +++ b/hw/acpi/cpu.c > > @@ -12,11 +12,13 @@ > > #define ACPI_CPU_FLAGS_OFFSET_RW 4 > > #define ACPI_CPU_CMD_OFFSET_WR 5 > > #define ACPI_CPU_CMD_DATA_OFFSET_RW 8 > > +#define ACPI_CPU_CMD_DATA2_OFFSET_RW 0 > > > > enum { > > CPHP_GET_NEXT_CPU_WITH_EVENT_CMD = 0, > > CPHP_OST_EVENT_CMD = 1, > > CPHP_OST_STATUS_CMD = 2, > > + CPHP_GET_CPU_ID_CMD = 3, > > CPHP_CMD_MAX > > }; > > > > @@ -74,11 +76,24 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size) > > case CPHP_GET_NEXT_CPU_WITH_EVENT_CMD: > > val = cpu_st->selector; > > break; > > + case CPHP_GET_CPU_ID_CMD: > > + val = cpu_to_le64(cdev->arch_id) & 0xFFFFFFFF; > > + break; > > default: > > break; > > } > > trace_cpuhp_acpi_read_cmd_data(cpu_st->selector, val); > > break; > > + case ACPI_CPU_CMD_DATA2_OFFSET_RW: > > + switch (cpu_st->command) { > > + case CPHP_GET_CPU_ID_CMD: > > + val = cpu_to_le64(cdev->arch_id) >> 32; > > + break; > > + default: > > + break; > > + } > > + trace_cpuhp_acpi_read_cmd_data2(cpu_st->selector, val); > > + break; > > default: > > break; > > } > > diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events > > index 96b8273297..afbc77de1c 100644 > > --- a/hw/acpi/trace-events > > +++ b/hw/acpi/trace-events > > @@ -23,6 +23,7 @@ cpuhp_acpi_read_flags(uint32_t idx, uint8_t flags) "idx[0x%"PRIx32"] flags: 0x%" > > cpuhp_acpi_write_idx(uint32_t idx) "set active cpu idx: 0x%"PRIx32 > > cpuhp_acpi_write_cmd(uint32_t idx, uint8_t cmd) "idx[0x%"PRIx32"] cmd: 0x%"PRIx8 > > cpuhp_acpi_read_cmd_data(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32 > > +cpuhp_acpi_read_cmd_data2(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32 > > cpuhp_acpi_cpu_has_events(uint32_t idx, bool ins, bool rm) "idx[0x%"PRIx32"] inserting: %d, removing: %d" > > cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]" > > cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]" > > > > Looks plausible to me, thanks (discounting the TODO item). > > Right now, I can't offer testing for patch#3 (I'm quite far from the > point where I'll be actually looking for a hotplugged CPU :) ), but > based on the docs patches #1 and #2, and my proposed updates, I can > rework my "possible CPU count detection" in OVMF. > > Do I need to check in OVMF specifically whether the "modern" CPU hotplug > register block is available? Can you tell me what the oldest machine > types are that support the modern interface? See 679dd1a95 (pc: use new CPU hotplug interface since 2.7 machine type) > Hmm... Commit abd49bc2ed2f ("docs: update ACPI CPU hotplug spec with new > protocol", 2016-06-24) seems relevant. First released in v2.7.0. I think > I should detect whether this interface is available. Can you make detection based on QEMU version (is dynamic detection really necessary)? > Can I use the following sequence to detect whether the interface is > available? > > 1. Store 0x0 to command register. > 2. Store 0x0 to selector register. > 3. Read 'command data' register. > 4. If value read is 0, the interface is available. By default legacy register block layout is in place (i.e. present cpus bitmap) where 1st byte is guarantied to be ">0" as it has at least the boot CPU bit set and writes to legacy bitmap are ignored. Currently AML code code does switching to modern interface, see docs/specs/acpi_cpu_hotplug.txt: " The first DWORD in bitmap is used in write mode to switch from legacy to new CPU hotplug interface, write 0 into it to do switch. " related code "if (opts.has_legacy_cphp) {" and cpu_status_write() Considering firmware runs the first, it should enable modern interface on its own 1. Store 0x0 to selector register (actually it's store into bitmap to attempt switch). and to check if interface is present 2. Store 0x0 to selector register (to ensure valid selector value (otherwise command is ignored)) 3. Store 0x0 to command register (to be able to read back selector from command data) 4. Store 0x0 to selector register (because #3 can select the a cpu with events if any) be aware libvirt may start QEMU in paused mode (hotplug context) and hotplugs extra CPUs with device_add and then let guest run. So firmware may see present CPUs with events at boot time. 5. Read 'command data' register. 6. If value read is 0, the interface is available. > (Because I assume that unmapped IO ports read as all-bits-one. Is that > right?) that's right but ports are mapped to legacy CPU bitmap, you can't count on all-bits-one case here. > BTW, can I dynamically detect support for the GET_CPU_ID command too? > I'm thinking, when I enumerate / count all possible CPUs, I can at once > fetch the arch IDs for all of them. If I only get zeros from the command > data registers, across all CPUs, in response to GET_CPU_ID, then the > command is not available. APICID == 0 is valid value, so one would be need to account for ' -smp 1 ' case where the only valid selector is 0 that leads to APIC ID = 0 If counted maxcpus > 1, then what you suggest will work if you pick the last CPU (apic id != 0). (at least for x86 guest, I don't know if it's fine wrt ARM guest) May be dynamic detection is just over-engineering. > Thanks > Laszlo > ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command 2019-10-18 16:18 ` Igor Mammedov @ 2019-10-21 13:06 ` Laszlo Ersek 2019-10-22 12:39 ` Laszlo Ersek 0 siblings, 1 reply; 43+ messages in thread From: Laszlo Ersek @ 2019-10-21 13:06 UTC (permalink / raw) To: Igor Mammedov Cc: Eduardo Habkost, Michael S. Tsirkin, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson Hello Igor, On 10/18/19 18:18, Igor Mammedov wrote: > On Thu, 10 Oct 2019 16:56:18 +0200 > Laszlo Ersek <lersek@redhat.com> wrote: [...] >> Right now, I can't offer testing for patch#3 (I'm quite far from the >> point where I'll be actually looking for a hotplugged CPU :) ), but >> based on the docs patches #1 and #2, and my proposed updates, I can >> rework my "possible CPU count detection" in OVMF. >> >> Do I need to check in OVMF specifically whether the "modern" CPU hotplug >> register block is available? Can you tell me what the oldest machine >> types are that support the modern interface? > See 679dd1a95 (pc: use new CPU hotplug interface since 2.7 machine type) > > >> Hmm... Commit abd49bc2ed2f ("docs: update ACPI CPU hotplug spec with new >> protocol", 2016-06-24) seems relevant. First released in v2.7.0. I think >> I should detect whether this interface is available. > Can you make detection based on QEMU version (is dynamic detection really necessary)? Thanks for following up on this; indeed it has been my remaining open question in this thread. I attempted to investigate a bit myself a few days ago, and indeed I can now find the commit hash "679dd1a95" in my bash history. So... If I can somehow query the QEMU machine type version -- or emulator version? -- inside the guest, I'm perfectly happy using that. I don't need dynamic detection specifically for this feature, I just need something to deduce the feature from. The problem is that the "possible CPU count" determination would run in OVMF in every case, regardless of whether OVMF was built with -D SMM_REQUIRE, and regardless of board type (q35 vs. i440fx). Requiring QEMU v2.7.0 or later just for booting OVMF would be a bit steep. So the idea is: - check (somehow) if QEMU has the modern hotplug register block - available: great, now set the boot CPU count and the possible CPU count independently of each other - otherwise: set both values to the boot CPU count (which we can fetch from fw_cfg, like we've always done) >> Can I use the following sequence to detect whether the interface is >> available? >> >> 1. Store 0x0 to command register. >> 2. Store 0x0 to selector register. >> 3. Read 'command data' register. >> 4. If value read is 0, the interface is available. > > By default legacy register block layout is in place > (i.e. present cpus bitmap) where 1st byte is guarantied to be ">0" as it has > at least the boot CPU bit set and writes to legacy bitmap are ignored. > > Currently AML code code does switching to modern interface, see > docs/specs/acpi_cpu_hotplug.txt: > " > The first DWORD in bitmap is used in write mode to switch from legacy > to new CPU hotplug interface, write 0 into it to do switch. > " > related code "if (opts.has_legacy_cphp) {" and cpu_status_write() > > Considering firmware runs the first, it should enable modern interface > on its own > 1. Store 0x0 to selector register (actually it's store into bitmap to attempt switch). > and to check if interface is present > 2. Store 0x0 to selector register (to ensure valid selector value (otherwise command is ignored)) > 3. Store 0x0 to command register (to be able to read back selector from command data) > 4. Store 0x0 to selector register (because #3 can select the a cpu with events if any) > be aware libvirt may start QEMU in paused mode (hotplug context) and hotplugs extra CPUs > with device_add and then let guest run. So firmware may see present CPUs with events > at boot time. > 5. Read 'command data' register. > 6. If value read is 0, the interface is available. Perfect! Basically this is prepending two "write 0 to selector register" steps to what I suspected. I certainly couldn't figure out the "switch to modern" step, and whether initializing the selector to something valid was needed at boot. Now I know. :) Thanks! > >> (Because I assume that unmapped IO ports read as all-bits-one. Is that >> right?) > that's right but ports are mapped to legacy CPU bitmap, you can't count on all-bits-one case here. > >> BTW, can I dynamically detect support for the GET_CPU_ID command too? >> I'm thinking, when I enumerate / count all possible CPUs, I can at once >> fetch the arch IDs for all of them. If I only get zeros from the command >> data registers, across all CPUs, in response to GET_CPU_ID, then the >> command is not available. > > APICID == 0 is valid value, so one would be need to account for ' -smp 1 ' > case where the only valid selector is 0 that leads to APIC ID = 0 > > If counted maxcpus > 1, then what you suggest will work > if you pick the last CPU (apic id != 0). (at least for x86 guest, > I don't know if it's fine wrt ARM guest) > > May be dynamic detection is just over-engineering. Dynamically detecting the presence of the GET_CPU_ID command is not important. That's because GET_CPU_ID is for a new use case (CPU hotplug with SMM), which has never worked before. So we can't regress it. We'll just modify the OvmfPkg/README file to spell out the minimum machine type requirement: don't try to hotplug a CPU when running OVMF built with SMM_REQUIRE, unless your machine type is pc-q35-4.2+ which is exactly what we did for base SMM: * The minimum required QEMU machine type is "pc-q35-2.5". The "possible CPU count" determination is more risky because that will modify a code path that's active on every boot, in every possible environment. I think I can rework my OVMF series for the "possible CPU count" fetching now. Thank you! Laszlo ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command 2019-10-21 13:06 ` Laszlo Ersek @ 2019-10-22 12:39 ` Laszlo Ersek 2019-10-22 14:42 ` Igor Mammedov 0 siblings, 1 reply; 43+ messages in thread From: Laszlo Ersek @ 2019-10-22 12:39 UTC (permalink / raw) To: Igor Mammedov Cc: Eduardo Habkost, Michael S. Tsirkin, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On 10/21/19 15:06, Laszlo Ersek wrote: > On 10/18/19 18:18, Igor Mammedov wrote: >> On Thu, 10 Oct 2019 16:56:18 +0200 >> Laszlo Ersek <lersek@redhat.com> wrote: [...] >>> Can I use the following sequence to detect whether the interface is >>> available? >>> >>> 1. Store 0x0 to command register. >>> 2. Store 0x0 to selector register. >>> 3. Read 'command data' register. >>> 4. If value read is 0, the interface is available. >> >> By default legacy register block layout is in place >> (i.e. present cpus bitmap) where 1st byte is guarantied to be ">0" as it has >> at least the boot CPU bit set and writes to legacy bitmap are ignored. >> >> Currently AML code code does switching to modern interface, see >> docs/specs/acpi_cpu_hotplug.txt: >> " >> The first DWORD in bitmap is used in write mode to switch from legacy >> to new CPU hotplug interface, write 0 into it to do switch. >> " >> related code "if (opts.has_legacy_cphp) {" and cpu_status_write() >> >> Considering firmware runs the first, it should enable modern interface >> on its own >> 1. Store 0x0 to selector register (actually it's store into bitmap to attempt switch). >> and to check if interface is present >> 2. Store 0x0 to selector register (to ensure valid selector value (otherwise command is ignored)) >> 3. Store 0x0 to command register (to be able to read back selector from command data) >> 4. Store 0x0 to selector register (because #3 can select the a cpu with events if any) >> be aware libvirt may start QEMU in paused mode (hotplug context) and hotplugs extra CPUs >> with device_add and then let guest run. So firmware may see present CPUs with events >> at boot time. >> 5. Read 'command data' register. >> 6. If value read is 0, the interface is available. > > Perfect! > > Basically this is prepending two "write 0 to selector register" steps to > what I suspected. I certainly couldn't figure out the "switch to modern" > step, and whether initializing the selector to something valid was > needed at boot. Now I know. :) Thanks! > >> >>> (Because I assume that unmapped IO ports read as all-bits-one. Is that >>> right?) >> that's right but ports are mapped to legacy CPU bitmap, you can't count on all-bits-one case here. It seems I rejoiced too soon. When we read the command data register in the last step, that is at offset 0x8 in the register block. Considering the legacy "CPU present bitmap", if no CPU is present in that range, then the firmware could read a zero value. I got confused because I thought we were reading at offset 0, which would always have bit0 set (for CPU#0). Can we detect the modern interface like this: 1. store 0x0 to selector register (attempt to switch) 2. read one byte at offset 0 in the register block 3. if bit#0 is set, the modern interface is unavailable; otherwise (= bit#0 clear), the modern interface is available Here's why: - if even the legacy interface is missing, then step 2 is an unassigned read, hence the value read is all-bits-one; bit#0 is set - if only the legacy interface is available, then bit#0 stands for CPU#0, it will be set - if the switch-over in step 1 is successful, then offset 0 is reserved, hence it returns all-bits-zero. With this, if we ever assigned offset 0 for reading, then we'd have to define it with bit#0 constantly clear. Thanks, Laszlo ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command 2019-10-22 12:39 ` Laszlo Ersek @ 2019-10-22 14:42 ` Igor Mammedov 2019-10-22 15:49 ` Laszlo Ersek 0 siblings, 1 reply; 43+ messages in thread From: Igor Mammedov @ 2019-10-22 14:42 UTC (permalink / raw) To: Laszlo Ersek Cc: Eduardo Habkost, Michael S. Tsirkin, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On Tue, 22 Oct 2019 14:39:24 +0200 Laszlo Ersek <lersek@redhat.com> wrote: > On 10/21/19 15:06, Laszlo Ersek wrote: > > On 10/18/19 18:18, Igor Mammedov wrote: > >> On Thu, 10 Oct 2019 16:56:18 +0200 > >> Laszlo Ersek <lersek@redhat.com> wrote: > > [...] > > >>> Can I use the following sequence to detect whether the interface is > >>> available? > >>> > >>> 1. Store 0x0 to command register. > >>> 2. Store 0x0 to selector register. > >>> 3. Read 'command data' register. > >>> 4. If value read is 0, the interface is available. > >> > >> By default legacy register block layout is in place > >> (i.e. present cpus bitmap) where 1st byte is guarantied to be ">0" as it has > >> at least the boot CPU bit set and writes to legacy bitmap are ignored. > >> > >> Currently AML code code does switching to modern interface, see > >> docs/specs/acpi_cpu_hotplug.txt: > >> " > >> The first DWORD in bitmap is used in write mode to switch from legacy > >> to new CPU hotplug interface, write 0 into it to do switch. > >> " > >> related code "if (opts.has_legacy_cphp) {" and cpu_status_write() > >> > >> Considering firmware runs the first, it should enable modern interface > >> on its own > >> 1. Store 0x0 to selector register (actually it's store into bitmap to attempt switch). > >> and to check if interface is present > >> 2. Store 0x0 to selector register (to ensure valid selector value (otherwise command is ignored)) > >> 3. Store 0x0 to command register (to be able to read back selector from command data) > >> 4. Store 0x0 to selector register (because #3 can select the a cpu with events if any) > >> be aware libvirt may start QEMU in paused mode (hotplug context) and hotplugs extra CPUs > >> with device_add and then let guest run. So firmware may see present CPUs with events > >> at boot time. > >> 5. Read 'command data' register. > >> 6. If value read is 0, the interface is available. > > > > Perfect! > > > > Basically this is prepending two "write 0 to selector register" steps to > > what I suspected. I certainly couldn't figure out the "switch to modern" > > step, and whether initializing the selector to something valid was > > needed at boot. Now I know. :) Thanks! > > > >> > >>> (Because I assume that unmapped IO ports read as all-bits-one. Is that > >>> right?) > >> that's right but ports are mapped to legacy CPU bitmap, you can't count on all-bits-one case here. > > It seems I rejoiced too soon. > > When we read the command data register in the last step, that is at > offset 0x8 in the register block. Considering the legacy "CPU present > bitmap", if no CPU is present in that range, then the firmware could > read a zero value. I got confused because I thought we were reading at > offset 0, which would always have bit0 set (for CPU#0). > > Can we detect the modern interface like this: > > 1. store 0x0 to selector register (attempt to switch) > 2. read one byte at offset 0 in the register block > 3. if bit#0 is set, the modern interface is unavailable; > otherwise (= bit#0 clear), the modern interface is available > > Here's why: > > - if even the legacy interface is missing, then step 2 is an unassigned > read, hence the value read is all-bits-one; bit#0 is set > > - if only the legacy interface is available, then bit#0 stands for > CPU#0, it will be set > > - if the switch-over in step 1 is successful, then offset 0 is reserved, > hence it returns all-bits-zero. > > With this, if we ever assigned offset 0 for reading, then we'd have to > define it with bit#0 constantly clear. There is no need to reserve bit#0 if in step #5 we use s/'command data'/'Command data 2'/ Alternatively we can reserve bit#0 and sequentially read upper half from 'Command data' (one a new flag to show that there is more data to read). (Upper half currently is not necessary, it's there for future ARM's MPIDR). One more thing, this behavior is based on artifacts of x86 machine and AllOnes fallback. Obviously it won't work with arm/virt, do we care about AVMF at this point? > Thanks, > Laszlo ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command 2019-10-22 14:42 ` Igor Mammedov @ 2019-10-22 15:49 ` Laszlo Ersek 2019-10-23 14:59 ` Igor Mammedov 0 siblings, 1 reply; 43+ messages in thread From: Laszlo Ersek @ 2019-10-22 15:49 UTC (permalink / raw) To: Igor Mammedov Cc: Eduardo Habkost, Michael S. Tsirkin, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On 10/22/19 16:42, Igor Mammedov wrote: > On Tue, 22 Oct 2019 14:39:24 +0200 > Laszlo Ersek <lersek@redhat.com> wrote: > >> On 10/21/19 15:06, Laszlo Ersek wrote: >>> On 10/18/19 18:18, Igor Mammedov wrote: >>>> Considering firmware runs the first, it should enable modern interface >>>> on its own >>>> 1. Store 0x0 to selector register (actually it's store into bitmap to attempt switch). >>>> and to check if interface is present >>>> 2. Store 0x0 to selector register (to ensure valid selector value (otherwise command is ignored)) >>>> 3. Store 0x0 to command register (to be able to read back selector from command data) >>>> 4. Store 0x0 to selector register (because #3 can select the a cpu with events if any) >>>> be aware libvirt may start QEMU in paused mode (hotplug context) and hotplugs extra CPUs >>>> with device_add and then let guest run. So firmware may see present CPUs with events >>>> at boot time. >>>> 5. Read 'command data' register. >>>> 6. If value read is 0, the interface is available. >> When we read the command data register in the last step, that is at >> offset 0x8 in the register block. Considering the legacy "CPU present >> bitmap", if no CPU is present in that range, then the firmware could >> read a zero value. I got confused because I thought we were reading at >> offset 0, which would always have bit0 set (for CPU#0). >> >> Can we detect the modern interface like this: >> >> 1. store 0x0 to selector register (attempt to switch) >> 2. read one byte at offset 0 in the register block >> 3. if bit#0 is set, the modern interface is unavailable; >> otherwise (= bit#0 clear), the modern interface is available >> >> Here's why: >> >> - if even the legacy interface is missing, then step 2 is an unassigned >> read, hence the value read is all-bits-one; bit#0 is set >> >> - if only the legacy interface is available, then bit#0 stands for >> CPU#0, it will be set >> >> - if the switch-over in step 1 is successful, then offset 0 is reserved, >> hence it returns all-bits-zero. >> >> With this, if we ever assigned offset 0 for reading, then we'd have to >> define it with bit#0 constantly clear. > > There is no need to reserve bit#0 if in step #5 we use s/'command data'/'Command data 2'/ Good idea. We can drop step 4 too: [0x0] Command data 2: (DWORD access, little endian) If the "CPU selector" value last stored by the guest refers to an impossible CPU, then 0. This is skipped by step 2. Otherwise, if the "Command field" value last stored by the guest differs from 3, then 0. This is triggered by step 3. So step 4 does not look necessary. (As long as the guest is OK with the selector ending up with a changed value.) Otherwise, the most significant 32 bits of the selected CPU's architecture specific ID. Not relevant for this use case. > Alternatively we can reserve bit#0 and sequentially read upper half from 'Command data' > (one a new flag to show that there is more data to read). I like the "Command data 2" register more. The "temporal domain" is always a complication in register definitions. > (Upper half currently is not necessary, it's there for future ARM's MPIDR). > > One more thing, this behavior is based on artifacts of x86 machine and AllOnes fallback. > Obviously it won't work with arm/virt, do we care about AVMF at this point? No, in the firmware, all this is strictly x86 code. The ArmVirtQemu guest firmware has no support for multiprocessing at this time, to my understanding. (Nonetheless, if the register block got placed at an MMIO base address on arm/virt, I think "unassigned_mem_ops" would apply there just the same.) Thanks! Laszlo ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command 2019-10-22 15:49 ` Laszlo Ersek @ 2019-10-23 14:59 ` Igor Mammedov 0 siblings, 0 replies; 43+ messages in thread From: Igor Mammedov @ 2019-10-23 14:59 UTC (permalink / raw) To: Laszlo Ersek Cc: Eduardo Habkost, Michael S. Tsirkin, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On Tue, 22 Oct 2019 17:49:06 +0200 Laszlo Ersek <lersek@redhat.com> wrote: > On 10/22/19 16:42, Igor Mammedov wrote: > > On Tue, 22 Oct 2019 14:39:24 +0200 > > Laszlo Ersek <lersek@redhat.com> wrote: > > > >> On 10/21/19 15:06, Laszlo Ersek wrote: > >>> On 10/18/19 18:18, Igor Mammedov wrote: > > >>>> Considering firmware runs the first, it should enable modern interface > >>>> on its own > >>>> 1. Store 0x0 to selector register (actually it's store into bitmap to attempt switch). > >>>> and to check if interface is present > >>>> 2. Store 0x0 to selector register (to ensure valid selector value (otherwise command is ignored)) > >>>> 3. Store 0x0 to command register (to be able to read back selector from command data) > >>>> 4. Store 0x0 to selector register (because #3 can select the a cpu with events if any) > >>>> be aware libvirt may start QEMU in paused mode (hotplug context) and hotplugs extra CPUs > >>>> with device_add and then let guest run. So firmware may see present CPUs with events > >>>> at boot time. > >>>> 5. Read 'command data' register. > >>>> 6. If value read is 0, the interface is available. > > >> When we read the command data register in the last step, that is at > >> offset 0x8 in the register block. Considering the legacy "CPU present > >> bitmap", if no CPU is present in that range, then the firmware could > >> read a zero value. I got confused because I thought we were reading at > >> offset 0, which would always have bit0 set (for CPU#0). > >> > >> Can we detect the modern interface like this: > >> > >> 1. store 0x0 to selector register (attempt to switch) > >> 2. read one byte at offset 0 in the register block > >> 3. if bit#0 is set, the modern interface is unavailable; > >> otherwise (= bit#0 clear), the modern interface is available > >> > >> Here's why: > >> > >> - if even the legacy interface is missing, then step 2 is an unassigned > >> read, hence the value read is all-bits-one; bit#0 is set > >> > >> - if only the legacy interface is available, then bit#0 stands for > >> CPU#0, it will be set > >> > >> - if the switch-over in step 1 is successful, then offset 0 is reserved, > >> hence it returns all-bits-zero. > >> > >> With this, if we ever assigned offset 0 for reading, then we'd have to > >> define it with bit#0 constantly clear. > > > > There is no need to reserve bit#0 if in step #5 we use s/'command data'/'Command data 2'/ > > Good idea. We can drop step 4 too: > > [0x0] Command data 2: (DWORD access, little endian) > If the "CPU selector" value last stored by the guest refers to > an impossible CPU, then 0. > > This is skipped by step 2. > > Otherwise, if the "Command field" value last stored by the > guest differs from 3, then 0. > > This is triggered by step 3. > > So step 4 does not look necessary. (As long as the guest is OK with the > selector ending up with a changed value.) sounds good, I'll respin patches taking this into account. > Otherwise, the most significant 32 bits of the selected CPU's > architecture specific ID. > > Not relevant for this use case. > > > Alternatively we can reserve bit#0 and sequentially read upper half from 'Command data' > > (one a new flag to show that there is more data to read). > > I like the "Command data 2" register more. The "temporal domain" is > always a complication in register definitions. > > > (Upper half currently is not necessary, it's there for future ARM's MPIDR). > > > > One more thing, this behavior is based on artifacts of x86 machine and AllOnes fallback. > > Obviously it won't work with arm/virt, do we care about AVMF at this point? > > No, in the firmware, all this is strictly x86 code. The ArmVirtQemu > guest firmware has no support for multiprocessing at this time, to my > understanding. > > (Nonetheless, if the register block got placed at an MMIO base address > on arm/virt, I think "unassigned_mem_ops" would apply there just the same.) > > Thanks! > Laszlo > > ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command 2019-10-09 13:22 ` [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command Igor Mammedov 2019-10-10 14:56 ` Laszlo Ersek @ 2019-10-24 15:07 ` Philippe Mathieu-Daudé 1 sibling, 0 replies; 43+ messages in thread From: Philippe Mathieu-Daudé @ 2019-10-24 15:07 UTC (permalink / raw) To: Igor Mammedov, qemu-devel Cc: Eduardo Habkost, Michael S. Tsirkin, Gerd Hoffmann, Paolo Bonzini, Laszlo Ersek, Richard Henderson Hi Igor, On 10/9/19 3:22 PM, Igor Mammedov wrote: > Extend CPU hotplug interface to return architecture specific > identifier for current CPU (in case of x86, it's APIC ID). > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > TODO: > * cripple it to behave old way on old machine types so that > new firmware started on new QEMU won't see a difference > when migrated to an old QEMU (i.e. QEMU that doesn't support > this command) > --- > docs/specs/acpi_cpu_hotplug.txt | 10 +++++++++- > hw/acpi/cpu.c | 15 +++++++++++++++ > hw/acpi/trace-events | 1 + > 3 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt > index 43c5a193f0..0438678249 100644 > --- a/docs/specs/acpi_cpu_hotplug.txt > +++ b/docs/specs/acpi_cpu_hotplug.txt > @@ -32,7 +32,9 @@ Register block size: > > read access: > offset: > - [0x0-0x3] reserved > + [0x0-0x3] Command data 2: (DWORD access) > + upper 32 bit of 'Command data' if returned data value is 64 bit. > + in case of error or unsupported command reads is 0x0 > [0x4] CPU device status fields: (1 byte access) > bits: > 0: Device is enabled and may be used by guest > @@ -87,6 +89,8 @@ write access: > 2: stores value into OST status register, triggers > ACPI_DEVICE_OST QMP event from QEMU to external applications > with current values of OST event and status registers. > + 3: OSPM reads architecture specific value identifying CPU > + (x86: APIC ID) > other values: reserved > > Selecting CPU device beyond possible range has no effect on platform: > @@ -115,3 +119,7 @@ Typical usecases: > 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 > + - Get architecture specific id for a CPU > + 1. pick a CPU to read from using 'CPU selector' register > + 2. write 0x3 int0 'Command field' register > + 3. read architecture specific id from 'Command data' register > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c > index 87f30a31d7..701542d860 100644 > --- a/hw/acpi/cpu.c > +++ b/hw/acpi/cpu.c > @@ -12,11 +12,13 @@ > #define ACPI_CPU_FLAGS_OFFSET_RW 4 > #define ACPI_CPU_CMD_OFFSET_WR 5 > #define ACPI_CPU_CMD_DATA_OFFSET_RW 8 > +#define ACPI_CPU_CMD_DATA2_OFFSET_RW 0 This part confuses me: #define ACPI_CPU_SELECTOR_OFFSET_WR 0 #define ACPI_CPU_CMD_DATA2_OFFSET_RW 0 What about: #define ACPI_CPU_SELECTOR_OFFSET_W 0 #define ACPI_CPU_CMD_DATA2_OFFSET_R 0 > > enum { > CPHP_GET_NEXT_CPU_WITH_EVENT_CMD = 0, > CPHP_OST_EVENT_CMD = 1, > CPHP_OST_STATUS_CMD = 2, > + CPHP_GET_CPU_ID_CMD = 3, > CPHP_CMD_MAX > }; > > @@ -74,11 +76,24 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size) > case CPHP_GET_NEXT_CPU_WITH_EVENT_CMD: > val = cpu_st->selector; > break; > + case CPHP_GET_CPU_ID_CMD: > + val = cpu_to_le64(cdev->arch_id) & 0xFFFFFFFF; > + break; > default: > break; > } > trace_cpuhp_acpi_read_cmd_data(cpu_st->selector, val); > break; > + case ACPI_CPU_CMD_DATA2_OFFSET_RW: > + switch (cpu_st->command) { > + case CPHP_GET_CPU_ID_CMD: > + val = cpu_to_le64(cdev->arch_id) >> 32; > + break; > + default: > + break; > + } > + trace_cpuhp_acpi_read_cmd_data2(cpu_st->selector, val); > + break; > default: > break; > } > diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events > index 96b8273297..afbc77de1c 100644 > --- a/hw/acpi/trace-events > +++ b/hw/acpi/trace-events > @@ -23,6 +23,7 @@ cpuhp_acpi_read_flags(uint32_t idx, uint8_t flags) "idx[0x%"PRIx32"] flags: 0x%" > cpuhp_acpi_write_idx(uint32_t idx) "set active cpu idx: 0x%"PRIx32 > cpuhp_acpi_write_cmd(uint32_t idx, uint8_t cmd) "idx[0x%"PRIx32"] cmd: 0x%"PRIx8 > cpuhp_acpi_read_cmd_data(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32 > +cpuhp_acpi_read_cmd_data2(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32 > cpuhp_acpi_cpu_has_events(uint32_t idx, bool ins, bool rm) "idx[0x%"PRIx32"] inserting: %d, removing: %d" > cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]" > cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]" > ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface 2019-10-09 13:22 [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface Igor Mammedov ` (2 preceding siblings ...) 2019-10-09 13:22 ` [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command Igor Mammedov @ 2019-10-10 9:56 ` Michael S. Tsirkin 2019-10-10 13:39 ` Igor Mammedov 3 siblings, 1 reply; 43+ messages in thread From: Michael S. Tsirkin @ 2019-10-10 9:56 UTC (permalink / raw) To: Igor Mammedov Cc: Eduardo Habkost, Laszlo Ersek, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On Wed, Oct 09, 2019 at 09:22:49AM -0400, Igor Mammedov wrote: > As an alternative to passing to firmware topology info via new fwcfg files > so it could recreate APIC IDs based on it and order CPUs are enumerated, > > extend CPU hotplug interface to return APIC ID as response to the new command > CPHP_GET_CPU_ID_CMD. One big piece missing here is motivation: Who's going to use this interface? So far CPU hotplug was used by the ACPI, so we didn't really commit to a fixed interface too strongly. Is this a replacement to Laszlo's fw cfg interface? If yes is the idea that OVMF going to depend on CPU hotplug directly then? It does not depend on it now, does it? If answers to all of the above is yes, then I don't really like it: it is better to keep all paravirt stuff in one place, namely in fw cfg. > > CC: Laszlo Ersek <lersek@redhat.com> > CC: Eduardo Habkost <ehabkost@redhat.com> > CC: "Michael S. Tsirkin" <mst@redhat.com> > CC: Gerd Hoffmann <kraxel@redhat.com> > CC: Paolo Bonzini <pbonzini@redhat.com> > CC: Philippe Mathieu-Daudé <philmd@redhat.com> > CC: Richard Henderson <rth@twiddle.net> > > Igor Mammedov (3): > acpi: cpuhp: fix 'Command data' description is spec > acpi: cpuhp: add typical usecases into spec > acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command > > docs/specs/acpi_cpu_hotplug.txt | 37 ++++++++++++++++++++++++++++++--- > hw/acpi/cpu.c | 15 +++++++++++++ > hw/acpi/trace-events | 1 + > 3 files changed, 50 insertions(+), 3 deletions(-) > > -- > 2.18.1 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface 2019-10-10 9:56 ` [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface Michael S. Tsirkin @ 2019-10-10 13:39 ` Igor Mammedov 2019-10-10 13:59 ` Michael S. Tsirkin 2019-10-10 14:16 ` Eduardo Habkost 0 siblings, 2 replies; 43+ messages in thread From: Igor Mammedov @ 2019-10-10 13:39 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Eduardo Habkost, Laszlo Ersek, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On Thu, 10 Oct 2019 05:56:55 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Oct 09, 2019 at 09:22:49AM -0400, Igor Mammedov wrote: > > As an alternative to passing to firmware topology info via new fwcfg files > > so it could recreate APIC IDs based on it and order CPUs are enumerated, > > > > extend CPU hotplug interface to return APIC ID as response to the new command > > CPHP_GET_CPU_ID_CMD. > > One big piece missing here is motivation: I thought the only willing reader was Laszlo (who is aware of context) so I skipped on details and confused others :/ > Who's going to use this interface? In current state it's for firmware, since ACPI tables can cheat by having APIC IDs statically built in. If we were creating CPU objects in ACPI dynamically we would be using this command as well. It would save us quite a bit space in ACPI blob but it would be a pain to debug and diagnose problems in ACPI tables, so I'd rather stay with static CPU descriptions in ACPI tables for the sake of maintenance. > So far CPU hotplug was used by the ACPI, so we didn't > really commit to a fixed interface too strongly. > > Is this a replacement to Laszlo's fw cfg interface? > If yes is the idea that OVMF going to depend on CPU hotplug directly then? > It does not depend on it now, does it? It doesn't, but then it doesn't support cpu hotplug, OVMF(SMM) needs to cooperate with QEMU "and" ACPI tables to perform the task and using the same interface/code path between all involved parties makes the task easier with the least amount of duplicated interfaces and more robust. Re-implementing alternative interface for firmware (fwcfg or what not) would work as well, but it's only question of time when ACPI and this new interface disagree on how world works and process falls apart. > If answers to all of the above is yes, then I don't really like it: it > is better to keep all paravirt stuff in one place, namely in fw cfg. Lets discuss, what cpu hotplug fwcfg interface could look like in [PATCH 3/4] hw/i386: add facility to expose CPU topology over fw-cfg mail thread and clarify (dis)likes with concrete reasons. So far I managed to convince myself that we ought to reuse and extend current CPU hotplug interface with firmware features, to endup with consolidated cpu hotplug process without introducing duplicate ABIs, but I could be wrong so lets see if fwcfg will be the better approach. > > CC: Laszlo Ersek <lersek@redhat.com> > > CC: Eduardo Habkost <ehabkost@redhat.com> > > CC: "Michael S. Tsirkin" <mst@redhat.com> > > CC: Gerd Hoffmann <kraxel@redhat.com> > > CC: Paolo Bonzini <pbonzini@redhat.com> > > CC: Philippe Mathieu-Daudé <philmd@redhat.com> > > CC: Richard Henderson <rth@twiddle.net> > > > > Igor Mammedov (3): > > acpi: cpuhp: fix 'Command data' description is spec > > acpi: cpuhp: add typical usecases into spec > > acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command > > > > docs/specs/acpi_cpu_hotplug.txt | 37 ++++++++++++++++++++++++++++++--- > > hw/acpi/cpu.c | 15 +++++++++++++ > > hw/acpi/trace-events | 1 + > > 3 files changed, 50 insertions(+), 3 deletions(-) > > > > -- > > 2.18.1 > ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface 2019-10-10 13:39 ` Igor Mammedov @ 2019-10-10 13:59 ` Michael S. Tsirkin 2019-10-10 15:57 ` Igor Mammedov 2019-10-10 14:16 ` Eduardo Habkost 1 sibling, 1 reply; 43+ messages in thread From: Michael S. Tsirkin @ 2019-10-10 13:59 UTC (permalink / raw) To: Igor Mammedov Cc: Eduardo Habkost, Laszlo Ersek, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On Thu, Oct 10, 2019 at 03:39:12PM +0200, Igor Mammedov wrote: > On Thu, 10 Oct 2019 05:56:55 -0400 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, Oct 09, 2019 at 09:22:49AM -0400, Igor Mammedov wrote: > > > As an alternative to passing to firmware topology info via new fwcfg files > > > so it could recreate APIC IDs based on it and order CPUs are enumerated, > > > > > > extend CPU hotplug interface to return APIC ID as response to the new command > > > CPHP_GET_CPU_ID_CMD. > > > > One big piece missing here is motivation: > I thought the only willing reader was Laszlo (who is aware of context) > so I skipped on details and confused others :/ > > > Who's going to use this interface? > In current state it's for firmware, since ACPI tables can cheat > by having APIC IDs statically built in. > > If we were creating CPU objects in ACPI dynamically > we would be using this command as well. I'm not sure how it's even possible to create devices dynamically. Well I guess it's possible with LoadTable. Is this what you had in mind? > It would save > us quite a bit space in ACPI blob but it would be a pain > to debug and diagnose problems in ACPI tables, so I'd rather > stay with static CPU descriptions in ACPI tables for the sake > of maintenance. > > So far CPU hotplug was used by the ACPI, so we didn't > > really commit to a fixed interface too strongly. > > > > Is this a replacement to Laszlo's fw cfg interface? > > If yes is the idea that OVMF going to depend on CPU hotplug directly then? > > It does not depend on it now, does it? > It doesn't, but then it doesn't support cpu hotplug, > OVMF(SMM) needs to cooperate with QEMU "and" ACPI tables to perform > the task and using the same interface/code path between all involved > parties makes the task easier with the least amount of duplicated > interfaces and more robust. > > Re-implementing alternative interface for firmware (fwcfg or what not) > would work as well, but it's only question of time when ACPI and > this new interface disagree on how world works and process falls > apart. Then we should consider switching acpi to use fw cfg. Or build another interface that can scale. > > If answers to all of the above is yes, then I don't really like it: it > > is better to keep all paravirt stuff in one place, namely in fw cfg. > Lets discuss, what cpu hotplug fwcfg interface could look like in > [PATCH 3/4] hw/i386: add facility to expose CPU topology over fw-cfg > mail thread and clarify (dis)likes with concrete reasons. > > So far I managed to convince myself that we ought to reuse > and extend current CPU hotplug interface with firmware features, > to endup with consolidated cpu hotplug process without > introducing duplicate ABIs, but I could be wrong so > lets see if fwcfg will be the better approach. > > > > > CC: Laszlo Ersek <lersek@redhat.com> > > > CC: Eduardo Habkost <ehabkost@redhat.com> > > > CC: "Michael S. Tsirkin" <mst@redhat.com> > > > CC: Gerd Hoffmann <kraxel@redhat.com> > > > CC: Paolo Bonzini <pbonzini@redhat.com> > > > CC: Philippe Mathieu-Daudé <philmd@redhat.com> > > > CC: Richard Henderson <rth@twiddle.net> > > > > > > Igor Mammedov (3): > > > acpi: cpuhp: fix 'Command data' description is spec > > > acpi: cpuhp: add typical usecases into spec > > > acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command > > > > > > docs/specs/acpi_cpu_hotplug.txt | 37 ++++++++++++++++++++++++++++++--- > > > hw/acpi/cpu.c | 15 +++++++++++++ > > > hw/acpi/trace-events | 1 + > > > 3 files changed, 50 insertions(+), 3 deletions(-) > > > > > > -- > > > 2.18.1 > > ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface 2019-10-10 13:59 ` Michael S. Tsirkin @ 2019-10-10 15:57 ` Igor Mammedov 2019-10-10 18:15 ` Michael S. Tsirkin ` (2 more replies) 0 siblings, 3 replies; 43+ messages in thread From: Igor Mammedov @ 2019-10-10 15:57 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Eduardo Habkost, Laszlo Ersek, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On Thu, 10 Oct 2019 09:59:42 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, Oct 10, 2019 at 03:39:12PM +0200, Igor Mammedov wrote: > > On Thu, 10 Oct 2019 05:56:55 -0400 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Wed, Oct 09, 2019 at 09:22:49AM -0400, Igor Mammedov wrote: > > > > As an alternative to passing to firmware topology info via new fwcfg files > > > > so it could recreate APIC IDs based on it and order CPUs are enumerated, > > > > > > > > extend CPU hotplug interface to return APIC ID as response to the new command > > > > CPHP_GET_CPU_ID_CMD. > > > > > > One big piece missing here is motivation: > > I thought the only willing reader was Laszlo (who is aware of context) > > so I skipped on details and confused others :/ > > > > > Who's going to use this interface? > > In current state it's for firmware, since ACPI tables can cheat > > by having APIC IDs statically built in. > > > > If we were creating CPU objects in ACPI dynamically > > we would be using this command as well. > > I'm not sure how it's even possible to create devices dynamically. Well > I guess it's possible with LoadTable. Is this what you had in > mind? Yep. I even played this shiny toy and I can say it's very tempting one. On the other side, even problem of legacy OSes not working with it aside, it's hard to debug and reproduce compared to static tables. So from maintaining pov I dislike it enough to be against it. > > It would save > > us quite a bit space in ACPI blob but it would be a pain > > to debug and diagnose problems in ACPI tables, so I'd rather > > stay with static CPU descriptions in ACPI tables for the sake > > of maintenance. > > > So far CPU hotplug was used by the ACPI, so we didn't > > > really commit to a fixed interface too strongly. > > > > > > Is this a replacement to Laszlo's fw cfg interface? > > > If yes is the idea that OVMF going to depend on CPU hotplug directly then? > > > It does not depend on it now, does it? > > It doesn't, but then it doesn't support cpu hotplug, > > OVMF(SMM) needs to cooperate with QEMU "and" ACPI tables to perform > > the task and using the same interface/code path between all involved > > parties makes the task easier with the least amount of duplicated > > interfaces and more robust. > > > > Re-implementing alternative interface for firmware (fwcfg or what not) > > would work as well, but it's only question of time when ACPI and > > this new interface disagree on how world works and process falls > > apart. > > Then we should consider switching acpi to use fw cfg. > Or build another interface that can scale. Could be an option, it would be a pain to write a driver in AML for fwcfg access though (I've looked at possibility to access fwcfg from AML about a year ago and gave up. I'm definitely not volunteering for the second attempt and can't even give an estimate it it's viable approach). But what scaling issue you are talking about, exactly? With current CPU hotplug interface we can handle upto UNIT32_MAX cpus, and extend interface without need to increase IO window we are using now. Granted IO access it not fastest compared to fwcfg in DMA mode, but we already doing stop machine when switching to SMM which is orders of magnitude slower. Consensus was to compromise on speed of CPU hotplug versus more complex and more problematic unicast SMM mode in OVMF (can't find a particular email but we have discussed it with Laszlo already, when I considered ways to optimize hotplug speed) > > > If answers to all of the above is yes, then I don't really like it: it > > > is better to keep all paravirt stuff in one place, namely in fw cfg. > > Lets discuss, what cpu hotplug fwcfg interface could look like in > > [PATCH 3/4] hw/i386: add facility to expose CPU topology over fw-cfg > > mail thread and clarify (dis)likes with concrete reasons. > > > > So far I managed to convince myself that we ought to reuse > > and extend current CPU hotplug interface with firmware features, > > to endup with consolidated cpu hotplug process without > > introducing duplicate ABIs, but I could be wrong so > > lets see if fwcfg will be the better approach. > > > > > > > > CC: Laszlo Ersek <lersek@redhat.com> > > > > CC: Eduardo Habkost <ehabkost@redhat.com> > > > > CC: "Michael S. Tsirkin" <mst@redhat.com> > > > > CC: Gerd Hoffmann <kraxel@redhat.com> > > > > CC: Paolo Bonzini <pbonzini@redhat.com> > > > > CC: Philippe Mathieu-Daudé <philmd@redhat.com> > > > > CC: Richard Henderson <rth@twiddle.net> > > > > > > > > Igor Mammedov (3): > > > > acpi: cpuhp: fix 'Command data' description is spec > > > > acpi: cpuhp: add typical usecases into spec > > > > acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command > > > > > > > > docs/specs/acpi_cpu_hotplug.txt | 37 ++++++++++++++++++++++++++++++--- > > > > hw/acpi/cpu.c | 15 +++++++++++++ > > > > hw/acpi/trace-events | 1 + > > > > 3 files changed, 50 insertions(+), 3 deletions(-) > > > > > > > > -- > > > > 2.18.1 > > > > ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface 2019-10-10 15:57 ` Igor Mammedov @ 2019-10-10 18:15 ` Michael S. Tsirkin 2019-10-11 7:41 ` Laszlo Ersek 2019-10-10 19:20 ` Eduardo Habkost 2019-10-11 6:54 ` Laszlo Ersek 2 siblings, 1 reply; 43+ messages in thread From: Michael S. Tsirkin @ 2019-10-10 18:15 UTC (permalink / raw) To: Igor Mammedov Cc: Eduardo Habkost, Laszlo Ersek, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On Thu, Oct 10, 2019 at 05:57:54PM +0200, Igor Mammedov wrote: > > Then we should consider switching acpi to use fw cfg. > > Or build another interface that can scale. > > Could be an option, it would be a pain to write a driver in AML for fwcfg access though > (I've looked at possibility to access fwcfg from AML about a year ago and gave up. > I'm definitely not volunteering for the second attempt and can't even give an estimate > it it's viable approach). > > But what scaling issue you are talking about, exactly? Just this: each new thing we add is an ad-hoc data structure with manually maintained backwards compatibility and no built-in discovery. fw cfg has built-in discovery and we've finally managed to handle compatibility reasonably well. PV is already very problematic. Spreading PV code all over the place like this is a very bad idea. For you CPU hotplug is something that you keep in mind first of all, but someone bringing up a new platform already has a steep hill to climb. Adding tons of custom firmware is not helping things. -- MST ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface 2019-10-10 18:15 ` Michael S. Tsirkin @ 2019-10-11 7:41 ` Laszlo Ersek 0 siblings, 0 replies; 43+ messages in thread From: Laszlo Ersek @ 2019-10-11 7:41 UTC (permalink / raw) To: Michael S. Tsirkin, Igor Mammedov Cc: Eduardo Habkost, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On 10/10/19 20:15, Michael S. Tsirkin wrote: > On Thu, Oct 10, 2019 at 05:57:54PM +0200, Igor Mammedov wrote: >>> Then we should consider switching acpi to use fw cfg. >>> Or build another interface that can scale. >> >> Could be an option, it would be a pain to write a driver in AML for fwcfg access though >> (I've looked at possibility to access fwcfg from AML about a year ago and gave up. >> I'm definitely not volunteering for the second attempt and can't even give an estimate >> it it's viable approach). >> >> But what scaling issue you are talking about, exactly? > > Just this: each new thing we add is an ad-hoc data structure with > manually maintained backwards compatibility and no built-in discovery. > > fw cfg has built-in discovery and we've finally managed to > handle compatibility reasonably well. > > PV is already very problematic. Spreading PV code all over the place > like this is a very bad idea. For you CPU hotplug is something that you > keep in mind first of all, but someone bringing up a new platform > already has a steep hill to climb. Adding tons of custom firmware is > not helping things. > - Custom firmware is unfortunately unavoidable in this case. SMM is a privilege barrier between the firmware and the OS. The firmware needs to care because to OS could use a hotplugged CPU to attack SMM, unless hardware and firmware co-operate to prevent that. This whole ordeal aims to prevent such an attack. If you remove SMM from the picture (build OVMF without it), there is no such privilege barrier between fw and OS, the firmware doesn't need to know anything about VCPU hotplug events, and hotplug already works. - Custom hardware is also expected. On physical platforms, there is a dedicated BMC / RAS thingy that e.g. provides the APIC-ID to firmware, when a CPU is hotplugged. One factor that makes this feature difficult is that edk2 does not contain any code driving such hardware; it only contains firmware code for *consuming* what the BMC / RAS thingy produces. We're trying to work backwards from that. We could implement a brand new hotplug controller device model, but it would be entirely PV (= it would be our own design). Reusing the CPU hotplug register block isn't worse, in that regard. This is my understanding anyway. - Using fw_cfg *DMA* in an SMI handler (at OS runtime) could be problematic. Especially if you consider SEV guests. For DMA transfers, memory has to be decrypted and encrypted, which involves page table manipulation. In the firmware, this is handled with a SEV-specific EDKII_IOMMU_PROTOCOL implementation. The fw_cfg library (transparently to the caller) utilizes the IOMMU protocol, for dealing with DMA in SEV guests. Such protocols are boot-time only; they are not runtime protocols, let alone SMM protocols. They are not available to SMI handlers. Raw IO ports are extremely attractive in this regard: they don't depend on page tables (SMM has its own set of page tables), they just work under SEV out of the box. (The only exception is [REP] INS*/OUTS*, as those depend on memory, not just registers, but no such instructions are expected for the hotplug register block.) - IO-port based fw_cfg might be suitable for the hotplug SMI handler, yes. Assuming it's really only the firmware that needs information from QEMU in the hotplug SMI handler, and not the other way around. (IO-port based fw_cfg doesn't support writes.) Now, when considering IO-port based fw_cfg in the SMI handler, we should at least mention the risk of interfering with the Linux guest's own fw_cfg driver. I've *never* been happy with anything non-firmware accessing fw_cfg (it says "*firmware* config" in the name!), so I wouldn't care much in practice. It's just that we should be aware that there is a chance for entanglement. (The SMI handler cannot restore the previous fw_cfg state, when it finishes up.) At least on a theoretical level, the ACPI CPU hotplug register block is nicer in this regard: the OS doesn't know about it at all (it is accessed only through QEMU-generated AML code), and the plan is for the same AML code to closely coordinate with the firmware. Namely, the AML would raise the hotplug SMI. Thanks Laszlo ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface 2019-10-10 15:57 ` Igor Mammedov 2019-10-10 18:15 ` Michael S. Tsirkin @ 2019-10-10 19:20 ` Eduardo Habkost 2019-10-11 8:01 ` Laszlo Ersek 2019-10-11 10:47 ` Igor Mammedov 2019-10-11 6:54 ` Laszlo Ersek 2 siblings, 2 replies; 43+ messages in thread From: Eduardo Habkost @ 2019-10-10 19:20 UTC (permalink / raw) To: Igor Mammedov Cc: Michael S. Tsirkin, Laszlo Ersek, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On Thu, Oct 10, 2019 at 05:57:54PM +0200, Igor Mammedov wrote: > On Thu, 10 Oct 2019 09:59:42 -0400 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Thu, Oct 10, 2019 at 03:39:12PM +0200, Igor Mammedov wrote: > > > On Thu, 10 Oct 2019 05:56:55 -0400 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Wed, Oct 09, 2019 at 09:22:49AM -0400, Igor Mammedov wrote: > > > > > As an alternative to passing to firmware topology info via new fwcfg files > > > > > so it could recreate APIC IDs based on it and order CPUs are enumerated, > > > > > > > > > > extend CPU hotplug interface to return APIC ID as response to the new command > > > > > CPHP_GET_CPU_ID_CMD. > > > > > > > > One big piece missing here is motivation: > > > I thought the only willing reader was Laszlo (who is aware of context) > > > so I skipped on details and confused others :/ > > > > > > > Who's going to use this interface? > > > In current state it's for firmware, since ACPI tables can cheat > > > by having APIC IDs statically built in. > > > > > > If we were creating CPU objects in ACPI dynamically > > > we would be using this command as well. > > > > I'm not sure how it's even possible to create devices dynamically. Well > > I guess it's possible with LoadTable. Is this what you had in > > mind? > > Yep. I even played this shiny toy and I can say it's very tempting one. > On the other side, even problem of legacy OSes not working with it aside, > it's hard to debug and reproduce compared to static tables. > So from maintaining pov I dislike it enough to be against it. > > > > > It would save > > > us quite a bit space in ACPI blob but it would be a pain > > > to debug and diagnose problems in ACPI tables, so I'd rather > > > stay with static CPU descriptions in ACPI tables for the sake > > > of maintenance. > > > > So far CPU hotplug was used by the ACPI, so we didn't > > > > really commit to a fixed interface too strongly. > > > > > > > > Is this a replacement to Laszlo's fw cfg interface? > > > > If yes is the idea that OVMF going to depend on CPU hotplug directly then? > > > > It does not depend on it now, does it? > > > It doesn't, but then it doesn't support cpu hotplug, > > > OVMF(SMM) needs to cooperate with QEMU "and" ACPI tables to perform > > > the task and using the same interface/code path between all involved > > > parties makes the task easier with the least amount of duplicated > > > interfaces and more robust. > > > > > > Re-implementing alternative interface for firmware (fwcfg or what not) > > > would work as well, but it's only question of time when ACPI and > > > this new interface disagree on how world works and process falls > > > apart. > > > > Then we should consider switching acpi to use fw cfg. > > Or build another interface that can scale. > > Could be an option, it would be a pain to write a driver in AML for fwcfg access though > (I've looked at possibility to access fwcfg from AML about a year ago and gave up. > I'm definitely not volunteering for the second attempt and can't even give an estimate > it it's viable approach). > > But what scaling issue you are talking about, exactly? > With current CPU hotplug interface we can handle upto UNIT32_MAX cpus, and extend > interface without need to increase IO window we are using now. > > Granted IO access it not fastest compared to fwcfg in DMA mode, but we already > doing stop machine when switching to SMM which is orders of magnitude slower. > Consensus was to compromise on speed of CPU hotplug versus more complex and more > problematic unicast SMM mode in OVMF (can't find a particular email but we have discussed > it with Laszlo already, when I considered ways to optimize hotplug speed) If we were designing the interface from the ground up, I would agree with Michael. But I don't see why we would reimplement everything from scratch now, if just providing the cpu_selector => cpu_hardware_id mapping to firmware is enough to make the existing interface work. If somebody is really unhappy with the current interface and wants to implement a new purely fw_cfg-based one (and write the corresponding ACPI code), they would be welcome. I just don't see why we should spend our time doing that now. -- Eduardo ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface 2019-10-10 19:20 ` Eduardo Habkost @ 2019-10-11 8:01 ` Laszlo Ersek 2019-10-11 13:00 ` Michael S. Tsirkin 2019-10-11 10:47 ` Igor Mammedov 1 sibling, 1 reply; 43+ messages in thread From: Laszlo Ersek @ 2019-10-11 8:01 UTC (permalink / raw) To: Eduardo Habkost, Igor Mammedov Cc: Michael S. Tsirkin, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On 10/10/19 21:20, Eduardo Habkost wrote: > On Thu, Oct 10, 2019 at 05:57:54PM +0200, Igor Mammedov wrote: >> On Thu, 10 Oct 2019 09:59:42 -0400 >> "Michael S. Tsirkin" <mst@redhat.com> wrote: >> >>> On Thu, Oct 10, 2019 at 03:39:12PM +0200, Igor Mammedov wrote: >>>> On Thu, 10 Oct 2019 05:56:55 -0400 >>>> "Michael S. Tsirkin" <mst@redhat.com> wrote: >>>> >>>>> On Wed, Oct 09, 2019 at 09:22:49AM -0400, Igor Mammedov wrote: >>>>>> As an alternative to passing to firmware topology info via new fwcfg files >>>>>> so it could recreate APIC IDs based on it and order CPUs are enumerated, >>>>>> >>>>>> extend CPU hotplug interface to return APIC ID as response to the new command >>>>>> CPHP_GET_CPU_ID_CMD. >>>>> >>>>> One big piece missing here is motivation: >>>> I thought the only willing reader was Laszlo (who is aware of context) >>>> so I skipped on details and confused others :/ >>>> >>>>> Who's going to use this interface? >>>> In current state it's for firmware, since ACPI tables can cheat >>>> by having APIC IDs statically built in. >>>> >>>> If we were creating CPU objects in ACPI dynamically >>>> we would be using this command as well. >>> >>> I'm not sure how it's even possible to create devices dynamically. Well >>> I guess it's possible with LoadTable. Is this what you had in >>> mind? >> >> Yep. I even played this shiny toy and I can say it's very tempting one. >> On the other side, even problem of legacy OSes not working with it aside, >> it's hard to debug and reproduce compared to static tables. >> So from maintaining pov I dislike it enough to be against it. >> >> >>>> It would save >>>> us quite a bit space in ACPI blob but it would be a pain >>>> to debug and diagnose problems in ACPI tables, so I'd rather >>>> stay with static CPU descriptions in ACPI tables for the sake >>>> of maintenance. >>>>> So far CPU hotplug was used by the ACPI, so we didn't >>>>> really commit to a fixed interface too strongly. >>>>> >>>>> Is this a replacement to Laszlo's fw cfg interface? >>>>> If yes is the idea that OVMF going to depend on CPU hotplug directly then? >>>>> It does not depend on it now, does it? >>>> It doesn't, but then it doesn't support cpu hotplug, >>>> OVMF(SMM) needs to cooperate with QEMU "and" ACPI tables to perform >>>> the task and using the same interface/code path between all involved >>>> parties makes the task easier with the least amount of duplicated >>>> interfaces and more robust. >>>> >>>> Re-implementing alternative interface for firmware (fwcfg or what not) >>>> would work as well, but it's only question of time when ACPI and >>>> this new interface disagree on how world works and process falls >>>> apart. >>> >>> Then we should consider switching acpi to use fw cfg. >>> Or build another interface that can scale. >> >> Could be an option, it would be a pain to write a driver in AML for fwcfg access though >> (I've looked at possibility to access fwcfg from AML about a year ago and gave up. >> I'm definitely not volunteering for the second attempt and can't even give an estimate >> it it's viable approach). >> >> But what scaling issue you are talking about, exactly? >> With current CPU hotplug interface we can handle upto UNIT32_MAX cpus, and extend >> interface without need to increase IO window we are using now. >> >> Granted IO access it not fastest compared to fwcfg in DMA mode, but we already >> doing stop machine when switching to SMM which is orders of magnitude slower. >> Consensus was to compromise on speed of CPU hotplug versus more complex and more >> problematic unicast SMM mode in OVMF (can't find a particular email but we have discussed >> it with Laszlo already, when I considered ways to optimize hotplug speed) > > If we were designing the interface from the ground up, I would > agree with Michael. But I don't see why we would reimplement > everything from scratch now, if just providing the > cpu_selector => cpu_hardware_id mapping to firmware is enough to > make the existing interface work. > > If somebody is really unhappy with the current interface and > wants to implement a new purely fw_cfg-based one (and write the > corresponding ACPI code), they would be welcome. Let me re-iterate the difficulties quickly: - DMA-based fw_cfg is troublesome in SEV guests (do you want to mess with page table entries in AML methods? or pre-allocate an always decrypted opregion? how large?) - IO port based fw_cfg does not support writes (and I reckon that, when the *OS* handles a hotplug event, it does have to talk back to QEMU) - the CPU hotplug AML would have to arbitrate with Linux's own fw_cfg driver (which exposes fw_cfg files to userspace, yay! /s) In the phys world, CPU hotplug takes dedicated RAS hardware. Shoehorning CPU hotplug into *firmware* config, when in two use cases [*], the firmware shouldn't even know about CPU hotplug, feels messy. [*] being (a) SeaBIOS, and (b) OVMF built without SMM > I just don't see why we should spend our time doing that now. I have to agree, we're already spread thin. ... I must admit: I didn't expect this, but now I've grown to *prefer* the CPU hotplug register block! Laszlo ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface 2019-10-11 8:01 ` Laszlo Ersek @ 2019-10-11 13:00 ` Michael S. Tsirkin 2019-10-11 16:13 ` Laszlo Ersek 0 siblings, 1 reply; 43+ messages in thread From: Michael S. Tsirkin @ 2019-10-11 13:00 UTC (permalink / raw) To: Laszlo Ersek Cc: Eduardo Habkost, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Igor Mammedov, Philippe Mathieu-Daudé, Richard Henderson On Fri, Oct 11, 2019 at 10:01:42AM +0200, Laszlo Ersek wrote: > On 10/10/19 21:20, Eduardo Habkost wrote: > > On Thu, Oct 10, 2019 at 05:57:54PM +0200, Igor Mammedov wrote: > >> On Thu, 10 Oct 2019 09:59:42 -0400 > >> "Michael S. Tsirkin" <mst@redhat.com> wrote: > >> > >>> On Thu, Oct 10, 2019 at 03:39:12PM +0200, Igor Mammedov wrote: > >>>> On Thu, 10 Oct 2019 05:56:55 -0400 > >>>> "Michael S. Tsirkin" <mst@redhat.com> wrote: > >>>> > >>>>> On Wed, Oct 09, 2019 at 09:22:49AM -0400, Igor Mammedov wrote: > >>>>>> As an alternative to passing to firmware topology info via new fwcfg files > >>>>>> so it could recreate APIC IDs based on it and order CPUs are enumerated, > >>>>>> > >>>>>> extend CPU hotplug interface to return APIC ID as response to the new command > >>>>>> CPHP_GET_CPU_ID_CMD. > >>>>> > >>>>> One big piece missing here is motivation: > >>>> I thought the only willing reader was Laszlo (who is aware of context) > >>>> so I skipped on details and confused others :/ > >>>> > >>>>> Who's going to use this interface? > >>>> In current state it's for firmware, since ACPI tables can cheat > >>>> by having APIC IDs statically built in. > >>>> > >>>> If we were creating CPU objects in ACPI dynamically > >>>> we would be using this command as well. > >>> > >>> I'm not sure how it's even possible to create devices dynamically. Well > >>> I guess it's possible with LoadTable. Is this what you had in > >>> mind? > >> > >> Yep. I even played this shiny toy and I can say it's very tempting one. > >> On the other side, even problem of legacy OSes not working with it aside, > >> it's hard to debug and reproduce compared to static tables. > >> So from maintaining pov I dislike it enough to be against it. > >> > >> > >>>> It would save > >>>> us quite a bit space in ACPI blob but it would be a pain > >>>> to debug and diagnose problems in ACPI tables, so I'd rather > >>>> stay with static CPU descriptions in ACPI tables for the sake > >>>> of maintenance. > >>>>> So far CPU hotplug was used by the ACPI, so we didn't > >>>>> really commit to a fixed interface too strongly. > >>>>> > >>>>> Is this a replacement to Laszlo's fw cfg interface? > >>>>> If yes is the idea that OVMF going to depend on CPU hotplug directly then? > >>>>> It does not depend on it now, does it? > >>>> It doesn't, but then it doesn't support cpu hotplug, > >>>> OVMF(SMM) needs to cooperate with QEMU "and" ACPI tables to perform > >>>> the task and using the same interface/code path between all involved > >>>> parties makes the task easier with the least amount of duplicated > >>>> interfaces and more robust. > >>>> > >>>> Re-implementing alternative interface for firmware (fwcfg or what not) > >>>> would work as well, but it's only question of time when ACPI and > >>>> this new interface disagree on how world works and process falls > >>>> apart. > >>> > >>> Then we should consider switching acpi to use fw cfg. > >>> Or build another interface that can scale. > >> > >> Could be an option, it would be a pain to write a driver in AML for fwcfg access though > >> (I've looked at possibility to access fwcfg from AML about a year ago and gave up. > >> I'm definitely not volunteering for the second attempt and can't even give an estimate > >> it it's viable approach). > >> > >> But what scaling issue you are talking about, exactly? > >> With current CPU hotplug interface we can handle upto UNIT32_MAX cpus, and extend > >> interface without need to increase IO window we are using now. > >> > >> Granted IO access it not fastest compared to fwcfg in DMA mode, but we already > >> doing stop machine when switching to SMM which is orders of magnitude slower. > >> Consensus was to compromise on speed of CPU hotplug versus more complex and more > >> problematic unicast SMM mode in OVMF (can't find a particular email but we have discussed > >> it with Laszlo already, when I considered ways to optimize hotplug speed) > > > > If we were designing the interface from the ground up, I would > > agree with Michael. But I don't see why we would reimplement > > everything from scratch now, if just providing the > > cpu_selector => cpu_hardware_id mapping to firmware is enough to > > make the existing interface work. > > > > If somebody is really unhappy with the current interface and > > wants to implement a new purely fw_cfg-based one (and write the > > corresponding ACPI code), they would be welcome. > > Let me re-iterate the difficulties quickly: > > - DMA-based fw_cfg is troublesome in SEV guests (do you want to mess > with page table entries in AML methods? or pre-allocate an always > decrypted opregion? how large?) > > - IO port based fw_cfg does not support writes (and I reckon that, when > the *OS* handles a hotplug event, it does have to talk back to QEMU) > > - the CPU hotplug AML would have to arbitrate with Linux's own fw_cfg > driver (which exposes fw_cfg files to userspace, yay! /s) > > In the phys world, CPU hotplug takes dedicated RAS hardware. Shoehorning > CPU hotplug into *firmware* config, when in two use cases [*], the > firmware shouldn't even know about CPU hotplug, feels messy. > > [*] being (a) SeaBIOS, and (b) OVMF built without SMM I agree. So ACPI should use a dedicated interface. > > I just don't see why we should spend our time doing that now. > > I have to agree, we're already spread thin. > > ... I must admit: I didn't expect this, but now I've grown to *prefer* > the CPU hotplug register block! > > Laszlo OK, send an ack then. -- MST ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface 2019-10-11 13:00 ` Michael S. Tsirkin @ 2019-10-11 16:13 ` Laszlo Ersek 0 siblings, 0 replies; 43+ messages in thread From: Laszlo Ersek @ 2019-10-11 16:13 UTC (permalink / raw) To: Michael S. Tsirkin, Igor Mammedov Cc: Eduardo Habkost, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On 10/11/19 15:00, Michael S. Tsirkin wrote: > On Fri, Oct 11, 2019 at 10:01:42AM +0200, Laszlo Ersek wrote: [...] >> ... I must admit: I didn't expect this, but now I've grown to *prefer* >> the CPU hotplug register block! > > OK, send an ack then. This RFC isn't mature enough for an ACK, but I like the direction, and I've given ample feedback. I'm looking forward to v1. When Igor posts v1, I plan to first write firmware code for deriving "max_cpus" through the register block. For that, I only need the docs to reflect reality *closely* (I've posted my own suggestions for the docs); plus "max_cpus" is something I can put to use immediately. Regarding the CPHP_GET_CPU_ID_CMD feature, I'll have to test that from within the SMI handler. Thus, until my "final" ACK, it's going to take a while. I'm OK if the QEMU patch set remains pending on the list meanwhile. Igor -- can you please answer my questions in this thread? (Part of my feedback has been questions.) Thanks! Laszlo ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface 2019-10-10 19:20 ` Eduardo Habkost 2019-10-11 8:01 ` Laszlo Ersek @ 2019-10-11 10:47 ` Igor Mammedov 1 sibling, 0 replies; 43+ messages in thread From: Igor Mammedov @ 2019-10-11 10:47 UTC (permalink / raw) To: Eduardo Habkost Cc: Michael S. Tsirkin, Laszlo Ersek, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On Thu, 10 Oct 2019 16:20:39 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Oct 10, 2019 at 05:57:54PM +0200, Igor Mammedov wrote: > > On Thu, 10 Oct 2019 09:59:42 -0400 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Thu, Oct 10, 2019 at 03:39:12PM +0200, Igor Mammedov wrote: > > > > On Thu, 10 Oct 2019 05:56:55 -0400 > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > On Wed, Oct 09, 2019 at 09:22:49AM -0400, Igor Mammedov wrote: > > > > > > As an alternative to passing to firmware topology info via new fwcfg files > > > > > > so it could recreate APIC IDs based on it and order CPUs are enumerated, > > > > > > > > > > > > extend CPU hotplug interface to return APIC ID as response to the new command > > > > > > CPHP_GET_CPU_ID_CMD. > > > > > > > > > > One big piece missing here is motivation: > > > > I thought the only willing reader was Laszlo (who is aware of context) > > > > so I skipped on details and confused others :/ > > > > > > > > > Who's going to use this interface? > > > > In current state it's for firmware, since ACPI tables can cheat > > > > by having APIC IDs statically built in. > > > > > > > > If we were creating CPU objects in ACPI dynamically > > > > we would be using this command as well. > > > > > > I'm not sure how it's even possible to create devices dynamically. Well > > > I guess it's possible with LoadTable. Is this what you had in > > > mind? > > > > Yep. I even played this shiny toy and I can say it's very tempting one. > > On the other side, even problem of legacy OSes not working with it aside, > > it's hard to debug and reproduce compared to static tables. > > So from maintaining pov I dislike it enough to be against it. > > > > > > > > It would save > > > > us quite a bit space in ACPI blob but it would be a pain > > > > to debug and diagnose problems in ACPI tables, so I'd rather > > > > stay with static CPU descriptions in ACPI tables for the sake > > > > of maintenance. > > > > > So far CPU hotplug was used by the ACPI, so we didn't > > > > > really commit to a fixed interface too strongly. > > > > > > > > > > Is this a replacement to Laszlo's fw cfg interface? > > > > > If yes is the idea that OVMF going to depend on CPU hotplug directly then? > > > > > It does not depend on it now, does it? > > > > It doesn't, but then it doesn't support cpu hotplug, > > > > OVMF(SMM) needs to cooperate with QEMU "and" ACPI tables to perform > > > > the task and using the same interface/code path between all involved > > > > parties makes the task easier with the least amount of duplicated > > > > interfaces and more robust. > > > > > > > > Re-implementing alternative interface for firmware (fwcfg or what not) > > > > would work as well, but it's only question of time when ACPI and > > > > this new interface disagree on how world works and process falls > > > > apart. > > > > > > Then we should consider switching acpi to use fw cfg. > > > Or build another interface that can scale. > > > > Could be an option, it would be a pain to write a driver in AML for fwcfg access though > > (I've looked at possibility to access fwcfg from AML about a year ago and gave up. > > I'm definitely not volunteering for the second attempt and can't even give an estimate > > it it's viable approach). > > > > But what scaling issue you are talking about, exactly? > > With current CPU hotplug interface we can handle upto UNIT32_MAX cpus, and extend > > interface without need to increase IO window we are using now. > > > > Granted IO access it not fastest compared to fwcfg in DMA mode, but we already > > doing stop machine when switching to SMM which is orders of magnitude slower. > > Consensus was to compromise on speed of CPU hotplug versus more complex and more > > problematic unicast SMM mode in OVMF (can't find a particular email but we have discussed > > it with Laszlo already, when I considered ways to optimize hotplug speed) > > If we were designing the interface from the ground up, I would > agree with Michael. But I don't see why we would reimplement > everything from scratch now, if just providing the > cpu_selector => cpu_hardware_id mapping to firmware is enough to > make the existing interface work. > > If somebody is really unhappy with the current interface and > wants to implement a new purely fw_cfg-based one (and write the > corresponding ACPI code), they would be welcome. I just don't > see why we should spend our time doing that now. Right, we can give fwcfg a shot next time we try to allocate new register block for a new PV interface, assuming it suits interface requirements. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface 2019-10-10 15:57 ` Igor Mammedov 2019-10-10 18:15 ` Michael S. Tsirkin 2019-10-10 19:20 ` Eduardo Habkost @ 2019-10-11 6:54 ` Laszlo Ersek 2 siblings, 0 replies; 43+ messages in thread From: Laszlo Ersek @ 2019-10-11 6:54 UTC (permalink / raw) To: Igor Mammedov, Michael S. Tsirkin Cc: Eduardo Habkost, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On 10/10/19 17:57, Igor Mammedov wrote: > On Thu, 10 Oct 2019 09:59:42 -0400 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > >> On Thu, Oct 10, 2019 at 03:39:12PM +0200, Igor Mammedov wrote: >>> On Thu, 10 Oct 2019 05:56:55 -0400 >>> "Michael S. Tsirkin" <mst@redhat.com> wrote: >>> >>>> On Wed, Oct 09, 2019 at 09:22:49AM -0400, Igor Mammedov wrote: >>>>> As an alternative to passing to firmware topology info via new fwcfg files >>>>> so it could recreate APIC IDs based on it and order CPUs are enumerated, >>>>> >>>>> extend CPU hotplug interface to return APIC ID as response to the new command >>>>> CPHP_GET_CPU_ID_CMD. >>>> >>>> One big piece missing here is motivation: >>> I thought the only willing reader was Laszlo (who is aware of context) >>> so I skipped on details and confused others :/ >>> >>>> Who's going to use this interface? >>> In current state it's for firmware, since ACPI tables can cheat >>> by having APIC IDs statically built in. >>> >>> If we were creating CPU objects in ACPI dynamically >>> we would be using this command as well. >> >> I'm not sure how it's even possible to create devices dynamically. Well >> I guess it's possible with LoadTable. Is this what you had in >> mind? > > Yep. I even played this shiny toy and I can say it's very tempting one. > On the other side, even problem of legacy OSes not working with it aside, > it's hard to debug and reproduce compared to static tables. > So from maintaining pov I dislike it enough to be against it. > > >>> It would save >>> us quite a bit space in ACPI blob but it would be a pain >>> to debug and diagnose problems in ACPI tables, so I'd rather >>> stay with static CPU descriptions in ACPI tables for the sake >>> of maintenance. >>>> So far CPU hotplug was used by the ACPI, so we didn't >>>> really commit to a fixed interface too strongly. >>>> >>>> Is this a replacement to Laszlo's fw cfg interface? >>>> If yes is the idea that OVMF going to depend on CPU hotplug directly then? >>>> It does not depend on it now, does it? >>> It doesn't, but then it doesn't support cpu hotplug, >>> OVMF(SMM) needs to cooperate with QEMU "and" ACPI tables to perform >>> the task and using the same interface/code path between all involved >>> parties makes the task easier with the least amount of duplicated >>> interfaces and more robust. >>> >>> Re-implementing alternative interface for firmware (fwcfg or what not) >>> would work as well, but it's only question of time when ACPI and >>> this new interface disagree on how world works and process falls >>> apart. >> >> Then we should consider switching acpi to use fw cfg. >> Or build another interface that can scale. > > Could be an option, it would be a pain to write a driver in AML for fwcfg access though > (I've looked at possibility to access fwcfg from AML about a year ago and gave up. > I'm definitely not volunteering for the second attempt and can't even give an estimate > it it's viable approach). > > But what scaling issue you are talking about, exactly? > With current CPU hotplug interface we can handle upto UNIT32_MAX cpus, and extend > interface without need to increase IO window we are using now. > > Granted IO access it not fastest compared to fwcfg in DMA mode, but we already > doing stop machine when switching to SMM which is orders of magnitude slower. > Consensus was to compromise on speed of CPU hotplug versus more complex and more > problematic unicast SMM mode in OVMF (can't find a particular email but we have discussed > it with Laszlo already, when I considered ways to optimize hotplug speed) Right, the speed of handling a CPU hotplug event is basically irrelevant, whereas broadcast SMI (in response to writing IO port 0xB2) is really important. Thanks Laszlo ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface 2019-10-10 13:39 ` Igor Mammedov 2019-10-10 13:59 ` Michael S. Tsirkin @ 2019-10-10 14:16 ` Eduardo Habkost 2019-10-10 14:49 ` Michael S. Tsirkin 2019-10-10 17:09 ` Igor Mammedov 1 sibling, 2 replies; 43+ messages in thread From: Eduardo Habkost @ 2019-10-10 14:16 UTC (permalink / raw) To: Igor Mammedov Cc: Michael S. Tsirkin, Laszlo Ersek, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On Thu, Oct 10, 2019 at 03:39:12PM +0200, Igor Mammedov wrote: > On Thu, 10 Oct 2019 05:56:55 -0400 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, Oct 09, 2019 at 09:22:49AM -0400, Igor Mammedov wrote: > > > As an alternative to passing to firmware topology info via new fwcfg files > > > so it could recreate APIC IDs based on it and order CPUs are enumerated, > > > > > > extend CPU hotplug interface to return APIC ID as response to the new command > > > CPHP_GET_CPU_ID_CMD. > > > > One big piece missing here is motivation: > I thought the only willing reader was Laszlo (who is aware of context) > so I skipped on details and confused others :/ > > > Who's going to use this interface? > In current state it's for firmware, since ACPI tables can cheat > by having APIC IDs statically built in. > > If we were creating CPU objects in ACPI dynamically > we would be using this command as well. It would save > us quite a bit space in ACPI blob but it would be a pain > to debug and diagnose problems in ACPI tables, so I'd rather > stay with static CPU descriptions in ACPI tables for the sake > of maintenance. > > > So far CPU hotplug was used by the ACPI, so we didn't > > really commit to a fixed interface too strongly. > > > > Is this a replacement to Laszlo's fw cfg interface? > > If yes is the idea that OVMF going to depend on CPU hotplug directly then? > > It does not depend on it now, does it? > It doesn't, but then it doesn't support cpu hotplug, > OVMF(SMM) needs to cooperate with QEMU "and" ACPI tables to perform > the task and using the same interface/code path between all involved > parties makes the task easier with the least amount of duplicated > interfaces and more robust. > > Re-implementing alternative interface for firmware (fwcfg or what not) > would work as well, but it's only question of time when ACPI and > this new interface disagree on how world works and process falls > apart. > > > If answers to all of the above is yes, then I don't really like it: it > > is better to keep all paravirt stuff in one place, namely in fw cfg. > Lets discuss, what cpu hotplug fwcfg interface could look like in > [PATCH 3/4] hw/i386: add facility to expose CPU topology over fw-cfg > mail thread and clarify (dis)likes with concrete reasons. > > So far I managed to convince myself that we ought to reuse > and extend current CPU hotplug interface with firmware features, > to endup with consolidated cpu hotplug process without > introducing duplicate ABIs, but I could be wrong so > lets see if fwcfg will be the better approach. > I was more inclined towards the approach in this patch, because I see it as just a bug fix in the CPU hotplug interface (which should have been using the hardware CPU identifier as the CPU selector since the beginning). Providing the missing information in fw_cfg isn't necessarily bad, but please document it explicitly as a hotplug_cpu_selector => cpu_hardware_id mapping, so people won't use "CPU index" as a generic identifier elsewhere. -- Eduardo ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface 2019-10-10 14:16 ` Eduardo Habkost @ 2019-10-10 14:49 ` Michael S. Tsirkin 2019-10-10 17:09 ` Igor Mammedov 1 sibling, 0 replies; 43+ messages in thread From: Michael S. Tsirkin @ 2019-10-10 14:49 UTC (permalink / raw) To: Eduardo Habkost Cc: Laszlo Ersek, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Igor Mammedov, Philippe Mathieu-Daudé, Richard Henderson On Thu, Oct 10, 2019 at 11:16:52AM -0300, Eduardo Habkost wrote: > On Thu, Oct 10, 2019 at 03:39:12PM +0200, Igor Mammedov wrote: > > On Thu, 10 Oct 2019 05:56:55 -0400 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Wed, Oct 09, 2019 at 09:22:49AM -0400, Igor Mammedov wrote: > > > > As an alternative to passing to firmware topology info via new fwcfg files > > > > so it could recreate APIC IDs based on it and order CPUs are enumerated, > > > > > > > > extend CPU hotplug interface to return APIC ID as response to the new command > > > > CPHP_GET_CPU_ID_CMD. > > > > > > One big piece missing here is motivation: > > I thought the only willing reader was Laszlo (who is aware of context) > > so I skipped on details and confused others :/ > > > > > Who's going to use this interface? > > In current state it's for firmware, since ACPI tables can cheat > > by having APIC IDs statically built in. > > > > If we were creating CPU objects in ACPI dynamically > > we would be using this command as well. It would save > > us quite a bit space in ACPI blob but it would be a pain > > to debug and diagnose problems in ACPI tables, so I'd rather > > stay with static CPU descriptions in ACPI tables for the sake > > of maintenance. > > > > > So far CPU hotplug was used by the ACPI, so we didn't > > > really commit to a fixed interface too strongly. > > > > > > Is this a replacement to Laszlo's fw cfg interface? > > > If yes is the idea that OVMF going to depend on CPU hotplug directly then? > > > It does not depend on it now, does it? > > It doesn't, but then it doesn't support cpu hotplug, > > OVMF(SMM) needs to cooperate with QEMU "and" ACPI tables to perform > > the task and using the same interface/code path between all involved > > parties makes the task easier with the least amount of duplicated > > interfaces and more robust. > > > > Re-implementing alternative interface for firmware (fwcfg or what not) > > would work as well, but it's only question of time when ACPI and > > this new interface disagree on how world works and process falls > > apart. > > > > > If answers to all of the above is yes, then I don't really like it: it > > > is better to keep all paravirt stuff in one place, namely in fw cfg. > > Lets discuss, what cpu hotplug fwcfg interface could look like in > > [PATCH 3/4] hw/i386: add facility to expose CPU topology over fw-cfg > > mail thread and clarify (dis)likes with concrete reasons. > > > > So far I managed to convince myself that we ought to reuse > > and extend current CPU hotplug interface with firmware features, > > to endup with consolidated cpu hotplug process without > > introducing duplicate ABIs, but I could be wrong so > > lets see if fwcfg will be the better approach. > > > > I was more inclined towards the approach in this patch, because I > see it as just a bug fix in the CPU hotplug interface (which > should have been using the hardware CPU identifier as the CPU > selector since the beginning). Well if ACPI is going to be using this, then that's different. Igor do you see any need for that? > Providing the missing information in fw_cfg isn't necessarily > bad, but please document it explicitly as a > hotplug_cpu_selector => cpu_hardware_id > mapping, so people won't use "CPU index" as a generic identifier > elsewhere. > > -- > Eduardo ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface 2019-10-10 14:16 ` Eduardo Habkost 2019-10-10 14:49 ` Michael S. Tsirkin @ 2019-10-10 17:09 ` Igor Mammedov 1 sibling, 0 replies; 43+ messages in thread From: Igor Mammedov @ 2019-10-10 17:09 UTC (permalink / raw) To: Eduardo Habkost Cc: Michael S. Tsirkin, Laszlo Ersek, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson On Thu, 10 Oct 2019 11:16:52 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Oct 10, 2019 at 03:39:12PM +0200, Igor Mammedov wrote: > > On Thu, 10 Oct 2019 05:56:55 -0400 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Wed, Oct 09, 2019 at 09:22:49AM -0400, Igor Mammedov wrote: > > > > As an alternative to passing to firmware topology info via new fwcfg files > > > > so it could recreate APIC IDs based on it and order CPUs are enumerated, > > > > > > > > extend CPU hotplug interface to return APIC ID as response to the new command > > > > CPHP_GET_CPU_ID_CMD. > > > > > > One big piece missing here is motivation: > > I thought the only willing reader was Laszlo (who is aware of context) > > so I skipped on details and confused others :/ > > > > > Who's going to use this interface? > > In current state it's for firmware, since ACPI tables can cheat > > by having APIC IDs statically built in. > > > > If we were creating CPU objects in ACPI dynamically > > we would be using this command as well. It would save > > us quite a bit space in ACPI blob but it would be a pain > > to debug and diagnose problems in ACPI tables, so I'd rather > > stay with static CPU descriptions in ACPI tables for the sake > > of maintenance. > > > > > So far CPU hotplug was used by the ACPI, so we didn't > > > really commit to a fixed interface too strongly. > > > > > > Is this a replacement to Laszlo's fw cfg interface? > > > If yes is the idea that OVMF going to depend on CPU hotplug directly then? > > > It does not depend on it now, does it? > > It doesn't, but then it doesn't support cpu hotplug, > > OVMF(SMM) needs to cooperate with QEMU "and" ACPI tables to perform > > the task and using the same interface/code path between all involved > > parties makes the task easier with the least amount of duplicated > > interfaces and more robust. > > > > Re-implementing alternative interface for firmware (fwcfg or what not) > > would work as well, but it's only question of time when ACPI and > > this new interface disagree on how world works and process falls > > apart. > > > > > If answers to all of the above is yes, then I don't really like it: it > > > is better to keep all paravirt stuff in one place, namely in fw cfg. > > Lets discuss, what cpu hotplug fwcfg interface could look like in > > [PATCH 3/4] hw/i386: add facility to expose CPU topology over fw-cfg > > mail thread and clarify (dis)likes with concrete reasons. > > > > So far I managed to convince myself that we ought to reuse > > and extend current CPU hotplug interface with firmware features, > > to endup with consolidated cpu hotplug process without > > introducing duplicate ABIs, but I could be wrong so > > lets see if fwcfg will be the better approach. > > > > I was more inclined towards the approach in this patch, because I > see it as just a bug fix in the CPU hotplug interface (which > should have been using the hardware CPU identifier as the CPU > selector since the beginning). > > Providing the missing information in fw_cfg isn't necessarily > bad, but please document it explicitly as a > hotplug_cpu_selector => cpu_hardware_id > mapping, so people won't use "CPU index" as a generic identifier > elsewhere. Currently cpu_selector is UID (or whatever you'd like to name it) for a CPU instance in ACPI tables. It just happens to be non sparse range [0..max_cpus) and was just a convenient way to make up IDs and handle them on hw side (requires simple array). Sure I'll document it as such to avoid mis-understanding, plus a bunch of other fixes to the spec. ^ permalink raw reply [flat|nested] 43+ messages in thread
end of thread, other threads:[~2019-10-24 16:13 UTC | newest] Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-09 13:22 [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface Igor Mammedov 2019-10-09 13:22 ` [RFC 1/3] acpi: cpuhp: fix 'Command data' description is spec Igor Mammedov 2019-10-10 12:33 ` Laszlo Ersek 2019-10-17 15:41 ` Igor Mammedov 2019-10-18 13:24 ` Laszlo Ersek 2019-10-10 13:31 ` Laszlo Ersek 2019-10-10 13:36 ` Laszlo Ersek 2019-10-22 17:17 ` Christophe de Dinechin 2019-10-22 17:37 ` Laszlo Ersek 2019-10-09 13:22 ` [RFC 2/3] acpi: cpuhp: add typical usecases into spec Igor Mammedov 2019-10-10 13:04 ` Laszlo Ersek 2019-10-10 13:15 ` Laszlo Ersek 2019-10-10 14:13 ` Laszlo Ersek 2019-10-18 14:45 ` Igor Mammedov 2019-10-09 13:22 ` [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command Igor Mammedov 2019-10-10 14:56 ` Laszlo Ersek 2019-10-10 15:06 ` Michael S. Tsirkin 2019-10-10 17:23 ` Igor Mammedov 2019-10-10 17:53 ` Laszlo Ersek 2019-10-10 19:26 ` Eduardo Habkost 2019-10-11 8:07 ` Laszlo Ersek 2019-10-18 16:18 ` Igor Mammedov 2019-10-21 13:06 ` Laszlo Ersek 2019-10-22 12:39 ` Laszlo Ersek 2019-10-22 14:42 ` Igor Mammedov 2019-10-22 15:49 ` Laszlo Ersek 2019-10-23 14:59 ` Igor Mammedov 2019-10-24 15:07 ` Philippe Mathieu-Daudé 2019-10-10 9:56 ` [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface Michael S. Tsirkin 2019-10-10 13:39 ` Igor Mammedov 2019-10-10 13:59 ` Michael S. Tsirkin 2019-10-10 15:57 ` Igor Mammedov 2019-10-10 18:15 ` Michael S. Tsirkin 2019-10-11 7:41 ` Laszlo Ersek 2019-10-10 19:20 ` Eduardo Habkost 2019-10-11 8:01 ` Laszlo Ersek 2019-10-11 13:00 ` Michael S. Tsirkin 2019-10-11 16:13 ` Laszlo Ersek 2019-10-11 10:47 ` Igor Mammedov 2019-10-11 6:54 ` Laszlo Ersek 2019-10-10 14:16 ` Eduardo Habkost 2019-10-10 14:49 ` Michael S. Tsirkin 2019-10-10 17:09 ` Igor Mammedov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.