[Qemu-devel] [PULL for-1.7 0/2] pc: e820 fw_cfg fixup.

[Qemu-devel] [PULL for-1.7 0/2] pc: e820 fw_cfg fixup.

[Gerd Hoffmann]

  Hi,

After some discussion with Andrea Arcangeli the initial
approach of extending the FW_CFG_E820_TABLE was scratched.
Unfortunaly that patch wasn't formally NACK'ed and became
commit 0624c7f916b4d97f17726d9b295d6a6b0dc5076d, so we
need to fixup the mess now, before 1.7.

The new approach is to leave FW_CFG_E820_TABLE alone and
add a new fw_cfg file instead, to avoid any compatibility
issues for sure (patch thread here:
http://comments.gmane.org/gmane.comp.emulators.qemu/238860).

This pull is the new-approach patch series rebased to latest
master (i.e. solve the conflict with
0624c7f916b4d97f17726d9b295d6a6b0dc5076d).

I suggest to pull this to fix things up.  The alternative is
to revert 0624c7f916b4d97f17726d9b295d6a6b0dc5076d and delay
the new interface to the 1.8 devel cycle.

cheers,
  Gerd


The following changes since commit a126050a103c924b03388a9a64ce9af8c96b0969:

  Merge remote-tracking branch 'kwolf/tags/for-anthony' into staging (2013-10-31 17:02:26 +0100)

are available in the git repository at:


  git://git.kraxel.org/qemu e820.1

for you to fetch changes up to 7db16f2480db5e246d34d0c453cff4f58549df0e:

  pc: register e820 entries for ram (2013-11-04 12:31:33 +0100)

----------------------------------------------------------------
Gerd Hoffmann (2):
      pc: add etc/e820 fw_cfg file
      pc: register e820 entries for ram

 hw/i386/pc.c | 47 +++++++++++++++++++++++++++++------------------
 1 file changed, 29 insertions(+), 18 deletions(-)

[Qemu-devel] [PATCH 1/2] pc: add etc/e820 fw_cfg file

[Gerd Hoffmann]

Unlike the existing FW_CFG_E820_TABLE entry which carries reservations
only the new etc/e820 file also has entries for RAM.

Format is simliar to the FW_CFG_E820_TABLE, it is a simple list of
e820_entry structs.  Unlike FW_CFG_E820_TABLE it has no count though
as the number of entries can be figured from the file size.

Cc: Andrea Arcangeli <aarcange@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/i386/pc.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index dee409d..a653ae4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -90,7 +90,9 @@ struct e820_table {
     struct e820_entry entry[E820_NR_ENTRIES];
 } QEMU_PACKED __attribute((__aligned__(4)));
 
-static struct e820_table e820_table;
+static struct e820_table e820_reserve;
+static struct e820_entry *e820_table;
+static unsigned e820_entries;
 struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
 
 void gsi_handler(void *opaque, int n, int level)
@@ -577,19 +579,32 @@ static void handle_a20_line_change(void *opaque, int irq, int level)
 
 int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
 {
-    int index = le32_to_cpu(e820_table.count);
+    int index = le32_to_cpu(e820_reserve.count);
     struct e820_entry *entry;
 
-    if (index >= E820_NR_ENTRIES)
-        return -EBUSY;
-    entry = &e820_table.entry[index++];
+    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);
+    }
 
-    entry->address = cpu_to_le64(address);
-    entry->length = cpu_to_le64(length);
-    entry->type = cpu_to_le32(type);
+    /* new "etc/e820" file -- include ram too */
+    e820_table = g_realloc(e820_table,
+                           sizeof(struct e820_entry) * (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_entries++;
 
-    e820_table.count = cpu_to_le32(index);
-    return index;
+    return e820_entries;
 }
 
 /* Calculates the limit to CPU APIC ID values
@@ -640,7 +655,9 @@ static FWCfgState *bochs_bios_init(void)
         fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
                          smbios_table, smbios_len);
     fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE,
-                     &e820_table, sizeof(e820_table));
+                     &e820_reserve, sizeof(e820_reserve));
+    fw_cfg_add_file(fw_cfg, "etc/e820", e820_table,
+                    sizeof(struct e820_entry) * e820_entries);
 
     fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_cfg, sizeof(hpet_cfg));
     /* allocate memory for the NUMA channel: one (64bit) word for the number
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] pc: register e820 entries for ram

[Gerd Hoffmann]

So RAM shows up in the new etc/e820 fw_cfg file.

Cc: Andrea Arcangeli <aarcange@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/i386/pc.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a653ae4..12c436e 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1174,13 +1174,7 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
     memory_region_init_alias(ram_below_4g, NULL, "ram-below-4g", ram,
                              0, below_4g_mem_size);
     memory_region_add_subregion(system_memory, 0, ram_below_4g);
-    if (0) {
-        /*
-         * Ideally we should do that too, but that would ruin the e820
-         * reservations added by seabios before initializing fw_cfg.
-         */
-        e820_add_entry(0, below_4g_mem_size, E820_RAM);
-    }
+    e820_add_entry(0, below_4g_mem_size, E820_RAM);
     if (above_4g_mem_size > 0) {
         ram_above_4g = g_malloc(sizeof(*ram_above_4g));
         memory_region_init_alias(ram_above_4g, NULL, "ram-above-4g", ram,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] pc: add etc/e820 fw_cfg file

[Andrea Arcangeli]

Hi,

On Mon, Nov 04, 2013 at 01:17:10PM +0100, Gerd Hoffmann wrote:
> Unlike the existing FW_CFG_E820_TABLE entry which carries reservations
> only the new etc/e820 file also has entries for RAM.

Acked, it looks the best the way to go if the objective is to keep
backwards compatibility with older seabios protocol.

I have to trust you on why we need to stay backwards compatible at all
times and why we can't commit an updated bios.bin before a new seabios
stable release happened. Otherwise we could have just fixed
FW_CFG_E820_TABLE breaking backwards compatibility without introducing
a partially overlapping fw_cfg.

About the file, I wonder what happens if too many people starts to use
files and we'll run out of FW_CFG_FILE_SLOTS at runtime (assert(index
< FW_CFG_FILE_SLOTS);). To me seeing the list of all fw_cfg IDs in a
header file to define all possible paravirt APIs seabios need to know
about, looked not so bad, but then grepping for fw_cfg_add_file is
equivalent. The main issue is that if we use files from now, it would
be nicer not to limit those to FW_CFG_FILE_SLOTS but to allocate them
a bit more dynamically without asserts but this is offtopic (I just
happened to read how files are created to review this patch).

Probably one patch could be added to
s/FW_CFG_E820_TABLE/FW_CFG_E820_TABLE_OLD/, otherwise if somebody read
fw_cfg.h, it won't be apparent that the grepping shouldn't stop there
to reach the real e820 map.

Thanks!
Andrea

Re: [Qemu-devel] [PATCH 1/2] pc: add etc/e820 fw_cfg file

[Gerd Hoffmann]

On Di, 2013-11-05 at 18:48 +0100, Andrea Arcangeli wrote:
> Hi,
> 
> On Mon, Nov 04, 2013 at 01:17:10PM +0100, Gerd Hoffmann wrote:
> > Unlike the existing FW_CFG_E820_TABLE entry which carries reservations
> > only the new etc/e820 file also has entries for RAM.
> 
> Acked, it looks the best the way to go if the objective is to keep
> backwards compatibility with older seabios protocol.
> 
> I have to trust you on why we need to stay backwards compatible at all
> times and why we can't commit an updated bios.bin before a new seabios
> stable release happened.

It's seabios policy, for good reasons.  Basically because it is a PITA
in certain cases if there are strict version requirements.  Also note
that seabios isn't the only possible firmware for qemu.

> About the file, I wonder what happens if too many people starts to use
> files and we'll run out of FW_CFG_FILE_SLOTS at runtime (assert(index
> < FW_CFG_FILE_SLOTS);).

Probably we should simply raise the number of slots.  We don't have
allocated any FW_CFG slots behind the file slot range yet, so there is
space.  Also there is a count in the file list struct and firmware uses
that to figure how many files are there and dynamically allocate memory
for the list, so growing the list shouldn't cause problems on the
firmware side too.

> Probably one patch could be added to
> s/FW_CFG_E820_TABLE/FW_CFG_E820_TABLE_OLD/, otherwise if somebody read
> fw_cfg.h, it won't be apparent that the grepping shouldn't stop there
> to reach the real e820 map.

Yep, either that or a comment.  Also there are more obsolete fw_cfg
entries I think.  Isn't the num_cpus entry superseded by numa info?

cheers,
  Gerd

[Qemu-devel] [PATCH 1/2] pc: add etc/e820 fw_cfg file

[Gerd Hoffmann]

Unlike the existing FW_CFG_E820_TABLE entry which carries reservations
only the new etc/e820 file also has entries for RAM.

Format is simliar to the FW_CFG_E820_TABLE, it is a simple list of
e820_entry structs.  Unlike FW_CFG_E820_TABLE it has no count though
as the number of entries can be figured from the file size.

Cc: Andrea Arcangeli <aarcange@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/i386/pc.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0c313fe..0500ab6 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -89,7 +89,9 @@ struct e820_table {
     struct e820_entry entry[E820_NR_ENTRIES];
 } QEMU_PACKED __attribute((__aligned__(4)));
 
-static struct e820_table e820_table;
+static struct e820_table e820_reserve;
+static struct e820_entry *e820_table;
+static unsigned e820_entries;
 struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
 
 void gsi_handler(void *opaque, int n, int level)
@@ -576,19 +578,32 @@ static void handle_a20_line_change(void *opaque, int irq, int level)
 
 int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
 {
-    int index = le32_to_cpu(e820_table.count);
+    int index = le32_to_cpu(e820_reserve.count);
     struct e820_entry *entry;
 
-    if (index >= E820_NR_ENTRIES)
-        return -EBUSY;
-    entry = &e820_table.entry[index++];
+    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);
+    }
 
-    entry->address = cpu_to_le64(address);
-    entry->length = cpu_to_le64(length);
-    entry->type = cpu_to_le32(type);
+    /* new "etc/e820" file -- include ram too */
+    e820_table = g_realloc(e820_table,
+                           sizeof(struct e820_entry) * (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_entries++;
 
-    e820_table.count = cpu_to_le32(index);
-    return index;
+    return e820_entries;
 }
 
 /* Calculates the limit to CPU APIC ID values
@@ -639,7 +654,9 @@ static FWCfgState *bochs_bios_init(void)
         fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
                          smbios_table, smbios_len);
     fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE,
-                     &e820_table, sizeof(e820_table));
+                     &e820_reserve, sizeof(e820_reserve));
+    fw_cfg_add_file(fw_cfg, "etc/e820", e820_table,
+                    sizeof(struct e820_entry) * e820_entries);
 
     fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_cfg, sizeof(hpet_cfg));
     /* allocate memory for the NUMA channel: one (64bit) word for the number
-- 
1.8.3.1