On 10/08/19 15:29, Philippe Mathieu-Daudé wrote: > Hi Laszlo, > > On 10/8/19 12:52 PM, 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. This is >> slightly complicated by the fact that the die count is specific to >> PCMachineState, but fw_cfg_arch_create() intends to be PC-independent >> (see >> commit 149c50cabcc4). > > The X86 topology is generic to the architecture (not machine specific) > so it is well placed in fw_cfg_arch_create().
Certainly -- I didn't mean to "complain" in the commit message, just to point out why I added a new parameter to fw_cfg_arch_create(). Because, my first instinct was to change fw_cfg_arch_create() to simply take a (PCMachineState*), and then fw_cfg_arch_create() could fetch whatever it needed, internally. But, upon finding your commit 149c50cabcc4, I realized that adding a new parameter was the correct approach (just "slightly complicated" relative to passing (PCMachineState*) whole-sale.) To wit, I didn't write "*tries* to be PC-independent", but "*intends* to be PC-independent". I agree with the intent :) > >> >> For now, the feature is temporarily disabled. > > I see you enable it in the PC machine in the next patch. > Do you plan to remove the 'expose_topology' argument and expose the key > later, or is this comment simply related to this patch? > > Ah, now I see you disable it previous to pc-4.2, OK. Right, the sentence only refers to the "false" argument in this patch, for the "expose_topology" parameter. It took me some time to come up with this solution. I certainly wanted to separate the feature from the versioned machine type changes. One approach could have been to introduce the fw_cfg_expose_topology() function in itself, in a separate patch -- but then bisection would break at that commit, because gcc doesn't like static functions (functions with internal linkage) that are never called. So I wanted to call the new function at once, but short-circuit it too. (I tend to build patch series at every stage, before posting them.) > >> Cc: "Michael S. Tsirkin" <m...@redhat.com> >> Cc: Eduardo Habkost <ehabk...@redhat.com> >> Cc: Igor Mammedov <imamm...@redhat.com> >> Cc: Marcel Apfelbaum <marcel.apfelb...@gmail.com> >> Cc: Paolo Bonzini <pbonz...@redhat.com> >> Cc: Philippe Mathieu-Daudé <phi...@redhat.com> >> Cc: Richard Henderson <r...@twiddle.net> >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515 >> Signed-off-by: Laszlo Ersek <ler...@redhat.com> >> --- >> hw/i386/fw_cfg.h | 30 +++++++++++++++++++++++++++++- >> hw/i386/fw_cfg.c | 26 ++++++++++++++++++++++++-- >> hw/i386/pc.c | 4 ++-- >> 3 files changed, 55 insertions(+), 5 deletions(-) >> >> diff --git a/hw/i386/fw_cfg.h b/hw/i386/fw_cfg.h >> index e0856a376996..d742435b9793 100644 >> --- a/hw/i386/fw_cfg.h >> +++ b/hw/i386/fw_cfg.h >> @@ -18,9 +18,37 @@ >> #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3) >> #define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4) >> +/** >> + * FWCfgX86Topology: expose the X86 CPU topology to guest firmware >> over fw-cfg. >> + * >> + * All fields have little-endian encoding. >> + * >> + * @dies: Number of dies per package (aka socket). Set it to 1 >> unless the >> + * concrete MachineState subclass defines it differently. >> + * @cores: Corresponds to @CpuTopology.@cores. >> + * @threads: Corresponds to @CpuTopology.@threads. >> + * @max_cpus: Corresponds to @CpuTopology.@max_cpus. >> + * >> + * Firmware can derive the package (aka socket) count with the following >> + * formula: >> + * >> + * DIV_ROUND_UP(@max_cpus, @dies * @cores * @threads) >> + * >> + * Firmware can derive APIC ID field widths and offsets per the standard >> + * calculations in "include/hw/i386/topology.h". >> + */ >> +typedef struct FWCfgX86Topology { >> + uint32_t dies; >> + uint32_t cores; >> + uint32_t threads; >> + uint32_t max_cpus; >> +} QEMU_PACKED FWCfgX86Topology; >> + >> FWCfgState *fw_cfg_arch_create(MachineState *ms, >> uint16_t boot_cpus, >> - uint16_t apic_id_limit); >> + uint16_t apic_id_limit, >> + unsigned smp_dies, >> + bool expose_topology); >> void fw_cfg_build_smbios(MachineState *ms, FWCfgState *fw_cfg); >> void fw_cfg_build_feature_control(MachineState *ms, FWCfgState >> *fw_cfg); >> diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c >> index 39b6bc60520c..33d09875014f 100644 >> --- a/hw/i386/fw_cfg.c >> +++ b/hw/i386/fw_cfg.c >> @@ -85,9 +85,26 @@ void fw_cfg_build_smbios(MachineState *ms, >> FWCfgState *fw_cfg) >> } >> } >> +static void fw_cfg_expose_topology(FWCfgState *fw_cfg, >> + unsigned dies, >> + unsigned cores, >> + unsigned threads, >> + unsigned max_cpus) >> +{ >> + FWCfgX86Topology *topo = g_new(FWCfgX86Topology, 1); >> + >> + topo->dies = cpu_to_le32(dies); >> + topo->cores = cpu_to_le32(cores); >> + topo->threads = cpu_to_le32(threads); >> + topo->max_cpus = cpu_to_le32(max_cpus); >> + fw_cfg_add_file(fw_cfg, "etc/x86-smp-topology", topo, sizeof *topo); >> +} >> + >> FWCfgState *fw_cfg_arch_create(MachineState *ms, >> - uint16_t boot_cpus, >> - uint16_t apic_id_limit) >> + uint16_t boot_cpus, >> + uint16_t apic_id_limit, >> + unsigned smp_dies, >> + bool expose_topology) >> { >> FWCfgState *fw_cfg; >> uint64_t *numa_fw_cfg; >> @@ -143,6 +160,11 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms, >> (1 + apic_id_limit + nb_numa_nodes) * >> sizeof(*numa_fw_cfg)); >> + if (expose_topology) { >> + fw_cfg_expose_topology(fw_cfg, smp_dies, ms->smp.cores, >> + ms->smp.threads, ms->smp.max_cpus); >> + } >> + >> return fw_cfg; >> } >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index bcda50efcc23..bb72b12edad2 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c >> @@ -1738,8 +1738,8 @@ void pc_memory_init(PCMachineState *pcms, >> option_rom_mr, >> 1); >> - fw_cfg = fw_cfg_arch_create(machine, >> - pcms->boot_cpus, pcms->apic_id_limit); >> + fw_cfg = fw_cfg_arch_create(machine, pcms->boot_cpus, >> pcms->apic_id_limit, >> + pcms->smp_dies, false); >> rom_set_fw(fw_cfg); >> > > Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com> Thank you! Laszlo
