hw/i386/pc: Do not restrict the fw_cfg functions to the PC machine

PATCH Superseded Philippe Mathieu-Daudé 2 participants 18 messages Jul 1, 2019 KVM

lore

Share & tools

lore

Philippe Mathieu-Daudé

author

2019-07-01 13:35 UTC #0 lore raw

v3 cover hw/i386/pc: Do not restrict the fw_cfg functions to the PC machine

Hi,

This is my take at salvaging some NEMU good work.
Samuel worked in adding the fw_cfg device to the x86-virt NEMU machine. This series is inspired by NEMU's commit 3cb92d080835 [0] and adapted to upstream style. The result makes the upstream codebase more modularizable.
There are very little logical changes, this is mostly a cleanup refactor.

Since v2 [2]:
- Addressed MST comments from v2 (only patch #2 modified)
- do not use unsigned for enum
- do not add unuseful documentation

Since v1 [1]:
- Addressed Li and MST comments

$ git backport-diff -u v2
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/15:[----] [--] 'hw/i386/pc: Use e820_get_num_entries() to access e820_entries' 002/15:[0131] [FC] 'hw/i386/pc: Extract e820 memory layout code' 003/15:[----] [--] 'hw/i386/pc: Use address_space_memory in place' 004/15:[----] [--] 'hw/i386/pc: Rename bochs_bios_init as more generic fw_cfg_arch_create' 005/15:[----] [--] 'hw/i386/pc: Pass the boot_cpus value by argument' 006/15:[----] [--] 'hw/i386/pc: Pass the apic_id_limit value by argument' 007/15:[----] [--] 'hw/i386/pc: Pass the CPUArchIdList array by argument' 008/15:[----] [--] 'hw/i386/pc: Let fw_cfg_init() use the generic MachineState' 009/15:[----] [--] 'hw/i386/pc: Let pc_build_smbios() take a FWCfgState argument' 010/15:[----] [--] 'hw/i386/pc: Let pc_build_smbios() take a generic MachineState argument' 011/15:[----] [--] 'hw/i386/pc: Rename pc_build_smbios() as generic fw_cfg_build_smbios()' 012/15:[----] [--] 'hw/i386/pc: Let pc_build_feature_control() take a FWCfgState argument' 013/15:[----] [--] 'hw/i386/pc: Let pc_build_feature_control() take a MachineState argument' 014/15:[----] [--] 'hw/i386/pc: Rename pc_build_feature_control() as generic fw_cfg_build_*' 015/15:[----] [--] 'hw/i386/pc: Extract the x86 generic fw_cfg code'

Regards,

Phil.

[0] https://github.com/intel/nemu/commit/3cb92d080835ac8d47c8b713156338afa33cff5c
[1] https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg05759.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg02786.html

Philippe Mathieu-Daudé (15):
hw/i386/pc: Use e820_get_num_entries() to access e820_entries hw/i386/pc: Extract e820 memory layout code
hw/i386/pc: Use address_space_memory in place
hw/i386/pc: Rename bochs_bios_init as more generic fw_cfg_arch_create hw/i386/pc: Pass the boot_cpus value by argument hw/i386/pc: Pass the apic_id_limit value by argument hw/i386/pc: Pass the CPUArchIdList array by argument hw/i386/pc: Let fw_cfg_init() use the generic MachineState hw/i386/pc: Let pc_build_smbios() take a FWCfgState argument hw/i386/pc: Let pc_build_smbios() take a generic MachineState argument hw/i386/pc: Rename pc_build_smbios() as generic fw_cfg_build_smbios() hw/i386/pc: Let pc_build_feature_control() take a FWCfgState argument hw/i386/pc: Let pc_build_feature_control() take a MachineState
argument
hw/i386/pc: Rename pc_build_feature_control() as generic

    fw_cfg_build_*

hw/i386/pc: Extract the x86 generic fw_cfg code

 hw/i386/Makefile.objs        |   2 +-
 hw/i386/e820_memory_layout.c |  59 ++++++++++
 hw/i386/e820_memory_layout.h |  42 ++++++++
 hw/i386/fw_cfg.c             | 137 ++++++++++++++++++++++++
 hw/i386/fw_cfg.h             |   8 ++
 hw/i386/pc.c                 | 201 ++---------------------------------
 include/hw/i386/pc.h         |  11 --
 target/i386/kvm.c            |   1 +
 8 files changed, 256 insertions(+), 205 deletions(-)
 create mode 100644 hw/i386/e820_memory_layout.c
 create mode 100644 hw/i386/e820_memory_layout.h

Signature · 1 line

2.20.1

Philippe Mathieu-Daudé

author

2019-07-01 13:35 UTC #1 lore raw

PATCH v3 1/15 hw/i386/pc: Use e820_get_num_entries() to access e820_entries

To be able to extract the e820* code out of this file (in the next patch), access e820_entries with its correct helper.

Reviewed-by Li Qiang <[email protected]>

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/i386/pc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

hw/i386/pc.c +1 -1

index e96360b47a..691726b85b 100644
@@ -1020,7 +1020,7 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)
     fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE,
                      &e820_reserve, sizeof(e820_reserve));
     fw_cfg_add_file(fw_cfg, "etc/e820", e820_table,
-                    sizeof(struct e820_entry) * e820_entries);
+                    sizeof(struct e820_entry) * e820_get_num_entries());
 
     fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_cfg, sizeof(hpet_cfg));
     /* allocate memory for the NUMA channel: one (64bit) word for the number

Signature · 1 line

2.20.1

Philippe Mathieu-Daudé

author

2019-07-01 13:35 UTC #2 lore raw

PATCH v3 2/15 hw/i386/pc: Extract e820 memory layout code

In reply to Philippe Mathieu-Daudé

Reviewed-by Li Qiang <[email protected]>

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
v3: KISS, do not use unsigned, do not add broken documentation

---
 hw/i386/Makefile.objs        |  2 +-
 hw/i386/e820_memory_layout.c | 59 ++++++++++++++++++++++++++++++++++
 hw/i386/e820_memory_layout.h | 42 +++++++++++++++++++++++++
 hw/i386/pc.c                 | 61 +-----------------------------------
 include/hw/i386/pc.h         | 11 -------
 target/i386/kvm.c            |  1 +
 6 files changed, 104 insertions(+), 72 deletions(-)
 create mode 100644 hw/i386/e820_memory_layout.c
 create mode 100644 hw/i386/e820_memory_layout.h

hw/i386/Makefile.objs +1 -1

index 5d9c9efd5f..d3374e0831 100644
@@ -1,5 +1,5 @@
 obj-$(CONFIG_KVM) += kvm/
-obj-y += multiboot.o
+obj-y += e820_memory_layout.o multiboot.o
 obj-y += pc.o
 obj-$(CONFIG_I440FX) += pc_piix.o
 obj-$(CONFIG_Q35) += pc_q35.o

hw/i386/e820_memory_layout.c new file +59 -0

59 lines not on this page · raw message

hw/i386/e820_memory_layout.h new file +42 -0

42 lines not on this page · raw message

hw/i386/pc.c +1 ~~-60~~

79 lines not on this page · raw message

include/hw/i386/pc.h +0 ~~-11~~

index c54cc54a47..99b0a2e705 100644
@@ -282,17 +282,6 @@ void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
                        const CPUArchIdList *apic_ids, GArray *entry);
 
-/* e820 types */
-#define E820_RAM        1
-#define E820_RESERVED   2
-#define E820_ACPI       3
-#define E820_NVS        4
-#define E820_UNUSABLE   5
-
-int e820_add_entry(uint64_t, uint64_t, uint32_t);
-int e820_get_num_entries(void);
-bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
-
 extern GlobalProperty pc_compat_4_0[];
 extern const size_t pc_compat_4_0_len;

target/i386/kvm.c +1 -0

index e4b4f5756a..cd4eb1ed0d 100644
@@ -38,6 +38,7 @@
 #include "hw/i386/apic-msidef.h"
 #include "hw/i386/intel_iommu.h"
 #include "hw/i386/x86-iommu.h"
+#include "hw/i386/e820_memory_layout.h"
 
 #include "hw/pci/pci.h"
 #include "hw/pci/msi.h"

Signature · 1 line

2.20.1

Philippe Mathieu-Daudé

author

2019-07-01 13:36 UTC #3 lore raw

PATCH v3 3/15 hw/i386/pc: Use address_space_memory in place

In reply to Philippe Mathieu-Daudé

The address_space_memory variable is used once. Use it in place and remove the argument.

Reviewed-by Li Qiang <[email protected]>

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/i386/pc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

hw/i386/pc.c +4 -3

index 8ac85eadf1..ea7422b16c 100644
@@ -929,7 +929,7 @@ static void pc_build_smbios(PCMachineState *pcms)
     }
 }
 
-static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)
+static FWCfgState *bochs_bios_init(PCMachineState *pcms)
 {
     FWCfgState *fw_cfg;
     uint64_t *numa_fw_cfg;

@@ -937,7 +937,8 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)
     const CPUArchIdList *cpus;
     MachineClass *mc = MACHINE_GET_CLASS(pcms);
 
-    fw_cfg = fw_cfg_init_io_dma(FW_CFG_IO_BASE, FW_CFG_IO_BASE + 4, as);
+    fw_cfg = fw_cfg_init_io_dma(FW_CFG_IO_BASE, FW_CFG_IO_BASE + 4,
+                                &address_space_memory);
     fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);
 
     /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:

@@ -1762,7 +1763,7 @@ void pc_memory_init(PCMachineState *pcms,
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = bochs_bios_init(&address_space_memory, pcms);
+    fw_cfg = bochs_bios_init(pcms);
 
     rom_set_fw(fw_cfg);

Signature · 1 line

2.20.1

Philippe Mathieu-Daudé

author

2019-07-01 13:36 UTC #4 lore raw

PATCH v3 4/15 hw/i386/pc: Rename bochs_bios_init as more generic fw_cfg_arch_create

In reply to Philippe Mathieu-Daudé

The bochs_bios_init() function is not restricted to the Bochs BIOS and is useful to other BIOS.
Since it is not specific to the PC machine, and can be reused by other machines of the X86 architecture, rename it as fw_cfg_arch_create().

Reviewed-by Li Qiang <[email protected]>

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
v2: Rename x86_create_fw_cfg() -> fw_cfg_arch_create() (MST)

---
 hw/i386/pc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

hw/i386/pc.c +3 -3

index ea7422b16c..5fc52f6a0e 100644
@@ -929,7 +929,7 @@ static void pc_build_smbios(PCMachineState *pcms)
     }
 }
 
-static FWCfgState *bochs_bios_init(PCMachineState *pcms)
+static FWCfgState *fw_cfg_arch_create(PCMachineState *pcms)
 {
     FWCfgState *fw_cfg;
     uint64_t *numa_fw_cfg;

@@ -1509,7 +1509,7 @@ void pc_cpus_init(PCMachineState *pcms)
      * Limit for the APIC ID value, so that all
      * CPU APIC IDs are < pcms->apic_id_limit.
      *
-     * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init().
+     * This is used for FW_CFG_MAX_CPUS. See comments on fw_cfg_arch_create().
      */
     pcms->apic_id_limit = x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
     possible_cpus = mc->possible_cpu_arch_ids(ms);

@@ -1763,7 +1763,7 @@ void pc_memory_init(PCMachineState *pcms,
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = bochs_bios_init(pcms);
+    fw_cfg = fw_cfg_arch_create(pcms);
 
     rom_set_fw(fw_cfg);

Signature · 1 line

2.20.1

Philippe Mathieu-Daudé

author

2019-07-01 13:36 UTC #5 lore raw

PATCH v3 5/15 hw/i386/pc: Pass the boot_cpus value by argument

In reply to Philippe Mathieu-Daudé

The boot_cpus is used once. Pass it by argument, this will allow us to remove the PCMachineState argument later.

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/i386/pc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

hw/i386/pc.c +4 -3

index 5fc52f6a0e..eaeb7891dd 100644
@@ -929,7 +929,8 @@ static void pc_build_smbios(PCMachineState *pcms)
     }
 }
 
-static FWCfgState *fw_cfg_arch_create(PCMachineState *pcms)
+static FWCfgState *fw_cfg_arch_create(PCMachineState *pcms,
+                                      uint16_t boot_cpus)
 {
     FWCfgState *fw_cfg;
     uint64_t *numa_fw_cfg;

@@ -939,7 +940,7 @@ static FWCfgState *fw_cfg_arch_create(PCMachineState *pcms)
 
     fw_cfg = fw_cfg_init_io_dma(FW_CFG_IO_BASE, FW_CFG_IO_BASE + 4,
                                 &address_space_memory);
-    fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, boot_cpus);
 
     /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
      *

@@ -1763,7 +1764,7 @@ void pc_memory_init(PCMachineState *pcms,
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = fw_cfg_arch_create(pcms);
+    fw_cfg = fw_cfg_arch_create(pcms, pcms->boot_cpus);
 
     rom_set_fw(fw_cfg);

Signature · 1 line

2.20.1

Philippe Mathieu-Daudé

author

2019-07-01 13:36 UTC #6 lore raw

PATCH v3 6/15 hw/i386/pc: Pass the apic_id_limit value by argument

In reply to Philippe Mathieu-Daudé

Pass the apic_id_limit value by argument, this will allow us to remove the PCMachineState argument later.

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/i386/pc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

hw/i386/pc.c +3 -2

index eaeb7891dd..0248c8dc17 100644
@@ -930,7 +930,8 @@ static void pc_build_smbios(PCMachineState *pcms)
 }
 
 static FWCfgState *fw_cfg_arch_create(PCMachineState *pcms,
-                                      uint16_t boot_cpus)
+                                      uint16_t boot_cpus,
+                                      uint16_t apic_id_limit)
 {
     FWCfgState *fw_cfg;
     uint64_t *numa_fw_cfg;

@@ -1764,7 +1765,7 @@ void pc_memory_init(PCMachineState *pcms,
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = fw_cfg_arch_create(pcms, pcms->boot_cpus);
+    fw_cfg = fw_cfg_arch_create(pcms, pcms->boot_cpus, pcms->apic_id_limit);
 
     rom_set_fw(fw_cfg);

Signature · 1 line

2.20.1

Philippe Mathieu-Daudé

author

2019-07-01 13:36 UTC #7 lore raw

PATCH v3 7/15 hw/i386/pc: Pass the CPUArchIdList array by argument

In reply to Philippe Mathieu-Daudé

Pass the CPUArchIdList array by argument, this will allow us to remove the PCMachineState argument later.

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/i386/pc.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

hw/i386/pc.c +8 -9

index 0248c8dc17..1e856704e1 100644
@@ -930,14 +930,13 @@ static void pc_build_smbios(PCMachineState *pcms)
 }
 
 static FWCfgState *fw_cfg_arch_create(PCMachineState *pcms,
+                                      const CPUArchIdList *cpus,
                                       uint16_t boot_cpus,
                                       uint16_t apic_id_limit)
 {
     FWCfgState *fw_cfg;
     uint64_t *numa_fw_cfg;
     int i;
-    const CPUArchIdList *cpus;
-    MachineClass *mc = MACHINE_GET_CLASS(pcms);
 
     fw_cfg = fw_cfg_init_io_dma(FW_CFG_IO_BASE, FW_CFG_IO_BASE + 4,
                                 &address_space_memory);

@@ -955,7 +954,7 @@ static FWCfgState *fw_cfg_arch_create(PCMachineState *pcms,
      * So for compatibility reasons with old BIOSes we are stuck with
      * "etc/max-cpus" actually being apic_id_limit
      */
-    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)pcms->apic_id_limit);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, apic_id_limit);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES,
                      acpi_tables, acpi_tables_len);

@@ -971,20 +970,19 @@ static FWCfgState *fw_cfg_arch_create(PCMachineState *pcms,
      * of nodes, one word for each VCPU->node and one word for each node to
      * hold the amount of memory.
      */
-    numa_fw_cfg = g_new0(uint64_t, 1 + pcms->apic_id_limit + nb_numa_nodes);
+    numa_fw_cfg = g_new0(uint64_t, 1 + apic_id_limit + nb_numa_nodes);
     numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes);
-    cpus = mc->possible_cpu_arch_ids(MACHINE(pcms));
     for (i = 0; i < cpus->len; i++) {
         unsigned int apic_id = cpus->cpus[i].arch_id;
-        assert(apic_id < pcms->apic_id_limit);
+        assert(apic_id < apic_id_limit);
         numa_fw_cfg[apic_id + 1] = cpu_to_le64(cpus->cpus[i].props.node_id);
     }
     for (i = 0; i < nb_numa_nodes; i++) {
-        numa_fw_cfg[pcms->apic_id_limit + 1 + i] =
+        numa_fw_cfg[apic_id_limit + 1 + i] =
             cpu_to_le64(numa_info[i].node_mem);
     }
     fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA, numa_fw_cfg,
-                     (1 + pcms->apic_id_limit + nb_numa_nodes) *
+                     (1 + apic_id_limit + nb_numa_nodes) *
                      sizeof(*numa_fw_cfg));
 
     return fw_cfg;

@@ -1765,7 +1763,8 @@ void pc_memory_init(PCMachineState *pcms,
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = fw_cfg_arch_create(pcms, pcms->boot_cpus, pcms->apic_id_limit);
+    fw_cfg = fw_cfg_arch_create(pcms, mc->possible_cpu_arch_ids(machine),
+                                pcms->boot_cpus, pcms->apic_id_limit);
 
     rom_set_fw(fw_cfg);

Signature · 1 line

2.20.1

Philippe Mathieu-Daudé

author

2019-07-01 13:36 UTC #8 lore raw

PATCH v3 8/15 hw/i386/pc: Let fw_cfg_init() use the generic MachineState

In reply to Philippe Mathieu-Daudé

We removed the PCMachineState access, we can now let the fw_cfg_init() function to take a generic MachineState object.

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/i386/pc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

hw/i386/pc.c +3 -2

index 1e856704e1..60ee71924a 100644
@@ -929,7 +929,7 @@ static void pc_build_smbios(PCMachineState *pcms)
     }
 }
 
-static FWCfgState *fw_cfg_arch_create(PCMachineState *pcms,
+static FWCfgState *fw_cfg_arch_create(MachineState *ms,
                                       const CPUArchIdList *cpus,
                                       uint16_t boot_cpus,
                                       uint16_t apic_id_limit)

@@ -1667,6 +1667,7 @@ void pc_memory_init(PCMachineState *pcms,
     MemoryRegion *ram_below_4g, *ram_above_4g;
     FWCfgState *fw_cfg;
     MachineState *machine = MACHINE(pcms);
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
 
     assert(machine->ram_size == pcms->below_4g_mem_size +

@@ -1763,7 +1764,7 @@ void pc_memory_init(PCMachineState *pcms,
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = fw_cfg_arch_create(pcms, mc->possible_cpu_arch_ids(machine),
+    fw_cfg = fw_cfg_arch_create(machine, mc->possible_cpu_arch_ids(machine),
                                 pcms->boot_cpus, pcms->apic_id_limit);
 
     rom_set_fw(fw_cfg);

Signature · 1 line

2.20.1

Christophe de Dinechin

2019-07-01 16:01 UTC #9 lore raw

Re: [Qemu-devel] [PATCH v3 08/15] hw/i386/pc: Let fw_cfg_init() use the generic MachineState

Quoted 2 lines · Philippe Mathieu-Daudé

We removed the PCMachineState access, we can now let the fw_cfg_init() function to take a generic MachineState object.

to take -> take

Quoted 12 lines

Suggested-by Samuel Ortiz <[email protected]>
Signed-off-by Philippe Mathieu-Daudé <[email protected]>
---
 hw/i386/pc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
hw/i386/pc.c +0 -0
index 1e856704e1..60ee71924a 100644

hw/i386/pc.c static void pc_build_smbios(PCMachineState *pcms) · line 932

Quoted 5 lines

hw/i386/pc.c +1 -1

@@ -929,7 +929,7 @@ static void pc_build_smbios(PCMachineState *pcms)
     }
 }
 
-static FWCfgState *fw_cfg_arch_create(PCMachineState *pcms,
+static FWCfgState *fw_cfg_arch_create(MachineState *ms,

I don't see where ms is used in the function. Maybe in a later patch, I did not receive patches 09-15 yet.

Quoted 12 lines

hw/i386/pc.c +1 -0

                                       const CPUArchIdList *cpus,
                                       uint16_t boot_cpus,
                                       uint16_t apic_id_limit)

@@ -1667,6 +1667,7 @@ void pc_memory_init(PCMachineState *pcms,
     MemoryRegion *ram_below_4g, *ram_above_4g;
     FWCfgState *fw_cfg;
     MachineState *machine = MACHINE(pcms);
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
 
     assert(machine->ram_size == pcms->below_4g_mem_size +

hw/i386/pc.c void pc_memory_init(PCMachineState *pcms, · line 1770

Quoted 8 lines

hw/i386/pc.c +1 -1

@@ -1763,7 +1764,7 @@ void pc_memory_init(PCMachineState *pcms,
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = fw_cfg_arch_create(pcms, mc->possible_cpu_arch_ids(machine),
+    fw_cfg = fw_cfg_arch_create(machine, mc->possible_cpu_arch_ids(machine),
                                 pcms->boot_cpus, pcms->apic_id_limit);
 
     rom_set_fw(fw_cfg);

--
Cheers,
Christophe de Dinechin (IRC c3d)

Philippe Mathieu-Daudé

author

2019-07-01 19:56 UTC #10 lore raw

Re: [Qemu-devel] [PATCH v3 08/15] hw/i386/pc: Let fw_cfg_init() use the generic MachineState

Quoted 22 lines · Christophe de Dinechin

Quoted 2 lines · Philippe Mathieu-Daudé
We removed the PCMachineState access, we can now let the fw_cfg_init() function to take a generic MachineState object.
to take -> take
Quoted 15 lines
Suggested-by Samuel Ortiz <[email protected]>
Signed-off-by Philippe Mathieu-Daudé <[email protected]>
---
 hw/i386/pc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
hw/i386/pc.c +0 -0
index 1e856704e1..60ee71924a 100644
@@ -929,7 +929,7 @@ static void pc_build_smbios(PCMachineState *pcms)
     }
 }
 

Quoted 5 lines

hw/i386/pc.c static void pc_build_smbios(PCMachineState *pcms) · line 932
Quoted 2 lines
hw/i386/pc.c +1 -1
-static FWCfgState *fw_cfg_arch_create(PCMachineState *pcms,
+static FWCfgState *fw_cfg_arch_create(MachineState *ms,
I don't see where ms is used in the function. Maybe in a later patch, I did not receive patches 09-15 yet.

You are right, it is not used (even if the following patches).

Quoted 20 lines

hw/i386/pc.c +2 -1

                                       const CPUArchIdList *cpus,
                                       uint16_t boot_cpus,
                                       uint16_t apic_id_limit)

@@ -1667,6 +1667,7 @@ void pc_memory_init(PCMachineState *pcms,
     MemoryRegion *ram_below_4g, *ram_above_4g;
     FWCfgState *fw_cfg;
     MachineState *machine = MACHINE(pcms);
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
 
     assert(machine->ram_size == pcms->below_4g_mem_size +

@@ -1763,7 +1764,7 @@ void pc_memory_init(PCMachineState *pcms,
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = fw_cfg_arch_create(pcms, mc->possible_cpu_arch_ids(machine),
+    fw_cfg = fw_cfg_arch_create(machine, mc->possible_cpu_arch_ids(machine),
                                 pcms->boot_cpus, pcms->apic_id_limit);
 
     rom_set_fw(fw_cfg);

Philippe Mathieu-Daudé

author

2019-07-01 13:36 UTC #11 lore raw

PATCH v3 9/15 hw/i386/pc: Let pc_build_smbios() take a FWCfgState argument

In reply to Philippe Mathieu-Daudé

Pass the FWCfgState object by argument, this will allow us to remove the PCMachineState argument later.

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/i386/pc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

hw/i386/pc.c +5 -5

index 60ee71924a..2b6502a38c 100644
@@ -887,7 +887,7 @@ static uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
     }
 }
 
-static void pc_build_smbios(PCMachineState *pcms)
+static void pc_build_smbios(PCMachineState *pcms, FWCfgState *fw_cfg)
 {
     uint8_t *smbios_tables, *smbios_anchor;
     size_t smbios_tables_len, smbios_anchor_len;

@@ -901,7 +901,7 @@ static void pc_build_smbios(PCMachineState *pcms)
 
     smbios_tables = smbios_get_table_legacy(&smbios_tables_len);
     if (smbios_tables) {
-        fw_cfg_add_bytes(pcms->fw_cfg, FW_CFG_SMBIOS_ENTRIES,
+        fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
                          smbios_tables, smbios_tables_len);
     }

@@ -922,9 +922,9 @@ static void pc_build_smbios(PCMachineState *pcms)
     g_free(mem_array);
 
     if (smbios_anchor) {
-        fw_cfg_add_file(pcms->fw_cfg, "etc/smbios/smbios-tables",
+        fw_cfg_add_file(fw_cfg, "etc/smbios/smbios-tables",
                         smbios_tables, smbios_tables_len);
-        fw_cfg_add_file(pcms->fw_cfg, "etc/smbios/smbios-anchor",
+        fw_cfg_add_file(fw_cfg, "etc/smbios/smbios-anchor",
                         smbios_anchor, smbios_anchor_len);
     }
 }

@@ -1590,7 +1590,7 @@ void pc_machine_done(Notifier *notifier, void *data)
 
     acpi_setup();
     if (pcms->fw_cfg) {
-        pc_build_smbios(pcms);
+        pc_build_smbios(pcms, pcms->fw_cfg);
         pc_build_feature_control_file(pcms);
         /* update FW_CFG_NB_CPUS to account for -device added CPUs */
         fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);

Signature · 1 line

2.20.1

Philippe Mathieu-Daudé

author

2019-07-01 13:36 UTC #12 lore raw

PATCH v3 10/15 hw/i386/pc: Let pc_build_smbios() take a generic MachineState argument

In reply to Philippe Mathieu-Daudé

Let the pc_build_smbios() function take a generic MachineState argument.

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/i386/pc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

hw/i386/pc.c +2 -3

index 2b6502a38c..1195394694 100644
@@ -887,13 +887,12 @@ static uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
     }
 }
 
-static void pc_build_smbios(PCMachineState *pcms, FWCfgState *fw_cfg)
+static void pc_build_smbios(MachineState *ms, FWCfgState *fw_cfg)
 {
     uint8_t *smbios_tables, *smbios_anchor;
     size_t smbios_tables_len, smbios_anchor_len;
     struct smbios_phys_mem_area *mem_array;
     unsigned i, array_count;
-    MachineState *ms = MACHINE(pcms);
     X86CPU *cpu = X86_CPU(ms->possible_cpus->cpus[0].cpu);
 
     /* tell smbios about cpuid version and features */

@@ -1590,7 +1589,7 @@ void pc_machine_done(Notifier *notifier, void *data)
 
     acpi_setup();
     if (pcms->fw_cfg) {
-        pc_build_smbios(pcms, pcms->fw_cfg);
+        pc_build_smbios(MACHINE(pcms), pcms->fw_cfg);
         pc_build_feature_control_file(pcms);
         /* update FW_CFG_NB_CPUS to account for -device added CPUs */
         fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);

Signature · 1 line

2.20.1

Philippe Mathieu-Daudé

author

2019-07-01 13:36 UTC #13 lore raw

PATCH v3 11/15 hw/i386/pc: Rename pc_build_smbios() as generic fw_cfg_build_smbios()

In reply to Philippe Mathieu-Daudé

Now that the pc_build_smbios() function has been refactored to not depend of PC specific types, rename it to a more generic name.

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/i386/pc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/i386/pc.c +2 -2

index 1195394694..ba476fab7e 100644
@@ -887,7 +887,7 @@ static uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
     }
 }
 
-static void pc_build_smbios(MachineState *ms, FWCfgState *fw_cfg)
+static void fw_cfg_build_smbios(MachineState *ms, FWCfgState *fw_cfg)
 {
     uint8_t *smbios_tables, *smbios_anchor;
     size_t smbios_tables_len, smbios_anchor_len;

@@ -1589,7 +1589,7 @@ void pc_machine_done(Notifier *notifier, void *data)
 
     acpi_setup();
     if (pcms->fw_cfg) {
-        pc_build_smbios(MACHINE(pcms), pcms->fw_cfg);
+        fw_cfg_build_smbios(MACHINE(pcms), pcms->fw_cfg);
         pc_build_feature_control_file(pcms);
         /* update FW_CFG_NB_CPUS to account for -device added CPUs */
         fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);

Signature · 1 line

2.20.1

Philippe Mathieu-Daudé

author

2019-07-01 13:36 UTC #14 lore raw

PATCH v3 12/15 hw/i386/pc: Let pc_build_feature_control() take a FWCfgState argument

In reply to Philippe Mathieu-Daudé

Pass the FWCfgState object by argument, this will allow us to remove the PCMachineState argument later.

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/i386/pc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

hw/i386/pc.c +4 -3

index ba476fab7e..c49617a3f1 100644
@@ -1518,7 +1518,8 @@ void pc_cpus_init(PCMachineState *pcms)
     }
 }
 
-static void pc_build_feature_control_file(PCMachineState *pcms)
+static void pc_build_feature_control_file(PCMachineState *pcms,
+                                          FWCfgState *fw_cfg)
 {
     MachineState *ms = MACHINE(pcms);
     X86CPU *cpu = X86_CPU(ms->possible_cpus->cpus[0].cpu);

@@ -1544,7 +1545,7 @@ static void pc_build_feature_control_file(PCMachineState *pcms)
 
     val = g_malloc(sizeof(*val));
     *val = cpu_to_le64(feature_control_bits | FEATURE_CONTROL_LOCKED);
-    fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
+    fw_cfg_add_file(fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
 }
 
 static void rtc_set_cpus_count(ISADevice *rtc, uint16_t cpus_count)

@@ -1590,7 +1591,7 @@ void pc_machine_done(Notifier *notifier, void *data)
     acpi_setup();
     if (pcms->fw_cfg) {
         fw_cfg_build_smbios(MACHINE(pcms), pcms->fw_cfg);
-        pc_build_feature_control_file(pcms);
+        pc_build_feature_control_file(pcms, pcms->fw_cfg);
         /* update FW_CFG_NB_CPUS to account for -device added CPUs */
         fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);
     }

Signature · 1 line

2.20.1

Philippe Mathieu-Daudé

author

2019-07-01 13:37 UTC #15 lore raw

PATCH v3 13/15 hw/i386/pc: Let pc_build_feature_control() take a MachineState argument

In reply to Philippe Mathieu-Daudé

Let the pc_build_feature_control_file() function take a generic MachineState argument.

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/i386/pc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

hw/i386/pc.c +2 -3

index c49617a3f1..2ab2c637fd 100644
@@ -1518,10 +1518,9 @@ void pc_cpus_init(PCMachineState *pcms)
     }
 }
 
-static void pc_build_feature_control_file(PCMachineState *pcms,
+static void pc_build_feature_control_file(MachineState *ms,
                                           FWCfgState *fw_cfg)
 {
-    MachineState *ms = MACHINE(pcms);
     X86CPU *cpu = X86_CPU(ms->possible_cpus->cpus[0].cpu);
     CPUX86State *env = &cpu->env;
     uint32_t unused, ecx, edx;

@@ -1591,7 +1590,7 @@ void pc_machine_done(Notifier *notifier, void *data)
     acpi_setup();
     if (pcms->fw_cfg) {
         fw_cfg_build_smbios(MACHINE(pcms), pcms->fw_cfg);
-        pc_build_feature_control_file(pcms, pcms->fw_cfg);
+        pc_build_feature_control_file(MACHINE(pcms), pcms->fw_cfg);
         /* update FW_CFG_NB_CPUS to account for -device added CPUs */
         fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);
     }

Signature · 1 line

2.20.1

Philippe Mathieu-Daudé

author

2019-07-01 13:37 UTC #16 lore raw

PATCH v3 14/15 hw/i386/pc: Rename pc_build_feature_control() as generic fw_cfg_build_*

In reply to Philippe Mathieu-Daudé

Now that the pc_build_feature_control_file() function has been refactored to not depend of PC specific types, rename it to a more generic name.

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/i386/pc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

hw/i386/pc.c +3 -3

index 2ab2c637fd..ea895d0192 100644
@@ -1518,8 +1518,8 @@ void pc_cpus_init(PCMachineState *pcms)
     }
 }
 
-static void pc_build_feature_control_file(MachineState *ms,
-                                          FWCfgState *fw_cfg)
+static void fw_cfg_build_feature_control(MachineState *ms,
+                                         FWCfgState *fw_cfg)
 {
     X86CPU *cpu = X86_CPU(ms->possible_cpus->cpus[0].cpu);
     CPUX86State *env = &cpu->env;

@@ -1590,7 +1590,7 @@ void pc_machine_done(Notifier *notifier, void *data)
     acpi_setup();
     if (pcms->fw_cfg) {
         fw_cfg_build_smbios(MACHINE(pcms), pcms->fw_cfg);
-        pc_build_feature_control_file(MACHINE(pcms), pcms->fw_cfg);
+        fw_cfg_build_feature_control(MACHINE(pcms), pcms->fw_cfg);
         /* update FW_CFG_NB_CPUS to account for -device added CPUs */
         fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);
     }

Signature · 1 line

2.20.1

Philippe Mathieu-Daudé

author

2019-07-01 13:37 UTC #17 lore raw

PATCH v3 15/15 hw/i386/pc: Extract the x86 generic fw_cfg code

In reply to Philippe Mathieu-Daudé

Extract all the functions that are not PC-machine specific into the (arch-specific) fw_cfg.c file. This will allow other X86-machine to reuse these functions.

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/i386/fw_cfg.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/i386/fw_cfg.h |   8 +++
 hw/i386/pc.c     | 130 +-------------------------------------------
 3 files changed, 146 insertions(+), 129 deletions(-)

hw/i386/fw_cfg.c +137 -0

index 380a819230..b033d99bc4 100644
@@ -13,8 +13,15 @@
  */
 
 #include "qemu/osdep.h"
+#include "sysemu/numa.h"
+#include "hw/acpi/acpi.h"
+#include "hw/firmware/smbios.h"
+#include "hw/i386/pc.h"
 #include "hw/i386/fw_cfg.h"
+#include "hw/timer/hpet.h"
 #include "hw/nvram/fw_cfg.h"
+#include "e820_memory_layout.h"
+#include "kvm_i386.h"
 
 const char *fw_cfg_arch_key_name(uint16_t key)
 {

@@ -36,3 +43,133 @@ const char *fw_cfg_arch_key_name(uint16_t key)
     }
     return NULL;
 }
+
+FWCfgState *fw_cfg_arch_create(MachineState *ms,
+                               const CPUArchIdList *cpus,
+                               uint16_t boot_cpus,
+                               uint16_t apic_id_limit)
+{
+    FWCfgState *fw_cfg;
+    uint64_t *numa_fw_cfg;
+    int i;
+
+    fw_cfg = fw_cfg_init_io_dma(FW_CFG_IO_BASE, FW_CFG_IO_BASE + 4,
+                                &address_space_memory);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, boot_cpus);
+
+    /*
+     * FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
+     *
+     * For machine types prior to 1.8, SeaBIOS needs FW_CFG_MAX_CPUS for
+     * building MPTable, ACPI MADT, ACPI CPU hotplug and ACPI SRAT table,
+     * that tables are based on xAPIC ID and QEMU<->SeaBIOS interface
+     * for CPU hotplug also uses APIC ID and not "CPU index".
+     * This means that FW_CFG_MAX_CPUS is not the "maximum number of CPUs",
+     * but the "limit to the APIC ID values SeaBIOS may see".
+     *
+     * So for compatibility reasons with old BIOSes we are stuck with
+     * "etc/max-cpus" actually being apic_id_limit
+     */
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, apic_id_limit);
+    fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES,
+                     acpi_tables, acpi_tables_len);
+    fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, kvm_allows_irq0_override());
+
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE,
+                     &e820_reserve, sizeof(e820_reserve));
+    fw_cfg_add_file(fw_cfg, "etc/e820", e820_table,
+                    sizeof(struct e820_entry) * e820_get_num_entries());
+
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_cfg, sizeof(hpet_cfg));
+    /*
+     * allocate memory for the NUMA channel: one (64bit) word for the number
+     * of nodes, one word for each VCPU->node and one word for each node to
+     * hold the amount of memory.
+     */
+    numa_fw_cfg = g_new0(uint64_t, 1 + apic_id_limit + nb_numa_nodes);
+    numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes);
+    for (i = 0; i < cpus->len; i++) {
+        unsigned int apic_id = cpus->cpus[i].arch_id;
+        assert(apic_id < apic_id_limit);
+        numa_fw_cfg[apic_id + 1] = cpu_to_le64(cpus->cpus[i].props.node_id);
+    }
+    for (i = 0; i < nb_numa_nodes; i++) {
+        numa_fw_cfg[apic_id_limit + 1 + i] =
+            cpu_to_le64(numa_info[i].node_mem);
+    }
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA, numa_fw_cfg,
+                     (1 + apic_id_limit + nb_numa_nodes) *
+                     sizeof(*numa_fw_cfg));
+
+    return fw_cfg;
+}
+
+void fw_cfg_build_smbios(MachineState *ms, FWCfgState *fw_cfg)
+{
+    uint8_t *smbios_tables, *smbios_anchor;
+    size_t smbios_tables_len, smbios_anchor_len;
+    struct smbios_phys_mem_area *mem_array;
+    unsigned i, array_count;
+    X86CPU *cpu = X86_CPU(ms->possible_cpus->cpus[0].cpu);
+
+    /* tell smbios about cpuid version and features */
+    smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
+
+    smbios_tables = smbios_get_table_legacy(&smbios_tables_len);
+    if (smbios_tables) {
+        fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
+                         smbios_tables, smbios_tables_len);
+    }
+
+    /* build the array of physical mem area from e820 table */
+    mem_array = g_malloc0(sizeof(*mem_array) * e820_get_num_entries());
+    for (i = 0, array_count = 0; i < e820_get_num_entries(); i++) {
+        uint64_t addr, len;
+
+        if (e820_get_entry(i, E820_RAM, &addr, &len)) {
+            mem_array[array_count].address = addr;
+            mem_array[array_count].length = len;
+            array_count++;
+        }
+    }
+    smbios_get_tables(mem_array, array_count,
+                      &smbios_tables, &smbios_tables_len,
+                      &smbios_anchor, &smbios_anchor_len);
+    g_free(mem_array);
+
+    if (smbios_anchor) {
+        fw_cfg_add_file(fw_cfg, "etc/smbios/smbios-tables",
+                        smbios_tables, smbios_tables_len);
+        fw_cfg_add_file(fw_cfg, "etc/smbios/smbios-anchor",
+                        smbios_anchor, smbios_anchor_len);
+    }
+}
+
+void fw_cfg_build_feature_control(MachineState *ms, FWCfgState *fw_cfg)
+{
+    X86CPU *cpu = X86_CPU(ms->possible_cpus->cpus[0].cpu);
+    CPUX86State *env = &cpu->env;
+    uint32_t unused, ecx, edx;
+    uint64_t feature_control_bits = 0;
+    uint64_t *val;
+
+    cpu_x86_cpuid(env, 1, 0, &unused, &unused, &ecx, &edx);
+    if (ecx & CPUID_EXT_VMX) {
+        feature_control_bits |= FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
+    }
+
+    if ((edx & (CPUID_EXT2_MCE | CPUID_EXT2_MCA)) ==
+        (CPUID_EXT2_MCE | CPUID_EXT2_MCA) &&
+        (env->mcg_cap & MCG_LMCE_P)) {
+        feature_control_bits |= FEATURE_CONTROL_LMCE;
+    }
+
+    if (!feature_control_bits) {
+        return;
+    }
+
+    val = g_malloc(sizeof(*val));
+    *val = cpu_to_le64(feature_control_bits | FEATURE_CONTROL_LOCKED);
+    fw_cfg_add_file(fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
+}

hw/i386/fw_cfg.h +8 -0

index 17a4bc32f2..f9047a74e8 100644
@@ -9,6 +9,7 @@
 #ifndef HW_I386_FW_CFG_H
 #define HW_I386_FW_CFG_H
 
+#include "hw/boards.h"
 #include "hw/nvram/fw_cfg.h"
 
 #define FW_CFG_ACPI_TABLES      (FW_CFG_ARCH_LOCAL + 0)

@@ -17,4 +18,11 @@
 #define FW_CFG_E820_TABLE       (FW_CFG_ARCH_LOCAL + 3)
 #define FW_CFG_HPET             (FW_CFG_ARCH_LOCAL + 4)
 
+FWCfgState *fw_cfg_arch_create(MachineState *ms,
+                               const CPUArchIdList *cpus,
+                               uint16_t boot_cpus,
+                               uint16_t apic_id_limit);
+void fw_cfg_build_smbios(MachineState *ms, FWCfgState *fw_cfg);
+void fw_cfg_build_feature_control(MachineState *ms, FWCfgState *fw_cfg);
+
 #endif

hw/i386/pc.c +1 ~~-129~~

148 lines not on this page · raw message

Signature · 1 line

2.20.1