[Qemu-devel] [PATCH 00/20] hw/i386/pc: Do not restrict the fw_cfg functions to the PC machine

[Qemu-devel] [PATCH 00/20] hw/i386/pc: Do not restrict the fw_cfg functions to the PC machine

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

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

-- 
2.20.1

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

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc.c         | 5 +++--
 include/hw/i386/pc.h | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2632b73f80..fc38b59d2d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -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);
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 43df7230a2..ad3a75d8fa 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -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

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

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年5月24日周五 下午2:40写道：

> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>

Reviewed-by: Li Qiang <liq3ea@gmail.com>


> ---
>  hw/i386/pc.c         | 5 +++--
>  include/hw/i386/pc.h | 2 +-
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 2632b73f80..fc38b59d2d 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -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);
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 43df7230a2..ad3a75d8fa 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -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
>
>
>

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

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc.c         | 4 ++--
 include/hw/i386/pc.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index fc38b59d2d..df8600ac24 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -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;
 }
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index ad3a75d8fa..28b19173b0 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -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

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

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年5月24日周五 下午2:37写道：

> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>


Reviewed-by: Li Qiang <liq3ea@gmail.com>


> ---
>  hw/i386/pc.c         | 4 ++--
>  include/hw/i386/pc.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index fc38b59d2d..df8600ac24 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -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;
>  }
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index ad3a75d8fa..28b19173b0 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -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
>
>
>

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

[Philippe Mathieu-Daudé]

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

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc.c         | 2 +-
 include/hw/i386/pc.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index df8600ac24..1245028dd6 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -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;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 28b19173b0..2bc48c03c6 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -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

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

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于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é <philmd@redhat.com>
>

Reviewed-by: Li Qiang <liq3ea@gmail.com>



> ---
>  hw/i386/pc.c         | 2 +-
>  include/hw/i386/pc.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index df8600ac24..1245028dd6 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -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;
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 28b19173b0..2bc48c03c6 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -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
>
>
>

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

[Philippe Mathieu-Daudé]

This ensure we won't use an incorrect value.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 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;
     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

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

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年5月24日周五 下午2:45写道：

> This ensure we won't use an incorrect value.
>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  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;
>

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



>      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
>
>
>

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

[Philippe Mathieu-Daudé]

Hi Li,

On 5/24/19 2:33 PM, Li Qiang wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com <mailto:philmd@redhat.com>> 于
> 2019年5月24日周五 下午2:45写道：
> 
>     This ensure we won't use an incorrect value.
> 
>     Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
>     ---
>      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;
> 
> 
> 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.

>          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
> 
> 

[Qemu-devel] [PATCH 05/20] hw/i386/pc: Add documentation to the e820_*() functions

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/i386/pc.h | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 10e77a40ce..95bf3278f2 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -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;
-- 
2.20.1

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

[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é <philmd@redhat.com>
---
 hw/i386/pc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index ac8343c728..2e195049a5 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -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

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

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于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é <philmd@redhat.com>
>


Reviewed-by: Li Qiang <liq3ea@gmail.com>



> ---
>  hw/i386/pc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index ac8343c728..2e195049a5 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -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
>
>
>

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

[Philippe Mathieu-Daudé]

Suggested-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 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

diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
index 5d9c9efd5f..d3374e0831 100644
--- a/hw/i386/Makefile.objs
+++ b/hw/i386/Makefile.objs
@@ -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
diff --git a/hw/i386/e820_memory_layout.c b/hw/i386/e820_memory_layout.c
new file mode 100644
index 0000000000..b9be08536c
--- /dev/null
+++ b/hw/i386/e820_memory_layout.c
@@ -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)) {
+        *address = le64_to_cpu(e820_table[idx].address);
+        *length = le64_to_cpu(e820_table[idx].length);
+        return true;
+    }
+    return false;
+}
diff --git a/hw/i386/e820_memory_layout.h b/hw/i386/e820_memory_layout.h
new file mode 100644
index 0000000000..64e88e4772
--- /dev/null
+++ b/hw/i386/e820_memory_layout.h
@@ -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.
+ */
+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.
+ */
+bool e820_get_entry(unsigned int index, E820Type type,
+                    uint64_t *address, uint64_t *length);
+
+#endif
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2e195049a5..fc22779ac1 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -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;
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 95bf3278f2..0f1bf667ae 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -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.
- */
-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.
- */
-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;
 
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 3b29ce5c0d..dbf890005e 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -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

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

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年5月24日周五 下午2:41写道：

> Suggested-by: Samuel Ortiz <sameo@linux.intel.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>


Reviewed-by: Li Qiang <liq3ea@gmail.com>


> ---
>  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
>
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index 5d9c9efd5f..d3374e0831 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -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
> diff --git a/hw/i386/e820_memory_layout.c b/hw/i386/e820_memory_layout.c
> new file mode 100644
> index 0000000000..b9be08536c
> --- /dev/null
> +++ b/hw/i386/e820_memory_layout.c
> @@ -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))
> {
> +        *address = le64_to_cpu(e820_table[idx].address);
> +        *length = le64_to_cpu(e820_table[idx].length);
> +        return true;
> +    }
> +    return false;
> +}
> diff --git a/hw/i386/e820_memory_layout.h b/hw/i386/e820_memory_layout.h
> new file mode 100644
> index 0000000000..64e88e4772
> --- /dev/null
> +++ b/hw/i386/e820_memory_layout.h
> @@ -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.
> + */
> +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.
> + */
> +bool e820_get_entry(unsigned int index, E820Type type,
> +                    uint64_t *address, uint64_t *length);
> +
> +#endif
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 2e195049a5..fc22779ac1 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -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;
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 95bf3278f2..0f1bf667ae 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -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.
> - */
> -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.
> - */
> -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;
>
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 3b29ce5c0d..dbf890005e 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -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
>
>

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

[Philippe Mathieu-Daudé]

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

Suggested-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index fc22779ac1..a3936bb29d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -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);
 
-- 
2.20.1

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

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于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 <sameo@linux.intel.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>

Reviewed-by: Li Qiang <liq3ea@gmail.com>


> ---
>  hw/i386/pc.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index fc22779ac1..a3936bb29d 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -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);
>
> --
> 2.20.1
>
>
>

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

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

Suggested-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 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 a3936bb29d..264074489b 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -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);
 
-- 
2.20.1

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

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于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 <sameo@linux.intel.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>

Reviewed-by: Li Qiang <liq3ea@gmail.com>



> ---
>  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 a3936bb29d..264074489b 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -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);
>
> --
> 2.20.1
>
>
>

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

[Philippe Mathieu-Daudé]

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

Suggested-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 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)
 {
     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

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

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于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 <sameo@linux.intel.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  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)
>


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




>  {
>      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
>
>
>

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

[Philippe Mathieu-Daudé]

On 5/24/19 5:30 PM, Li Qiang wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com <mailto:philmd@redhat.com>> 于
> 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 <sameo@linux.intel.com
>     <mailto:sameo@linux.intel.com>>
>     Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
>     ---
>      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)
> 
> 
> 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 :)

> 
>      {
>          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
> 
> 

[Qemu-devel] [PATCH 11/20] hw/i386/pc: Pass the apic_id_limit value by argument

[Philippe Mathieu-Daudé]

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

Suggested-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 01894b9875..fe07baeb1d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -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);
 
-- 
2.20.1

[Qemu-devel] [PATCH 12/20] hw/i386/pc: Pass the CPUArchIdList array by argument

[Philippe Mathieu-Daudé]

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

Suggested-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index fe07baeb1d..6cfdb09f34 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -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);
 
-- 
2.20.1

[Qemu-devel] [PATCH 13/20] hw/i386/pc: Let fw_cfg_init() use the generic MachineState

[Philippe Mathieu-Daudé]

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

Suggested-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 6cfdb09f34..e8813ef2b6 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, 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);
-- 
2.20.1

[Qemu-devel] [PATCH 14/20] hw/i386/pc: Let pc_build_smbios() take a FWCfgState argument

[Philippe Mathieu-Daudé]

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

Suggested-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e8813ef2b6..b029aee6c7 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -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);
-- 
2.20.1

[Qemu-devel] [PATCH 15/20] hw/i386/pc: Let pc_build_smbios() take a generic MachineState argument

[Philippe Mathieu-Daudé]

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

Suggested-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index b029aee6c7..a79b344eb5 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -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);
-- 
2.20.1

[Qemu-devel] [PATCH 16/20] hw/i386/pc: Rename pc_build_smbios() as generic fw_cfg_build_smbios()

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

Suggested-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a79b344eb5..2804c4dc1b 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -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);
-- 
2.20.1

[Qemu-devel] [PATCH 17/20] hw/i386/pc: Let pc_build_feature_control() take a FWCfgState argument

[Philippe Mathieu-Daudé]

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

Suggested-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2804c4dc1b..0db0ba3893 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -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);
     }
-- 
2.20.1

[Qemu-devel] [PATCH 18/20] hw/i386/pc: Let pc_build_feature_control() take a MachineState argument

[Philippe Mathieu-Daudé]

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

Suggested-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0db0ba3893..0ff2cea1ee 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -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);
     }
-- 
2.20.1

[Qemu-devel] [PATCH 19/20] hw/i386/pc: Rename pc_build_feature_control() as generic fw_cfg_build_*

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

Suggested-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 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 0ff2cea1ee..4d333aba82 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -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);
     }
-- 
2.20.1

[Qemu-devel] [PATCH 20/20] hw/i386/pc: Extract the x86 generic fw_cfg code

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

Suggested-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/fw_cfg.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/i386/fw_cfg.h |   6 +++
 hw/i386/pc.c     | 128 +-------------------------------------------
 3 files changed, 142 insertions(+), 127 deletions(-)

diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
index 380a819230..bdd744c040 100644
--- a/hw/i386/fw_cfg.c
+++ b/hw/i386/fw_cfg.c
@@ -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;
+}
diff --git a/hw/i386/fw_cfg.h b/hw/i386/fw_cfg.h
index 17a4bc32f2..14a9223374 100644
--- a/hw/i386/fw_cfg.h
+++ b/hw/i386/fw_cfg.h
@@ -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
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 4d333aba82..0e59756ed5 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -79,6 +79,7 @@
 #include "hw/net/ne2000-isa.h"
 #include "standard-headers/asm-x86/bootparam.h"
 #include "e820_memory_layout.h"
+#include "fw_cfg.h"
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ
@@ -886,104 +887,6 @@ static uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
     }
 }
 
-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;
-    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);
-    }
-}
-
-static 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;
-}
-
 static long get_file_size(FILE *f)
 {
     long where, size;
@@ -1515,35 +1418,6 @@ void pc_cpus_init(PCMachineState *pcms)
     }
 }
 
-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;
-    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));
-}
-
 static void rtc_set_cpus_count(ISADevice *rtc, uint16_t cpus_count)
 {
     if (cpus_count > 0xff) {
-- 
2.20.1

Re: [Qemu-devel] [PATCH 00/20] hw/i386/pc: Do not restrict the fw_cfg functions to the PC machine

[Michael S. Tsirkin]

On Fri, May 24, 2019 at 08:35:33AM +0200, Philippe Mathieu-Daudé wrote:
> 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.

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!


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