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

PATCH Superseded Philippe Mathieu-Daudé 3 participants 33 messages May 24, 2019 – May 30, 2019 KVM

lore

Share & tools

lore

Philippe Mathieu-Daudé

author

2019-05-24 06:37 UTC #0 lore raw

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 [*] 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.

Regards,

Phil.

[*] https://github.com/intel/nemu/commit/3cb92d080835ac8d47c8b713156338afa33cff5c

Philippe Mathieu-Daudé (20):
hw/i386/pc: Use unsigned type to index arrays hw/i386/pc: Use size_t type to hold/return a size of array hw/i386/pc: Let e820_add_entry() return a ssize_t type hw/i386/pc: Add the E820Type enum type
hw/i386/pc: Add documentation to the e820_*() functions 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() more generic as

    x86_create_fw_cfg()
  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 |  62 +++++++++++
 hw/i386/e820_memory_layout.h |  76 +++++++++++++
 hw/i386/fw_cfg.c             | 135 +++++++++++++++++++++++
 hw/i386/fw_cfg.h             |   6 ++
 hw/i386/pc.c                 | 201 ++---------------------------------
 include/hw/i386/pc.h         |  11 --
 target/i386/kvm.c            |   1 +
 8 files changed, 289 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-05-24 06:40 UTC #1 lore raw

PATCH 1/20 hw/i386/pc: Use unsigned type to index arrays

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

---
 hw/i386/pc.c         | 5 +++--
 include/hw/i386/pc.h | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

hw/i386/pc.c +3 -2

index 2632b73f80..fc38b59d2d 100644
@@ -870,7 +870,7 @@ static void handle_a20_line_change(void *opaque, int irq, int level)
 
 int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
 {
-    int index = le32_to_cpu(e820_reserve.count);
+    unsigned int index = le32_to_cpu(e820_reserve.count);
     struct e820_entry *entry;
 
     if (type != E820_RAM) {

@@ -902,7 +902,8 @@ int e820_get_num_entries(void)
     return e820_entries;
 }
 
-bool e820_get_entry(int idx, uint32_t type, uint64_t *address, uint64_t *length)
+bool e820_get_entry(unsigned int idx, uint32_t type,
+                    uint64_t *address, uint64_t *length)
 {
     if (idx < e820_entries && e820_table[idx].type == cpu_to_le32(type)) {
         *address = le64_to_cpu(e820_table[idx].address);

include/hw/i386/pc.h +1 -1

index 43df7230a2..ad3a75d8fa 100644
@@ -291,7 +291,7 @@ void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
 
 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 *);
+bool e820_get_entry(unsigned int, uint32_t, uint64_t *, uint64_t *);
 
 extern GlobalProperty pc_compat_4_0[];
 extern const size_t pc_compat_4_0_len;

Signature · 1 line

2.20.1

Li Qiang

Reviewed-by on v1 1/20

2019-05-24 12:13 UTC #2 lore raw

Re: [Qemu-devel] [PATCH 01/20] hw/i386/pc: Use unsigned type to index arrays

Quoted 2 lines · Philippe Mathieu-Daudé <[email protected]> 于2019年5月24日周五下午2:40写道：

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

Reviewed-by Li Qiang <[email protected]>

Quoted 48 lines

---
 hw/i386/pc.c         | 5 +++--
 include/hw/i386/pc.h | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

hw/i386/pc.c +1 -1

index 2632b73f80..fc38b59d2d 100644
@@ -870,7 +870,7 @@ static void handle_a20_line_change(void *opaque, int
 irq, int level)
 
 int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
 {
-    int index = le32_to_cpu(e820_reserve.count);
+    unsigned int index = le32_to_cpu(e820_reserve.count);
     struct e820_entry *entry;

     if (type != E820_RAM) {

hw/i386/pc.c +2 -1

@@ -902,7 +902,8 @@ int e820_get_num_entries(void)
     return e820_entries;
 }
 
-bool e820_get_entry(int idx, uint32_t type, uint64_t *address, uint64_t
 *length)
+bool e820_get_entry(unsigned int idx, uint32_t type,
+                    uint64_t *address, uint64_t *length)
 {
     if (idx < e820_entries && e820_table[idx].type == cpu_to_le32(type)) {

         *address = le64_to_cpu(e820_table[idx].address);

include/hw/i386/pc.h +1 -2

index 43df7230a2..ad3a75d8fa 100644
@@ -291,7 +291,7 @@ void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
 
 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 *);
+bool e820_get_entry(unsigned int, uint32_t, uint64_t *, uint64_t *);
 
 extern GlobalProperty pc_compat_4_0[];
 extern const size_t pc_compat_4_0_len;
--

2.20.1

Philippe Mathieu-Daudé

author

2019-05-24 06:38 UTC #3 lore raw

PATCH 2/20 hw/i386/pc: Use size_t type to hold/return a size of array

In reply to Philippe Mathieu-Daudé

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

---
 hw/i386/pc.c         | 4 ++--
 include/hw/i386/pc.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

hw/i386/pc.c +2 -2

index fc38b59d2d..df8600ac24 100644
@@ -104,7 +104,7 @@ struct e820_table {
 
 static struct e820_table e820_reserve;
 static struct e820_entry *e820_table;
-static unsigned e820_entries;
+static size_t e820_entries;
 struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
 
 /* Physical Address of PVH entry point read from kernel ELF NOTE */

@@ -897,7 +897,7 @@ int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
     return e820_entries;
 }
 
-int e820_get_num_entries(void)
+size_t e820_get_num_entries(void)
 {
     return e820_entries;
 }

include/hw/i386/pc.h +1 -1

index ad3a75d8fa..28b19173b0 100644
@@ -290,7 +290,7 @@ void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
 #define E820_UNUSABLE   5
 
 int e820_add_entry(uint64_t, uint64_t, uint32_t);
-int e820_get_num_entries(void);
+size_t e820_get_num_entries(void);
 bool e820_get_entry(unsigned int, uint32_t, uint64_t *, uint64_t *);
 
 extern GlobalProperty pc_compat_4_0[];

Signature · 1 line

2.20.1

Li Qiang

Reviewed-by on v1 2/20

2019-05-24 12:11 UTC #4 lore raw

Re: [Qemu-devel] [PATCH 02/20] hw/i386/pc: Use size_t type to hold/return a size of array

Quoted 2 lines · Philippe Mathieu-Daudé <[email protected]> 于2019年5月24日周五下午2:37写道：

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

Reviewed-by Li Qiang <[email protected]>

Quoted 46 lines

---
 hw/i386/pc.c         | 4 ++--
 include/hw/i386/pc.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

hw/i386/pc.c +2 -2

index fc38b59d2d..df8600ac24 100644
@@ -104,7 +104,7 @@ struct e820_table {
 
 static struct e820_table e820_reserve;
 static struct e820_entry *e820_table;
-static unsigned e820_entries;
+static size_t e820_entries;
 struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
 
 /* Physical Address of PVH entry point read from kernel ELF NOTE */

@@ -897,7 +897,7 @@ int e820_add_entry(uint64_t address, uint64_t length,
 uint32_t type)
     return e820_entries;
 }
 
-int e820_get_num_entries(void)
+size_t e820_get_num_entries(void)
 {
     return e820_entries;

}

include/hw/i386/pc.h +1 -2

index ad3a75d8fa..28b19173b0 100644
@@ -290,7 +290,7 @@ void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
 #define E820_UNUSABLE   5
 
 int e820_add_entry(uint64_t, uint64_t, uint32_t);
-int e820_get_num_entries(void);
+size_t e820_get_num_entries(void);
 bool e820_get_entry(unsigned int, uint32_t, uint64_t *, uint64_t *);
 
 extern GlobalProperty pc_compat_4_0[];
--

2.20.1

Philippe Mathieu-Daudé

author

2019-05-24 06:43 UTC #5 lore raw

PATCH 3/20 hw/i386/pc: Let e820_add_entry() return a ssize_t type

In reply to Philippe Mathieu-Daudé

e820_add_entry() returns an array size on success, or a negative value on error.

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

---
 hw/i386/pc.c         | 2 +-
 include/hw/i386/pc.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

hw/i386/pc.c +1 -1

index df8600ac24..1245028dd6 100644
@@ -868,7 +868,7 @@ static void handle_a20_line_change(void *opaque, int irq, int level)
     x86_cpu_set_a20(cpu, level);
 }
 
-int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
+ssize_t e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
 {
     unsigned int index = le32_to_cpu(e820_reserve.count);
     struct e820_entry *entry;

include/hw/i386/pc.h +1 -1

index 28b19173b0..2bc48c03c6 100644
@@ -289,7 +289,7 @@ void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
 #define E820_NVS        4
 #define E820_UNUSABLE   5
 
-int e820_add_entry(uint64_t, uint64_t, uint32_t);
+ssize_t e820_add_entry(uint64_t, uint64_t, uint32_t);
 size_t e820_get_num_entries(void);
 bool e820_get_entry(unsigned int, uint32_t, uint64_t *, uint64_t *);

Signature · 1 line

2.20.1

Li Qiang

Reviewed-by on v1 3/20

2019-05-24 12:11 UTC #6 lore raw

Re: [Qemu-devel] [PATCH 03/20] hw/i386/pc: Let e820_add_entry() return a ssize_t type

Quoted 5 lines · Philippe Mathieu-Daudé <[email protected]> 于2019年5月24日周五下午2:43写道：

e820_add_entry() returns an array size on success, or a negative value on error.
Signed-off-by Philippe Mathieu-Daudé <[email protected]>

Reviewed-by Li Qiang <[email protected]>

Quoted 37 lines

---
 hw/i386/pc.c         | 2 +-
 include/hw/i386/pc.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

hw/i386/pc.c +1 -1

index df8600ac24..1245028dd6 100644
@@ -868,7 +868,7 @@ static void handle_a20_line_change(void *opaque, int
 irq, int level)
     x86_cpu_set_a20(cpu, level);
 }
 
-int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
+ssize_t e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
 {
     unsigned int index = le32_to_cpu(e820_reserve.count);

     struct e820_entry *entry;

include/hw/i386/pc.h +1 -2

index 28b19173b0..2bc48c03c6 100644
@@ -289,7 +289,7 @@ void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
 #define E820_NVS        4
 #define E820_UNUSABLE   5
 
-int e820_add_entry(uint64_t, uint64_t, uint32_t);
+ssize_t e820_add_entry(uint64_t, uint64_t, uint32_t);
 size_t e820_get_num_entries(void);
 bool e820_get_entry(unsigned int, uint32_t, uint64_t *, uint64_t *);
 
--

2.20.1

Philippe Mathieu-Daudé

author

2019-05-24 06:46 UTC #7 lore raw

PATCH 4/20 hw/i386/pc: Add the E820Type enum type

In reply to Philippe Mathieu-Daudé

This ensure we won't use an incorrect value.

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

---
 hw/i386/pc.c         | 12 +++++++-----
 include/hw/i386/pc.h | 16 ++++++++++------
 2 files changed, 17 insertions(+), 11 deletions(-)

hw/i386/pc.c +7 -5

index 1245028dd6..ac8343c728 100644
@@ -868,9 +868,10 @@ static void handle_a20_line_change(void *opaque, int irq, int level)
     x86_cpu_set_a20(cpu, level);
 }
 
-ssize_t e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
+ssize_t e820_add_entry(uint64_t address, uint64_t length, E820Type type)
 {
     unsigned int index = le32_to_cpu(e820_reserve.count);
+    uint32_t utype = (uint32_t)type;
     struct e820_entry *entry;
 
     if (type != E820_RAM) {

@@ -882,7 +883,7 @@ ssize_t e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
 
         entry->address = cpu_to_le64(address);
         entry->length = cpu_to_le64(length);
-        entry->type = cpu_to_le32(type);
+        entry->type = cpu_to_le32(utype);
 
         e820_reserve.count = cpu_to_le32(index);
     }

@@ -891,7 +892,7 @@ ssize_t e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
     e820_table = g_renew(struct e820_entry, e820_table, e820_entries + 1);
     e820_table[e820_entries].address = cpu_to_le64(address);
     e820_table[e820_entries].length = cpu_to_le64(length);
-    e820_table[e820_entries].type = cpu_to_le32(type);
+    e820_table[e820_entries].type = cpu_to_le32(utype);
     e820_entries++;
 
     return e820_entries;

@@ -902,10 +903,11 @@ size_t e820_get_num_entries(void)
     return e820_entries;
 }
 
-bool e820_get_entry(unsigned int idx, uint32_t type,
+bool e820_get_entry(unsigned int idx, E820Type type,
                     uint64_t *address, uint64_t *length)
 {
-    if (idx < e820_entries && e820_table[idx].type == cpu_to_le32(type)) {
+    uint32_t utype = (uint32_t)type;
+    if (idx < e820_entries && e820_table[idx].type == cpu_to_le32(utype)) {
         *address = le64_to_cpu(e820_table[idx].address);
         *length = le64_to_cpu(e820_table[idx].length);
         return true;

include/hw/i386/pc.h +10 -6

index 2bc48c03c6..10e77a40ce 100644
@@ -282,12 +282,16 @@ 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
+/**
+ * E820Type: Type of the e820 address range.
+ */
+typedef enum {
+    E820_RAM        = 1,
+    E820_RESERVED   = 2,
+    E820_ACPI       = 3,
+    E820_NVS        = 4,
+    E820_UNUSABLE   = 5
+} E820Type;
 
 ssize_t e820_add_entry(uint64_t, uint64_t, uint32_t);
 size_t e820_get_num_entries(void);

Signature · 1 line

2.20.1

Li Qiang

2019-05-24 12:36 UTC #8 lore raw

Re: [Qemu-devel] [PATCH 04/20] hw/i386/pc: Add the E820Type enum type

Quoted 2 lines · Philippe Mathieu-Daudé <[email protected]> 于2019年5月24日周五下午2:45写道：

This ensure we won't use an incorrect value.

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

Quoted 10 lines

---
 hw/i386/pc.c         | 12 +++++++-----
 include/hw/i386/pc.h | 16 ++++++++++------
 2 files changed, 17 insertions(+), 11 deletions(-)

hw/i386/pc.c +0 -0

index 1245028dd6..ac8343c728 100644

hw/i386/pc.c static void handle_a20_line_change(void *opaque, int · line 876

Quoted 10 lines

hw/i386/pc.c +2 -1

@@ -868,9 +868,10 @@ static void handle_a20_line_change(void *opaque, int
 irq, int level)
     x86_cpu_set_a20(cpu, level);
 }
 
-ssize_t e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
+ssize_t e820_add_entry(uint64_t address, uint64_t length, E820Type type)
 {
     unsigned int index = le32_to_cpu(e820_reserve.count);
+    uint32_t utype = (uint32_t)type;

I don't have strong opinion for this, as I don't like add an explicit conversion.

Quoted 71 lines

hw/i386/pc.c +0 -0

     struct e820_entry *entry;

     if (type != E820_RAM) {

hw/i386/pc.c +1 -1

@@ -882,7 +883,7 @@ ssize_t e820_add_entry(uint64_t address, uint64_t
 length, uint32_t type)
 
         entry->address = cpu_to_le64(address);
         entry->length = cpu_to_le64(length);
-        entry->type = cpu_to_le32(type);
+        entry->type = cpu_to_le32(utype);
 
         e820_reserve.count = cpu_to_le32(index);

hw/i386/pc.c +1 -1

@@ -891,7 +892,7 @@ ssize_t e820_add_entry(uint64_t address, uint64_t
 length, uint32_t type)
     e820_table = g_renew(struct e820_entry, e820_table, e820_entries + 1);
     e820_table[e820_entries].address = cpu_to_le64(address);
     e820_table[e820_entries].length = cpu_to_le64(length);
-    e820_table[e820_entries].type = cpu_to_le32(type);
+    e820_table[e820_entries].type = cpu_to_le32(utype);
     e820_entries++;

     return e820_entries;

hw/i386/pc.c +3 -2

@@ -902,10 +903,11 @@ size_t e820_get_num_entries(void)
     return e820_entries;
 }
 
-bool e820_get_entry(unsigned int idx, uint32_t type,
+bool e820_get_entry(unsigned int idx, E820Type type,
                     uint64_t *address, uint64_t *length)
 {
-    if (idx < e820_entries && e820_table[idx].type == cpu_to_le32(type)) {
+    uint32_t utype = (uint32_t)type;
+    if (idx < e820_entries && e820_table[idx].type == cpu_to_le32(utype))
 {
         *address = le64_to_cpu(e820_table[idx].address);
         *length = le64_to_cpu(e820_table[idx].length);

         return true;

include/hw/i386/pc.h +10 -6

index 2bc48c03c6..10e77a40ce 100644
@@ -282,12 +282,16 @@ 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
+/**
+ * E820Type: Type of the e820 address range.
+ */
+typedef enum {
+    E820_RAM        = 1,
+    E820_RESERVED   = 2,
+    E820_ACPI       = 3,
+    E820_NVS        = 4,
+    E820_UNUSABLE   = 5
+} E820Type;
 
 ssize_t e820_add_entry(uint64_t, uint64_t, uint32_t);

size_t e820_get_num_entries(void);
--
2.20.1

Philippe Mathieu-Daudé

author

2019-05-30 16:35 UTC #9 lore raw

Re: [Qemu-devel] [PATCH 04/20] hw/i386/pc: Add the E820Type enum type

Hi Li,

Quoted 30 lines · Li Qiang

Philippe Mathieu-Daudé <[email protected] <mailto:[email protected]>> 于 2019年5月24日周五下午2:45写道：

    This ensure we won't use an incorrect value.

    Signed-off-by: Philippe Mathieu-Daudé <[email protected]
    <mailto:[email protected]>>
    ---
     hw/i386/pc.c         | 12 +++++++-----
     include/hw/i386/pc.h | 16 ++++++++++------
     2 files changed, 17 insertions(+), 11 deletions(-)

    diff --git a/hw/i386/pc.c b/hw/i386/pc.c
    index 1245028dd6..ac8343c728 100644
    --- a/hw/i386/pc.c
    +++ b/hw/i386/pc.c
    @@ -868,9 +868,10 @@ static void handle_a20_line_change(void
    *opaque, int irq, int level)
         x86_cpu_set_a20(cpu, level);
     }

    -ssize_t e820_add_entry(uint64_t address, uint64_t length, uint32_t
    type)
    +ssize_t e820_add_entry(uint64_t address, uint64_t length, E820Type
    type)
     {
         unsigned int index = le32_to_cpu(e820_reserve.count);
    +    uint32_t utype = (uint32_t)type;

Quoted 2 lines

I don't have strong opinion for this, as I don't like add an explicit conversion.

Usually I try to not over-cast, but I guess remember I added that because some Clang build was failing, but I started a build on Travis-CI and all passed, so I might have been trying in a local build directory with stricter CPPFLAGS.
I'll clean that out.

Thanks for your review of this series!

Phil.

Quoted 72 lines

         struct e820_entry *entry;

         if (type != E820_RAM) {
    @@ -882,7 +883,7 @@ ssize_t e820_add_entry(uint64_t address,
    uint64_t length, uint32_t type)

             entry->address = cpu_to_le64(address);
             entry->length = cpu_to_le64(length);
    -        entry->type = cpu_to_le32(type);
    +        entry->type = cpu_to_le32(utype);

             e820_reserve.count = cpu_to_le32(index);
         }
    @@ -891,7 +892,7 @@ ssize_t e820_add_entry(uint64_t address,
    uint64_t length, uint32_t type)
         e820_table = g_renew(struct e820_entry, e820_table,
    e820_entries + 1);
         e820_table[e820_entries].address = cpu_to_le64(address);
         e820_table[e820_entries].length = cpu_to_le64(length);
    -    e820_table[e820_entries].type = cpu_to_le32(type);
    +    e820_table[e820_entries].type = cpu_to_le32(utype);
         e820_entries++;

         return e820_entries;
    @@ -902,10 +903,11 @@ size_t e820_get_num_entries(void)
         return e820_entries;
     }

    -bool e820_get_entry(unsigned int idx, uint32_t type,
    +bool e820_get_entry(unsigned int idx, E820Type type,
                         uint64_t *address, uint64_t *length)
     {
    -    if (idx < e820_entries && e820_table[idx].type ==
    cpu_to_le32(type)) {
    +    uint32_t utype = (uint32_t)type;
    +    if (idx < e820_entries && e820_table[idx].type ==
    cpu_to_le32(utype)) {
             *address = le64_to_cpu(e820_table[idx].address);
             *length = le64_to_cpu(e820_table[idx].length);
             return true;
    diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
    index 2bc48c03c6..10e77a40ce 100644
    --- a/include/hw/i386/pc.h
    +++ b/include/hw/i386/pc.h
    @@ -282,12 +282,16 @@ 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
    +/**
    + * E820Type: Type of the e820 address range.
    + */
    +typedef enum {
    +    E820_RAM        = 1,
    +    E820_RESERVED   = 2,
    +    E820_ACPI       = 3,
    +    E820_NVS        = 4,
    +    E820_UNUSABLE   = 5
    +} E820Type;

     ssize_t e820_add_entry(uint64_t, uint64_t, uint32_t);
     size_t e820_get_num_entries(void);
    -- 
    2.20.1

Philippe Mathieu-Daudé

author

2019-05-24 06:41 UTC #10 lore raw

PATCH 5/20 hw/i386/pc: Add documentation to the e820_*() functions

In reply to Philippe Mathieu-Daudé

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

---
 include/hw/i386/pc.h | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

include/hw/i386/pc.h +35 -2

index 10e77a40ce..95bf3278f2 100644
@@ -293,9 +293,42 @@ typedef enum {
     E820_UNUSABLE   = 5
 } E820Type;
 
-ssize_t e820_add_entry(uint64_t, uint64_t, uint32_t);
+/**
+ * e820_add_entry: Add an #e820_entry to the @e820_table.
+ *
+ * Returns the number of entries of the e820_table on success,
+ *         or a negative errno otherwise.
+ *
+ * @address: The base address of the structure which the BIOS is to fill in.
+ * @length: The length in bytes of the structure passed to the BIOS.
+ * @type: The #E820Type of the address range.
+ */
+ssize_t e820_add_entry(uint64_t address, uint64_t length, E820Type type);
+
+/**
+ * e820_get_num_entries: The number of entries of the @e820_table.
+ *
+ * Returns the number of entries of the e820_table.
+ */
 size_t e820_get_num_entries(void);
-bool e820_get_entry(unsigned int, uint32_t, uint64_t *, uint64_t *);
+
+/**
+ * e820_get_entry: Get the address/length of an #e820_entry.
+ *
+ * If the #e820_entry stored at @index is of #E820Type @type, fills @address
+ * and @length with the #e820_entry values and return @true.
+ * Return @false otherwise.
+ *
+ * @index: The index of the #e820_entry to get values.
+ * @type: The @E820Type of the address range expected.
+ * @address: Pointer to the base address of the #e820_entry structure to
+ *           be filled.
+ * @length: Pointer to the length (in bytes) of the #e820_entry structure
+ *          to be filled.
+ * @return: true if the entry was found, false otherwise.
+ */
+bool e820_get_entry(unsigned int index, E820Type type,
+                    uint64_t *address, uint64_t *length);
 
 extern GlobalProperty pc_compat_4_0[];
 extern const size_t pc_compat_4_0_len;

Signature · 1 line

2.20.1

Philippe Mathieu-Daudé

author

2019-05-24 06:48 UTC #11 lore raw

PATCH 6/20 hw/i386/pc: Use e820_get_num_entries() to access e820_entries

In reply to Philippe Mathieu-Daudé

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

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 ac8343c728..2e195049a5 100644
@@ -1022,7 +1022,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

Li Qiang

Reviewed-by on v1 6/20

2019-05-24 13:14 UTC #12 lore raw

Re: [Qemu-devel] [PATCH 06/20] hw/i386/pc: Use e820_get_num_entries() to access e820_entries

Quoted 5 lines · Philippe Mathieu-Daudé <[email protected]> 于2019年5月24日周五下午2:48写道：

To be able to extract the e820* code out of this file (in the next patch), access e820_entries with its correct helper.
Signed-off-by Philippe Mathieu-Daudé <[email protected]>

Reviewed-by Li Qiang <[email protected]>

Quoted 24 lines

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

hw/i386/pc.c +1 -1

index ac8343c728..2e195049a5 100644
@@ -1022,7 +1022,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
--
2.20.1

Philippe Mathieu-Daudé

author

2019-05-24 06:37 UTC #13 lore raw

PATCH 7/20 hw/i386/pc: Extract e820 memory layout code

In reply to Philippe Mathieu-Daudé

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

---
 hw/i386/Makefile.objs        |  2 +-
 hw/i386/e820_memory_layout.c | 62 +++++++++++++++++++++++++++++
 hw/i386/e820_memory_layout.h | 76 ++++++++++++++++++++++++++++++++++++
 hw/i386/pc.c                 | 64 +-----------------------------
 include/hw/i386/pc.h         | 48 -----------------------
 target/i386/kvm.c            |  1 +
 6 files changed, 141 insertions(+), 112 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 +62 -0

62 lines not on this page · raw message

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

76 lines not on this page · raw message

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

82 lines not on this page · raw message

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

54 lines not on this page · raw message

target/i386/kvm.c +1 -0

index 3b29ce5c0d..dbf890005e 100644
@@ -39,6 +39,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

Li Qiang

Reviewed-by on v1 7/20

2019-05-24 13:28 UTC #14 lore raw

Re: [Qemu-devel] [PATCH 07/20] hw/i386/pc: Extract e820 memory layout code

Quoted 3 lines · Philippe Mathieu-Daudé <[email protected]> 于2019年5月24日周五下午2:41写道：

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

Reviewed-by Li Qiang <[email protected]>

Quoted 344 lines

---
 hw/i386/Makefile.objs        |  2 +-
 hw/i386/e820_memory_layout.c | 62 +++++++++++++++++++++++++++++
 hw/i386/e820_memory_layout.h | 76 ++++++++++++++++++++++++++++++++++++
 hw/i386/pc.c                 | 64 +-----------------------------
 include/hw/i386/pc.h         | 48 -----------------------
 target/i386/kvm.c            |  1 +
 6 files changed, 141 insertions(+), 112 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 +56 -0

new file mode 100644
index 0000000000..b9be08536c
@@ -0,0 +1,62 @@
+/*
+ * QEMU BIOS e820 routines
+ *
+ * Copyright (c) 2003-2004 Fabrice Bellard
+ *
+ * SPDX-License-Identifier: MIT
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/bswap.h"
+#include "e820_memory_layout.h"
+
+static size_t e820_entries;
+struct e820_table e820_reserve;
+struct e820_entry *e820_table;
+
+ssize_t e820_add_entry(uint64_t address, uint64_t length, E820Type type)
+{
+    unsigned int index = le32_to_cpu(e820_reserve.count);
+    uint32_t utype = (uint32_t)type;
+    struct e820_entry *entry;
+
+    if (type != E820_RAM) {
+        /* old FW_CFG_E820_TABLE entry -- reservations only */
+        if (index >= E820_NR_ENTRIES) {
+            return -EBUSY;
+        }
+        entry = &e820_reserve.entry[index++];
+
+        entry->address = cpu_to_le64(address);
+        entry->length = cpu_to_le64(length);
+        entry->type = cpu_to_le32(utype);
+
+        e820_reserve.count = cpu_to_le32(index);
+    }
+
+    /* new "etc/e820" file -- include ram too */
+    e820_table = g_renew(struct e820_entry, e820_table, e820_entries + 1);
+    e820_table[e820_entries].address = cpu_to_le64(address);
+    e820_table[e820_entries].length = cpu_to_le64(length);
+    e820_table[e820_entries].type = cpu_to_le32(utype);
+    e820_entries++;
+
+    return e820_entries;
+}
+
+size_t e820_get_num_entries(void)
+{
+    return e820_entries;
+}
+
+bool e820_get_entry(unsigned int idx, E820Type type,
+                    uint64_t *address, uint64_t *length)
+{
+    uint32_t utype = (uint32_t)type;
+    if (idx < e820_entries && e820_table[idx].type == cpu_to_le32(utype))

{

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

+        *address = le64_to_cpu(e820_table[idx].address);
+        *length = le64_to_cpu(e820_table[idx].length);
+        return true;
+    }
+    return false;
+}

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

new file mode 100644
index 0000000000..64e88e4772
@@ -0,0 +1,76 @@
+/*
+ * QEMU BIOS e820 routines
+ *
+ * Copyright (c) 2003-2004 Fabrice Bellard
+ *
+ * SPDX-License-Identifier: MIT
+ */
+
+#ifndef HW_I386_E820_H
+#define HW_I386_E820_H
+
+/**
+ * E820Type: Type of the e820 address range.
+ */
+typedef enum {
+    E820_RAM        = 1,
+    E820_RESERVED   = 2,
+    E820_ACPI       = 3,
+    E820_NVS        = 4,
+    E820_UNUSABLE   = 5
+} E820Type;
+
+#define E820_NR_ENTRIES 16
+
+struct e820_entry {
+    uint64_t address;
+    uint64_t length;
+    uint32_t type;
+} QEMU_PACKED __attribute((__aligned__(4)));
+
+struct e820_table {
+    uint32_t count;
+    struct e820_entry entry[E820_NR_ENTRIES];
+} QEMU_PACKED __attribute((__aligned__(4)));
+
+extern struct e820_table e820_reserve;
+extern struct e820_entry *e820_table;
+
+/**
+ * e820_add_entry: Add an #e820_entry to the @e820_table.
+ *
+ * Returns the number of entries of the e820_table on success,
+ *         or a negative errno otherwise.
+ *
+ * @address: The base address of the structure which the BIOS is to fill

in.
+ * @length: The length in bytes of the structure passed to the BIOS.
+ * @type: The #E820Type of the address range.
+ */

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

+ssize_t e820_add_entry(uint64_t address, uint64_t length, E820Type type);
+
+/**
+ * e820_get_num_entries: The number of entries of the @e820_table.
+ *
+ * Returns the number of entries of the e820_table.
+ */
+size_t e820_get_num_entries(void);
+
+/**
+ * e820_get_entry: Get the address/length of an #e820_entry.
+ *
+ * If the #e820_entry stored at @index is of #E820Type @type, fills

@address
+ * and @length with the #e820_entry values and return @true.
+ * Return @false otherwise.
+ *
+ * @index: The index of the #e820_entry to get values.
+ * @type: The @E820Type of the address range expected.
+ * @address: Pointer to the base address of the #e820_entry structure to
+ * be filled.
+ * @length: Pointer to the length (in bytes) of the #e820_entry structure
+ * to be filled.
+ * @return: true if the entry was found, false otherwise.
+ */

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

+bool e820_get_entry(unsigned int index, E820Type type,
+                    uint64_t *address, uint64_t *length);
+
+#endif

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

index 2e195049a5..fc22779ac1 100644
@@ -78,6 +78,7 @@
 #include "hw/i386/intel_iommu.h"
 #include "hw/net/ne2000-isa.h"
 #include "standard-headers/asm-x86/bootparam.h"
+#include "e820_memory_layout.h"
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ

@@ -89,22 +90,6 @@
 #define DPRINTF(fmt, ...)
 #endif
 
-#define E820_NR_ENTRIES                16
-
-struct e820_entry {
-    uint64_t address;
-    uint64_t length;
-    uint32_t type;
-} QEMU_PACKED __attribute((__aligned__(4)));
-
-struct e820_table {
-    uint32_t count;
-    struct e820_entry entry[E820_NR_ENTRIES];
-} QEMU_PACKED __attribute((__aligned__(4)));
-
-static struct e820_table e820_reserve;
-static struct e820_entry *e820_table;
-static size_t e820_entries;
 struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
 
 /* Physical Address of PVH entry point read from kernel ELF NOTE */

@@ -868,53 +853,6 @@ static void handle_a20_line_change(void *opaque, int
 irq, int level)
     x86_cpu_set_a20(cpu, level);
 }
 
-ssize_t e820_add_entry(uint64_t address, uint64_t length, E820Type type)
-{
-    unsigned int index = le32_to_cpu(e820_reserve.count);
-    uint32_t utype = (uint32_t)type;
-    struct e820_entry *entry;
-
-    if (type != E820_RAM) {
-        /* old FW_CFG_E820_TABLE entry -- reservations only */
-        if (index >= E820_NR_ENTRIES) {
-            return -EBUSY;
-        }
-        entry = &e820_reserve.entry[index++];
-
-        entry->address = cpu_to_le64(address);
-        entry->length = cpu_to_le64(length);
-        entry->type = cpu_to_le32(utype);
-
-        e820_reserve.count = cpu_to_le32(index);
-    }
-
-    /* new "etc/e820" file -- include ram too */
-    e820_table = g_renew(struct e820_entry, e820_table, e820_entries + 1);
-    e820_table[e820_entries].address = cpu_to_le64(address);
-    e820_table[e820_entries].length = cpu_to_le64(length);
-    e820_table[e820_entries].type = cpu_to_le32(utype);
-    e820_entries++;
-
-    return e820_entries;
-}
-
-size_t e820_get_num_entries(void)
-{
-    return e820_entries;
-}
-
-bool e820_get_entry(unsigned int idx, E820Type type,
-                    uint64_t *address, uint64_t *length)
-{
-    uint32_t utype = (uint32_t)type;
-    if (idx < e820_entries && e820_table[idx].type == cpu_to_le32(utype))
 {
-        *address = le64_to_cpu(e820_table[idx].address);
-        *length = le64_to_cpu(e820_table[idx].length);
-        return true;
-    }
-    return false;
-}
-
 /* Enables contiguous-apic-ID mode, for compatibility */

static bool compat_apic_id_mode;

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

index 95bf3278f2..0f1bf667ae 100644
@@ -282,54 +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);
 
-/**
- * E820Type: Type of the e820 address range.
- */
-typedef enum {
-    E820_RAM        = 1,
-    E820_RESERVED   = 2,
-    E820_ACPI       = 3,
-    E820_NVS        = 4,
-    E820_UNUSABLE   = 5
-} E820Type;
-
-/**
- * e820_add_entry: Add an #e820_entry to the @e820_table.
- *
- * Returns the number of entries of the e820_table on success,
- *         or a negative errno otherwise.
- *
- * @address: The base address of the structure which the BIOS is to fill

in.
- * @length: The length in bytes of the structure passed to the BIOS.
- * @type: The #E820Type of the address range.
- */

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

-ssize_t e820_add_entry(uint64_t address, uint64_t length, E820Type type);
-
-/**
- * e820_get_num_entries: The number of entries of the @e820_table.
- *
- * Returns the number of entries of the e820_table.
- */
-size_t e820_get_num_entries(void);
-
-/**
- * e820_get_entry: Get the address/length of an #e820_entry.
- *
- * If the #e820_entry stored at @index is of #E820Type @type, fills

@address
- * and @length with the #e820_entry values and return @true.
- * Return @false otherwise.
- *
- * @index: The index of the #e820_entry to get values.
- * @type: The @E820Type of the address range expected.
- * @address: Pointer to the base address of the #e820_entry structure to
- * be filled.
- * @length: Pointer to the length (in bytes) of the #e820_entry structure
- * to be filled.
- * @return: true if the entry was found, false otherwise.
- */

include/hw/i386/pc.h +0 -3

-bool e820_get_entry(unsigned int index, E820Type type,
-                    uint64_t *address, uint64_t *length);
-
 extern GlobalProperty pc_compat_4_0[];
 extern const size_t pc_compat_4_0_len;

target/i386/kvm.c +1 -1

index 3b29ce5c0d..dbf890005e 100644
@@ -39,6 +39,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"
--

2.20.1

Philippe Mathieu-Daudé

author

2019-05-24 06:39 UTC #15 lore raw

PATCH 8/20 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.

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 fc22779ac1..a3936bb29d 100644
@@ -928,7 +928,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;

@@ -936,7 +936,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:

@@ -1761,7 +1762,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

Li Qiang

Reviewed-by on v1 8/20

2019-05-24 13:38 UTC #16 lore raw

Re: [Qemu-devel] [PATCH 08/20] hw/i386/pc: Use address_space_memory in place

Quoted 6 lines · Philippe Mathieu-Daudé <[email protected]> 于2019年5月24日周五下午2:38写道：

The address_space_memory variable is used once.
Use it in place and remove the argument.
Suggested-by Samuel Ortiz <[email protected]>
Signed-off-by Philippe Mathieu-Daudé <[email protected]>

Reviewed-by Li Qiang <[email protected]>

Quoted 42 lines

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

hw/i386/pc.c +3 -2

index fc22779ac1..a3936bb29d 100644
@@ -928,7 +928,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;

@@ -936,7 +936,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:

hw/i386/pc.c +1 -2

@@ -1761,7 +1762,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);
 
--

2.20.1

Philippe Mathieu-Daudé

author

2019-05-24 06:46 UTC #17 lore raw

PATCH 9/20 hw/i386/pc: Rename bochs_bios_init() more generic as x86_create_fw_cfg()

In reply to Philippe Mathieu-Daudé

The bochs_bios_init() is not restricted to the Bochs BIOS and is useful to other BIOS. Rename it to be more generic.

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 a3936bb29d..264074489b 100644
@@ -928,7 +928,7 @@ static void pc_build_smbios(PCMachineState *pcms)
     }
 }
 
-static FWCfgState *bochs_bios_init(PCMachineState *pcms)
+static FWCfgState *x86_create_fw_cfg(PCMachineState *pcms)
 {
     FWCfgState *fw_cfg;
     uint64_t *numa_fw_cfg;

@@ -1508,7 +1508,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 x86_create_fw_cfg().
      */
     pcms->apic_id_limit = x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
     possible_cpus = mc->possible_cpu_arch_ids(ms);

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

Signature · 1 line

2.20.1

Li Qiang

Reviewed-by on v1 9/20

2019-05-24 13:36 UTC #18 lore raw

Re: [Qemu-devel] [PATCH 09/20] hw/i386/pc: Rename bochs_bios_init() more generic as x86_create_fw_cfg()

Quoted 6 lines · Philippe Mathieu-Daudé <[email protected]> 于2019年5月24日周五下午2:46写道：

The bochs_bios_init() is not restricted to the Bochs BIOS and is useful to other BIOS. Rename it to be more generic.
Suggested-by Samuel Ortiz <[email protected]>
Signed-off-by Philippe Mathieu-Daudé <[email protected]>

Reviewed-by Li Qiang <[email protected]>

Quoted 42 lines

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

hw/i386/pc.c +1 -2

index a3936bb29d..264074489b 100644
@@ -928,7 +928,7 @@ static void pc_build_smbios(PCMachineState *pcms)
     }
 }
 
-static FWCfgState *bochs_bios_init(PCMachineState *pcms)
+static FWCfgState *x86_create_fw_cfg(PCMachineState *pcms)
 {
     FWCfgState *fw_cfg;
     uint64_t *numa_fw_cfg;

@@ -1508,7 +1508,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
x86_create_fw_cfg().

      */
     pcms->apic_id_limit = x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
     possible_cpus = mc->possible_cpu_arch_ids(ms);

hw/i386/pc.c +1 -2

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

2.20.1

Philippe Mathieu-Daudé

author

2019-05-24 06:50 UTC #19 lore raw

PATCH 10/20 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 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

hw/i386/pc.c +3 -3

index 264074489b..01894b9875 100644
@@ -928,7 +928,7 @@ static void pc_build_smbios(PCMachineState *pcms)
     }
 }
 
-static FWCfgState *x86_create_fw_cfg(PCMachineState *pcms)
+static FWCfgState *x86_create_fw_cfg(PCMachineState *pcms, uint16_t boot_cpus)
 {
     FWCfgState *fw_cfg;
     uint64_t *numa_fw_cfg;

@@ -938,7 +938,7 @@ static FWCfgState *x86_create_fw_cfg(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:
      *

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

Signature · 1 line

2.20.1

Li Qiang

2019-05-24 15:38 UTC #20 lore raw

Re: [Qemu-devel] [PATCH 10/20] hw/i386/pc: Pass the boot_cpus value by argument

Quoted 14 lines · Philippe Mathieu-Daudé <[email protected]> 于2019年5月24日周五下午2:50写道：

The boot_cpus is used once. Pass it by argument, this will allow us to remove the PCMachineState argument later.
Suggested-by Samuel Ortiz <[email protected]>
Signed-off-by Philippe Mathieu-Daudé <[email protected]>
---
 hw/i386/pc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
hw/i386/pc.c +0 -0
index 264074489b..01894b9875 100644

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

Quoted 7 lines

hw/i386/pc.c +1 -1

@@ -928,7 +928,7 @@ static void pc_build_smbios(PCMachineState *pcms)
     }
 }
 
-static FWCfgState *x86_create_fw_cfg(PCMachineState *pcms)
+static FWCfgState *x86_create_fw_cfg(PCMachineState *pcms, uint16_t
 boot_cpus)

For the patches 10/11/12, I don't think this is an elegant solution. When we add more data like 'boot_cpus'
we need add more arguments?

Thanks,
Li Qiang

Quoted 27 lines

hw/i386/pc.c +0 -0

     FWCfgState *fw_cfg;
     uint64_t *numa_fw_cfg;

hw/i386/pc.c +1 -1

@@ -938,7 +938,7 @@ static FWCfgState *x86_create_fw_cfg(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:

hw/i386/pc.c +1 -2

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

2.20.1

Philippe Mathieu-Daudé

author

2019-05-30 15:21 UTC #21 lore raw

Re: [Qemu-devel] [PATCH 10/20] hw/i386/pc: Pass the boot_cpus value by argument

Quoted 27 lines · Li Qiang

Philippe Mathieu-Daudé <[email protected] <mailto:[email protected]>> 于 2019年5月24日周五下午2:50写道：

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

    Suggested-by: Samuel Ortiz <[email protected]
    <mailto:[email protected]>>
    Signed-off-by: Philippe Mathieu-Daudé <[email protected]
    <mailto:[email protected]>>
    ---
     hw/i386/pc.c | 6 +++---
     1 file changed, 3 insertions(+), 3 deletions(-)

    diff --git a/hw/i386/pc.c b/hw/i386/pc.c
    index 264074489b..01894b9875 100644
    --- a/hw/i386/pc.c
    +++ b/hw/i386/pc.c
    @@ -928,7 +928,7 @@ static void pc_build_smbios(PCMachineState *pcms)
         }
     }

    -static FWCfgState *x86_create_fw_cfg(PCMachineState *pcms)
    +static FWCfgState *x86_create_fw_cfg(PCMachineState *pcms, uint16_t
    boot_cpus)

Quoted 3 lines

For the patches 10/11/12, I don't think this is an elegant solution. When we add more data like 'boot_cpus'
we need add more arguments?

This fonction is called once at machine creation, so there is no performance penalty. To keep the code modularizable (reusable) it is an acceptable tradeoff :)

Quoted 27 lines

     {
         FWCfgState *fw_cfg;
         uint64_t *numa_fw_cfg;
    @@ -938,7 +938,7 @@ static FWCfgState
    *x86_create_fw_cfg(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:
          *
    @@ -1762,7 +1762,7 @@ void pc_memory_init(PCMachineState *pcms,
                                             option_rom_mr,
                                             1);

    -    fw_cfg = x86_create_fw_cfg(pcms);
    +    fw_cfg = x86_create_fw_cfg(pcms, pcms->boot_cpus);

         rom_set_fw(fw_cfg);

    -- 
    2.20.1

Philippe Mathieu-Daudé

author

2019-05-24 06:40 UTC #22 lore raw

PATCH 11/20 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 01894b9875..fe07baeb1d 100644
@@ -928,7 +928,8 @@ static void pc_build_smbios(PCMachineState *pcms)
     }
 }
 
-static FWCfgState *x86_create_fw_cfg(PCMachineState *pcms, uint16_t boot_cpus)
+static FWCfgState *x86_create_fw_cfg(PCMachineState *pcms,
+                                     uint16_t boot_cpus, uint16_t apic_id_limit)
 {
     FWCfgState *fw_cfg;
     uint64_t *numa_fw_cfg;

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

Signature · 1 line

2.20.1

Philippe Mathieu-Daudé

author

2019-05-24 06:42 UTC #23 lore raw

PATCH 12/20 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 | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

hw/i386/pc.c +8 ~~-10~~

index fe07baeb1d..6cfdb09f34 100644
@@ -928,14 +928,12 @@ static void pc_build_smbios(PCMachineState *pcms)
     }
 }
 
-static FWCfgState *x86_create_fw_cfg(PCMachineState *pcms,
+static FWCfgState *x86_create_fw_cfg(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);

@@ -953,7 +951,7 @@ static FWCfgState *x86_create_fw_cfg(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);

@@ -969,20 +967,19 @@ static FWCfgState *x86_create_fw_cfg(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;

@@ -1763,7 +1760,8 @@ void pc_memory_init(PCMachineState *pcms,
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = x86_create_fw_cfg(pcms, pcms->boot_cpus, pcms->apic_id_limit);
+    fw_cfg = x86_create_fw_cfg(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-05-24 06:44 UTC #24 lore raw

PATCH 13/20 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 6cfdb09f34..e8813ef2b6 100644
@@ -928,7 +928,7 @@ static void pc_build_smbios(PCMachineState *pcms)
     }
 }
 
-static FWCfgState *x86_create_fw_cfg(PCMachineState *pcms, const CPUArchIdList *cpus,
+static FWCfgState *x86_create_fw_cfg(MachineState *ms, const CPUArchIdList *cpus,
                                      uint16_t boot_cpus, uint16_t apic_id_limit)
 {
     FWCfgState *fw_cfg;

@@ -1664,6 +1664,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 +

@@ -1760,7 +1761,7 @@ void pc_memory_init(PCMachineState *pcms,
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = x86_create_fw_cfg(pcms, mc->possible_cpu_arch_ids(machine),
+    fw_cfg = x86_create_fw_cfg(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

Philippe Mathieu-Daudé

author

2019-05-24 06:47 UTC #25 lore raw

PATCH 14/20 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 e8813ef2b6..b029aee6c7 100644
@@ -886,7 +886,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;

@@ -900,7 +900,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);
     }

@@ -921,9 +921,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);
     }
 }

@@ -1587,7 +1587,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-05-24 06:49 UTC #26 lore raw

PATCH 15/20 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 b029aee6c7..a79b344eb5 100644
@@ -886,13 +886,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 */

@@ -1587,7 +1586,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-05-24 06:52 UTC #27 lore raw

PATCH 16/20 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 a79b344eb5..2804c4dc1b 100644
@@ -886,7 +886,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;

@@ -1586,7 +1586,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-05-24 06:54 UTC #28 lore raw

PATCH 17/20 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 2804c4dc1b..0db0ba3893 100644
@@ -1515,7 +1515,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);

@@ -1541,7 +1542,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)

@@ -1587,7 +1588,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-05-24 06:55 UTC #29 lore raw

PATCH 18/20 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 0db0ba3893..0ff2cea1ee 100644
@@ -1515,10 +1515,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;

@@ -1588,7 +1587,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-05-24 06:56 UTC #30 lore raw

PATCH 19/20 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 0ff2cea1ee..4d333aba82 100644
@@ -1515,8 +1515,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;

@@ -1587,7 +1587,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-05-24 06:43 UTC #31 lore raw

PATCH 20/20 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 re-use these functions.

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

---
 hw/i386/fw_cfg.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/i386/fw_cfg.h |   6 +++
 hw/i386/pc.c     | 128 +-------------------------------------------
 3 files changed, 142 insertions(+), 127 deletions(-)

hw/i386/fw_cfg.c +135 -0

index 380a819230..bdd744c040 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,131 @@ const char *fw_cfg_arch_key_name(uint16_t key)
     }
     return NULL;
 }
+
+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));
+}
+
+FWCfgState *x86_create_fw_cfg(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;
+}

hw/i386/fw_cfg.h +6 -0

index 17a4bc32f2..14a9223374 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,9 @@
 #define FW_CFG_E820_TABLE       (FW_CFG_ARCH_LOCAL + 3)
 #define FW_CFG_HPET             (FW_CFG_ARCH_LOCAL + 4)
 
+FWCfgState *x86_create_fw_cfg(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 ~~-127~~

146 lines not on this page · raw message

Signature · 1 line

2.20.1

Michael S. Tsirkin

2019-05-29 03:22 UTC #32 lore raw

In reply to Philippe Mathieu-Daudé

Quoted 10 lines · Philippe Mathieu-Daudé

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 [*] 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.

Quoted 3 lines

Regards,
Phil.

So I take issue with some of the renames. When I see fw_cfg_XXXX I expect the definition be in fw_cfg.c not in pc.c

Please use prefixes that match the file name -
avoids namespace conflicts and makes it easy to
find files.

This is not to say that the current system is perfect:
we have bochs_bios_init which isn't in bochs.c, should be pc_bochs_bios_init or whatever.

Thanks!

Quoted 41 lines

[*] https://github.com/intel/nemu/commit/3cb92d080835ac8d47c8b713156338afa33cff5c
Philippe Mathieu-Daudé (20):
hw/i386/pc: Use unsigned type to index arrays hw/i386/pc: Use size_t type to hold/return a size of array hw/i386/pc: Let e820_add_entry() return a ssize_t type hw/i386/pc: Add the E820Type enum type
hw/i386/pc: Add documentation to the e820_*() functions 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() more generic as
    x86_create_fw_cfg()
  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 |  62 +++++++++++
 hw/i386/e820_memory_layout.h |  76 +++++++++++++
 hw/i386/fw_cfg.c             | 135 +++++++++++++++++++++++
 hw/i386/fw_cfg.h             |   6 ++
 hw/i386/pc.c                 | 201 ++---------------------------------
 include/hw/i386/pc.h         |  11 --
 target/i386/kvm.c            |   1 +
 8 files changed, 289 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