[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/4] hw/i386: add facility to expose CPU topology over fw-cfg
From: |
Laszlo Ersek |
Subject: |
Re: [PATCH 3/4] hw/i386: add facility to expose CPU topology over fw-cfg |
Date: |
Tue, 8 Oct 2019 20:31:22 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 |
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" <address@hidden>
>> Cc: Eduardo Habkost <address@hidden>
>> Cc: Igor Mammedov <address@hidden>
>> Cc: Marcel Apfelbaum <address@hidden>
>> Cc: Paolo Bonzini <address@hidden>
>> Cc: Philippe Mathieu-Daudé <address@hidden>
>> Cc: Richard Henderson <address@hidden>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
>> Signed-off-by: Laszlo Ersek <address@hidden>
>> ---
>> 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é <address@hidden>
Thank you!
Laszlo
- [PATCH 0/4] hw/i386: pass "MachineState.smp.max_cpus" to OVMF, Laszlo Ersek, 2019/10/08
- [PATCH 2/4] target/i386: remove useless enable_compat_apic_id_mode() prototype, Laszlo Ersek, 2019/10/08
- [PATCH 1/4] fw_cfg: bump file slots to 40, Laszlo Ersek, 2019/10/08
- [PATCH 3/4] hw/i386: add facility to expose CPU topology over fw-cfg, Laszlo Ersek, 2019/10/08
- Re: [PATCH 3/4] hw/i386: add facility to expose CPU topology over fw-cfg, Philippe Mathieu-Daudé, 2019/10/08
- Re: [PATCH 3/4] hw/i386: add facility to expose CPU topology over fw-cfg,
Laszlo Ersek <=
- Re: [PATCH 3/4] hw/i386: add facility to expose CPU topology over fw-cfg, Igor Mammedov, 2019/10/08
- Re: [PATCH 3/4] hw/i386: add facility to expose CPU topology over fw-cfg, Michael S. Tsirkin, 2019/10/10
- Re: [PATCH 3/4] hw/i386: add facility to expose CPU topology over fw-cfg, Igor Mammedov, 2019/10/10
- Re: [PATCH 3/4] hw/i386: add facility to expose CPU topology over fw-cfg, Laszlo Ersek, 2019/10/10
- Re: [PATCH 3/4] hw/i386: add facility to expose CPU topology over fw-cfg, Michael S. Tsirkin, 2019/10/10
- Re: [PATCH 3/4] hw/i386: add facility to expose CPU topology over fw-cfg, Laszlo Ersek, 2019/10/11
- Re: [PATCH 3/4] hw/i386: add facility to expose CPU topology over fw-cfg, Laszlo Ersek, 2019/10/11
- Re: [PATCH 3/4] hw/i386: add facility to expose CPU topology over fw-cfg, Laszlo Ersek, 2019/10/10