[RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface

Igor Mammedov posted 3 patches 4 years, 6 months ago
Test asan passed
Test checkpatch passed
Test FreeBSD passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test docker-quick@centos7 passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20191009132252.17860-1-imammedo@redhat.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>
docs/specs/acpi_cpu_hotplug.txt | 37 ++++++++++++++++++++++++++++++---
hw/acpi/cpu.c                   | 15 +++++++++++++
hw/acpi/trace-events            |  1 +
3 files changed, 50 insertions(+), 3 deletions(-)
[RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface
Posted by Igor Mammedov 4 years, 6 months ago
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


Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface
Posted by Michael S. Tsirkin 4 years, 6 months ago
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

Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface
Posted by Igor Mammedov 4 years, 6 months ago
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  
> 


Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface
Posted by Michael S. Tsirkin 4 years, 6 months ago
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  
> > 

Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface
Posted by Igor Mammedov 4 years, 6 months ago
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  
> > > 
> 


Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface
Posted by Laszlo Ersek 4 years, 6 months ago
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

Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface
Posted by Eduardo Habkost 4 years, 6 months ago
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

Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface
Posted by Igor Mammedov 4 years, 6 months ago
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.

Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface
Posted by Laszlo Ersek 4 years, 6 months ago
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

Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface
Posted by Michael S. Tsirkin 4 years, 6 months ago
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

Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface
Posted by Laszlo Ersek 4 years, 6 months ago
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


Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface
Posted by Michael S. Tsirkin 4 years, 6 months ago
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

Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface
Posted by Laszlo Ersek 4 years, 6 months ago
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

Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface
Posted by Eduardo Habkost 4 years, 6 months ago
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

Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface
Posted by Michael S. Tsirkin 4 years, 6 months ago
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

Re: [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to cpu hotplug MMIO interface
Posted by Igor Mammedov 4 years, 6 months ago
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.