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

Ani Sinha posted 1 patch 2 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220420043904.1225153-1-ani@anisinha.ca
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, Sergio Lopez <slp@redhat.com>
There is a newer version of this series
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(-)
[PATCH] hw/i386/e820: remove legacy reserved entries for e820
Posted by Ani Sinha 2 years ago
e820 reserved entries were used before the dynamic entries with fw config files
were intoduced. Please see 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. QEMU machines 1.7 and newer no longer
use the reserved entries. 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 now as QEMU 7.0 has deprecated i440fx
machines 1.7 and older. 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.

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
Re: [PATCH] hw/i386/e820: remove legacy reserved entries for e820
Posted by Gerd Hoffmann 2 years ago
On Wed, Apr 20, 2022 at 10:09:04AM +0530, Ani Sinha wrote:
> e820 reserved entries were used before the dynamic entries with fw config files
> were intoduced. Please see 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. QEMU machines 1.7 and newer no longer
> use the reserved entries. Seabios uses fw config files and

We still support machine types 1.6 & older (although they are
deprecated already), so I think this has to wait until the 1.7 + older
machine types are actually dropped.

Otherwise:
Acked-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd
Re: [PATCH] hw/i386/e820: remove legacy reserved entries for e820
Posted by Ani Sinha 2 years ago
On Wed, Apr 20, 2022 at 1:57 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Wed, Apr 20, 2022 at 10:09:04AM +0530, Ani Sinha wrote:
> > e820 reserved entries were used before the dynamic entries with fw config files
> > were intoduced. Please see 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. QEMU machines 1.7 and newer no longer
> > use the reserved entries. Seabios uses fw config files and
>
> We still support machine types 1.6 & older (although they are
> deprecated already), so I think this has to wait until the 1.7 + older
> machine types are actually dropped.

How much time does it need to pass before something is dropped after
depreciation?

>
> Otherwise:
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
>
> take care,
>   Gerd
>
Re: [PATCH] hw/i386/e820: remove legacy reserved entries for e820
Posted by Gerd Hoffmann 2 years ago
On Wed, Apr 20, 2022 at 02:25:54PM +0530, Ani Sinha wrote:
> On Wed, Apr 20, 2022 at 1:57 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > On Wed, Apr 20, 2022 at 10:09:04AM +0530, Ani Sinha wrote:
> > > e820 reserved entries were used before the dynamic entries with fw config files
> > > were intoduced. Please see 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. QEMU machines 1.7 and newer no longer
> > > use the reserved entries. Seabios uses fw config files and
> >
> > We still support machine types 1.6 & older (although they are
> > deprecated already), so I think this has to wait until the 1.7 + older
> > machine types are actually dropped.
> 
> How much time does it need to pass before something is dropped after
> depreciation?

Two releases.  Deprecated in January: f59fb1889f48 ("hw/i386/pc_piix:
Mark the machine types from version 1.4 to 1.7 as deprecated"), so 7.0
and 7.1, in the 7.2 devel cycle they can be removed.  Plus a bunch of
followup cleanups like this patch.  I think we can also drop the 128k
seabios images then.

take care,
  Gerd