Re: [PATCH 3/4] hw/i386: add facility to expose CPU topology over fw-cfg

[Laszlo Ersek <lersek@redhat.com>]

On 10/09/19 13:13, Igor Mammedov wrote:
> On Tue, 8 Oct 2019 20:58:30 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> Eduardo, Igor,
>>
>> On 10/08/19 12:52, Laszlo Ersek wrote:
>>> FW_CFG_MAX_CPUS exposes the (exclusive) maximum APIC ID to guest firmware,
>>> due to historical reasons. That value is not useful to edk2, however. For
>>> supporting VCPU hotplug, edk2 needs:
>>>
>>> - the boot CPU count (already exposed in FW_CFG_NB_CPUS),
>>>
>>> - and the maximum foreseen CPU count (tracked in
>>>   "MachineState.smp.max_cpus", but not currently exposed).
>>>
>>> Add a new fw-cfg file to expose "max_cpus".
>>>
>>> While at it, expose the rest of the topology too (die / core / thread
>>> counts), because I expect that the VCPU hotplug feature for OVMF will
>>> ultimately need those too, and the data size is not large.  
>>
>> 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).
> 
> In general duplicating cpu_index+topo => apic id in firmware I
> consider as a really bad idea (even ignoring cpu_index 
> which I were trying to get rid of in QEMU), it's going to break
> when algorithms diverge and it will be never ending race.

OK.

> Topology is rather messy business, not only arch specific but also
> cpu specific (ex: on my review TODO list, there is a series for
> fixing broken EPYCs topo). Who knows what other variables would be
> add dependencies for calculating APIC IDs down the road.
> 
> I also consider to re-use CPU hotplug interface on ARM, which will
> bring its own set of algorithms.
> 
> Let's instead add a command to CPU hotplug interface to return
> APIC ID (which QEMU already calculated) and later MPIDR (ARM)
> for selected CPU, so firmware could get it while enumeration CPUs
> via that interface.

Sounds good to me, thanks.

I'll stay tuned for your patches! :)

Thanks!
Laszlo