[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH] hw/i386/e820: remove legacy reserved entries for e820
From: |
Igor Mammedov |
Subject: |
Re: [RFC PATCH] hw/i386/e820: remove legacy reserved entries for e820 |
Date: |
Thu, 10 Mar 2022 17:42:27 +0100 |
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
> >
>