diff mbox series

[04/20] hw/i386/pc: Add the E820Type enum type

Message ID 20190524063553.5339-5-philmd@redhat.com
State New
Headers show
Series hw/i386/pc: Do not restrict the fw_cfg functions to the PC machine | expand

Commit Message

Philippe Mathieu-Daudé May 24, 2019, 6:35 a.m. UTC 
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(-)

Comments

Li Qiang May 24, 2019, 12:33 p.m. UTC | #1 
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
>
>
>
Philippe Mathieu-Daudé May 30, 2019, 4:34 p.m. UTC | #2 
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
> 
>
diff mbox series

Patch

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