Re: [RFC PATCH] hw/i386/e820: remove legacy reserved entries for e820

[Ani Sinha]

On Mon, Feb 28, 2022 at 8:56 PM Ani Sinha <ani@anisinha.ca> wrote:
>
> e820 reserved entries were used before the dynamic entries with fw config 
> files
> were intoduced into qemu with the following change:
> 7d67110f2d9a6("pc: add etc/e820 fw_cfg file")
>
> Identical support was introduced into seabios as well with the following 
> commit:
> ce39bd4031820 ("Add support for etc/e820 fw_cfg file")
>
> Both the above commits are now quite old. Seabios uses fw config files and
> dynamic e820 entries by default and only falls back to using reserved entries
> when it has to work with old qemu (versions earlier than 1.7). Please see
> functions qemu_cfg_e820() and qemu_early_e820(). It is safe to remove legacy
> FW_CFG_E820_TABLE and associated code. It would be incredibly rare to run the
> latest qemu version with a very old version of seabios that did not support
> fw config files for e820.
>
> As far as I could see, edk2/ovfm never supported reserved entries and uses fw
> config files from the beginning. So there should be no incompatibilities with
> ovfm as well.

Igor, Gerd, as I had replied in the other thread, I am not sure if we
need the compatibility dance in order to do this. I think we can't
carry this legacy stuff on forever.
Please advice.

>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> ---
>  hw/i386/e820_memory_layout.c | 20 +-------------------
>  hw/i386/e820_memory_layout.h |  8 --------
>  hw/i386/fw_cfg.c             |  3 ---
>  hw/i386/fw_cfg.h             |  1 -
>  hw/i386/microvm.c            |  2 --
>  5 files changed, 1 insertion(+), 33 deletions(-)
>
> diff --git a/hw/i386/e820_memory_layout.c b/hw/i386/e820_memory_layout.c
> index bcf9eaf837..06970ac44a 100644
> --- a/hw/i386/e820_memory_layout.c
> +++ b/hw/i386/e820_memory_layout.c
> @@ -11,29 +11,11 @@
>  #include "e820_memory_layout.h"
>
>  static size_t e820_entries;
> -struct e820_table e820_reserve;
>  struct e820_entry *e820_table;
>
>  int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
>  {
> -    int index = le32_to_cpu(e820_reserve.count);
> -    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(type);
> -
> -        e820_reserve.count = cpu_to_le32(index);
> -    }
> -
> -    /* new "etc/e820" file -- include ram too */
> +    /* new "etc/e820" file -- include ram and reserved entries */
>      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);
> diff --git a/hw/i386/e820_memory_layout.h b/hw/i386/e820_memory_layout.h
> index 2a0ceb8b9c..daf41cc4b4 100644
> --- a/hw/i386/e820_memory_layout.h
> +++ b/hw/i386/e820_memory_layout.h
> @@ -16,20 +16,12 @@
>  #define E820_NVS        4
>  #define E820_UNUSABLE   5
>
> -#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;
>
>  int e820_add_entry(uint64_t address, uint64_t length, uint32_t type);
> diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
> index a283785a8d..72a42f3c66 100644
> --- a/hw/i386/fw_cfg.c
> +++ b/hw/i386/fw_cfg.c
> @@ -36,7 +36,6 @@ const char *fw_cfg_arch_key_name(uint16_t key)
>          {FW_CFG_ACPI_TABLES, "acpi_tables"},
>          {FW_CFG_SMBIOS_ENTRIES, "smbios_entries"},
>          {FW_CFG_IRQ0_OVERRIDE, "irq0_override"},
> -        {FW_CFG_E820_TABLE, "e820_table"},
>          {FW_CFG_HPET, "hpet"},
>      };
>
> @@ -127,8 +126,6 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms,
>  #endif
>      fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, 1);
>
> -    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());
>
> diff --git a/hw/i386/fw_cfg.h b/hw/i386/fw_cfg.h
> index 275f15c1c5..86ca7c1c0c 100644
> --- a/hw/i386/fw_cfg.h
> +++ b/hw/i386/fw_cfg.h
> @@ -17,7 +17,6 @@
>  #define FW_CFG_ACPI_TABLES      (FW_CFG_ARCH_LOCAL + 0)
>  #define FW_CFG_SMBIOS_ENTRIES   (FW_CFG_ARCH_LOCAL + 1)
>  #define FW_CFG_IRQ0_OVERRIDE    (FW_CFG_ARCH_LOCAL + 2)
> -#define FW_CFG_E820_TABLE       (FW_CFG_ARCH_LOCAL + 3)
>  #define FW_CFG_HPET             (FW_CFG_ARCH_LOCAL + 4)
>
>  FWCfgState *fw_cfg_arch_create(MachineState *ms,
> diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
> index 4b3b1dd262..f2101e7293 100644
> --- a/hw/i386/microvm.c
> +++ b/hw/i386/microvm.c
> @@ -324,8 +324,6 @@ static void microvm_memory_init(MicrovmMachineState *mms)
>      fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, machine->smp.max_cpus);
>      fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)machine->ram_size);
>      fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, 1);
> -    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());
>
> --
> 2.25.1
>