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

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 10 Mar 2022 11:51:37 +0530
Ani Sinha <ani@anisinha.ca> wrote:

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

see commit 7d67110f2d
until we have older machine types it must stay or have a compat knob,
once they are gone we can remove it as 1.7 and older machine types
are supposed to have newer SeaBIOS version that doesn't utilize
it anymore.

so add a compat knob (not sure if it's worth the trouble) or
wait till pre 1.7 machines are gone and then rebase/repost.

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