[PATCH] Seabios - read e820 reserve from qemu_cfg

[PATCH] Seabios - read e820 reserve from qemu_cfg

[Jes Sorensen]

[-- Attachment #1: Type: text/plain, Size: 547 bytes --]

Hi,

Right now KVM/QEMU relies on hard coded values in Seabios for the
reserved area for the TSS pages and the EPT page.

I'd like to suggest we change this to pass the value from QEMU via
qemu-cfg making it possible to move it around dynamically in the future.

Attached is a patch to Seabios for this, which defaults to the current
hard coded value if no value is provided by qemu-cfg. We can remove
the backwards compatibility later.

I'll post the QEMU patches for upstream QEMU and QEMU-KVM in a minute.

Comments most welcome!

Cheers,
Jes


[-- Attachment #2: 0005-kvm-e820-reserve.patch --]
[-- Type: text/plain, Size: 2953 bytes --]

Read location and size of KVM switch area from qemu-cfg

Read location of KVM's switch area (VMX TSS pages and EPT) from QEMU
via qemu-cfg instead of relying on hard coded values.

For now, fall back to the old hard coded values for compatibility
reasons. Compatibility code can be removed in the future.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

---
 src/paravirt.c |    9 +++++++++
 src/paravirt.h |    7 +++++++
 src/post.c     |   14 ++++++++++----
 3 files changed, 26 insertions(+), 4 deletions(-)

Index: seabios/src/paravirt.c
===================================================================
--- seabios.orig/src/paravirt.c
+++ seabios/src/paravirt.c
@@ -132,6 +132,15 @@ u16 qemu_cfg_smbios_entries(void)
     return cnt;
 }
 
+int qemu_cfg_read_e820_reserve(struct qemu_cfg_e820_reserve *reserve)
+{
+    if (!qemu_cfg_present)
+        return 0;
+
+    qemu_cfg_read((void *)reserve, sizeof(*reserve));
+    return reserve->length;
+}
+
 struct smbios_header {
     u16 length;
     u8 type;
Index: seabios/src/paravirt.h
===================================================================
--- seabios.orig/src/paravirt.h
+++ seabios/src/paravirt.h
@@ -36,6 +36,7 @@ static inline int kvm_para_available(voi
 #define QEMU_CFG_ACPI_TABLES		(QEMU_CFG_ARCH_LOCAL + 0)
 #define QEMU_CFG_SMBIOS_ENTRIES		(QEMU_CFG_ARCH_LOCAL + 1)
 #define QEMU_CFG_IRQ0_OVERRIDE		(QEMU_CFG_ARCH_LOCAL + 2)
+#define QEMU_CFG_E820_RESERVE		(QEMU_CFG_ARCH_LOCAL + 3)
 
 extern int qemu_cfg_present;
 
@@ -61,8 +62,14 @@ typedef struct QemuCfgFile {
     char name[56];
 } QemuCfgFile;
 
+struct qemu_cfg_e820_reserve {
+    u32 addr;
+    u32 length;
+};
+
 u16 qemu_cfg_first_file(QemuCfgFile *entry);
 u16 qemu_cfg_next_file(QemuCfgFile *entry);
 u32 qemu_cfg_read_file(QemuCfgFile *entry, void *dst, u32 maxlen);
+int qemu_cfg_read_e820_reserve(struct qemu_cfg_e820_reserve *reserve);
 
 #endif
Index: seabios/src/post.c
===================================================================
--- seabios.orig/src/post.c
+++ seabios/src/post.c
@@ -135,10 +135,16 @@ ram_probe(void)
              , E820_RESERVED);
     add_e820(BUILD_BIOS_ADDR, BUILD_BIOS_SIZE, E820_RESERVED);
 
-    if (kvm_para_available())
-        // 4 pages before the bios, 3 pages for vmx tss pages, the
-        // other page for EPT real mode pagetable
-        add_e820(0xfffbc000, 4*4096, E820_RESERVED);
+    if (kvm_para_available()) {
+        struct qemu_cfg_e820_reserve e820_reserve;
+        if (qemu_cfg_read_e820_reserve(&e820_reserve))
+            add_e820(e820_reserve.addr, e820_reserve.length, E820_RESERVED);
+        else {
+            // 4 pages before the bios, 3 pages for vmx tss pages, the
+            // other page for EPT real mode pagetable
+            add_e820(0xfffbc000, 4*4096, E820_RESERVED);
+        }
+    }
 
     dprintf(1, "Ram Size=0x%08x (0x%08x%08x high)\n"
             , RamSize, (u32)(RamSizeOver4G >> 32), (u32)RamSizeOver4G);

[Qemu-devel] [PATCH] Seabios - read e820 reserve from qemu_cfg

[Jes Sorensen]

[-- Attachment #1: Type: text/plain, Size: 547 bytes --]

Hi,

Right now KVM/QEMU relies on hard coded values in Seabios for the
reserved area for the TSS pages and the EPT page.

I'd like to suggest we change this to pass the value from QEMU via
qemu-cfg making it possible to move it around dynamically in the future.

Attached is a patch to Seabios for this, which defaults to the current
hard coded value if no value is provided by qemu-cfg. We can remove
the backwards compatibility later.

I'll post the QEMU patches for upstream QEMU and QEMU-KVM in a minute.

Comments most welcome!

Cheers,
Jes


[-- Attachment #2: 0005-kvm-e820-reserve.patch --]
[-- Type: text/plain, Size: 2953 bytes --]

Read location and size of KVM switch area from qemu-cfg

Read location of KVM's switch area (VMX TSS pages and EPT) from QEMU
via qemu-cfg instead of relying on hard coded values.

For now, fall back to the old hard coded values for compatibility
reasons. Compatibility code can be removed in the future.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

---
 src/paravirt.c |    9 +++++++++
 src/paravirt.h |    7 +++++++
 src/post.c     |   14 ++++++++++----
 3 files changed, 26 insertions(+), 4 deletions(-)

Index: seabios/src/paravirt.c
===================================================================
--- seabios.orig/src/paravirt.c
+++ seabios/src/paravirt.c
@@ -132,6 +132,15 @@ u16 qemu_cfg_smbios_entries(void)
     return cnt;
 }
 
+int qemu_cfg_read_e820_reserve(struct qemu_cfg_e820_reserve *reserve)
+{
+    if (!qemu_cfg_present)
+        return 0;
+
+    qemu_cfg_read((void *)reserve, sizeof(*reserve));
+    return reserve->length;
+}
+
 struct smbios_header {
     u16 length;
     u8 type;
Index: seabios/src/paravirt.h
===================================================================
--- seabios.orig/src/paravirt.h
+++ seabios/src/paravirt.h
@@ -36,6 +36,7 @@ static inline int kvm_para_available(voi
 #define QEMU_CFG_ACPI_TABLES		(QEMU_CFG_ARCH_LOCAL + 0)
 #define QEMU_CFG_SMBIOS_ENTRIES		(QEMU_CFG_ARCH_LOCAL + 1)
 #define QEMU_CFG_IRQ0_OVERRIDE		(QEMU_CFG_ARCH_LOCAL + 2)
+#define QEMU_CFG_E820_RESERVE		(QEMU_CFG_ARCH_LOCAL + 3)
 
 extern int qemu_cfg_present;
 
@@ -61,8 +62,14 @@ typedef struct QemuCfgFile {
     char name[56];
 } QemuCfgFile;
 
+struct qemu_cfg_e820_reserve {
+    u32 addr;
+    u32 length;
+};
+
 u16 qemu_cfg_first_file(QemuCfgFile *entry);
 u16 qemu_cfg_next_file(QemuCfgFile *entry);
 u32 qemu_cfg_read_file(QemuCfgFile *entry, void *dst, u32 maxlen);
+int qemu_cfg_read_e820_reserve(struct qemu_cfg_e820_reserve *reserve);
 
 #endif
Index: seabios/src/post.c
===================================================================
--- seabios.orig/src/post.c
+++ seabios/src/post.c
@@ -135,10 +135,16 @@ ram_probe(void)
              , E820_RESERVED);
     add_e820(BUILD_BIOS_ADDR, BUILD_BIOS_SIZE, E820_RESERVED);
 
-    if (kvm_para_available())
-        // 4 pages before the bios, 3 pages for vmx tss pages, the
-        // other page for EPT real mode pagetable
-        add_e820(0xfffbc000, 4*4096, E820_RESERVED);
+    if (kvm_para_available()) {
+        struct qemu_cfg_e820_reserve e820_reserve;
+        if (qemu_cfg_read_e820_reserve(&e820_reserve))
+            add_e820(e820_reserve.addr, e820_reserve.length, E820_RESERVED);
+        else {
+            // 4 pages before the bios, 3 pages for vmx tss pages, the
+            // other page for EPT real mode pagetable
+            add_e820(0xfffbc000, 4*4096, E820_RESERVED);
+        }
+    }
 
     dprintf(1, "Ram Size=0x%08x (0x%08x%08x high)\n"
             , RamSize, (u32)(RamSizeOver4G >> 32), (u32)RamSizeOver4G);

[PATCH] QEMU-KVM - provide e820 reserve through qemu_cfg

[Jes Sorensen]

[-- Attachment #1: Type: text/plain, Size: 100 bytes --]

Hi,

This is the QEMU-KVM bits for providing the e820-reserve space through
qemu-cfg.

Cheers,
Jes


[-- Attachment #2: 0011-qemu-kvm-e820-reserve.patch --]
[-- Type: text/plain, Size: 3549 bytes --]

Use qemu-cfg to notify the BIOS of the location of the TSS range to
reserve in the e820 table, to avoid relying on hard coded values.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

---
 hw/fw_cfg.h       |    5 +++++
 hw/pc.c           |    4 ++++
 kvm.h             |    2 ++
 qemu-kvm-x86.c    |    6 ++++++
 target-i386/kvm.c |    7 +++++++
 5 files changed, 24 insertions(+)

Index: qemu-kvm/hw/fw_cfg.h
===================================================================
--- qemu-kvm.orig/hw/fw_cfg.h
+++ qemu-kvm/hw/fw_cfg.h
@@ -67,4 +67,9 @@ FWCfgState *fw_cfg_init(uint32_t ctl_por
 
 #endif /* NO_QEMU_PROTOS */
 
+struct fw_cfg_e820_reserve {
+    uint32_t addr;
+    uint32_t length;
+};
+
 #endif
Index: qemu-kvm/hw/pc.c
===================================================================
--- qemu-kvm.orig/hw/pc.c
+++ qemu-kvm/hw/pc.c
@@ -66,6 +66,7 @@
 #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_RESERVE (FW_CFG_ARCH_LOCAL + 3)
 
 #define MAX_IDE_BUS 2
 
@@ -73,6 +74,7 @@ static fdctrl_t *floppy_controller;
 static RTCState *rtc_state;
 static PITState *pit;
 static PCII440FXState *i440fx_state;
+struct fw_cfg_e820_reserve e820_reserve;
 
 qemu_irq *ioapic_irq_hack;
 
@@ -475,6 +477,8 @@ static void *bochs_bios_init(void)
     if (smbios_table)
         fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
                          smbios_table, smbios_len);
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_RESERVE, (uint8_t *)&e820_reserve,
+                     sizeof(struct fw_cfg_e820_reserve));
 
     /* allocate memory for the NUMA channel: one (64bit) word for the number
      * of nodes, one word for each VCPU->node and one word for each node to
Index: qemu-kvm/kvm.h
===================================================================
--- qemu-kvm.orig/kvm.h
+++ qemu-kvm/kvm.h
@@ -101,6 +101,8 @@ void kvm_arch_reset_vcpu(CPUState *env);
 struct kvm_guest_debug;
 struct kvm_debug_exit_arch;
 
+extern struct fw_cfg_e820_reserve e820_reserve;
+
 struct kvm_sw_breakpoint {
     target_ulong pc;
     target_ulong saved_insn;
Index: qemu-kvm/qemu-kvm-x86.c
===================================================================
--- qemu-kvm.orig/qemu-kvm-x86.c
+++ qemu-kvm/qemu-kvm-x86.c
@@ -23,6 +23,7 @@
 
 #include "kvm.h"
 #include "hw/pc.h"
+#include "hw/fw_cfg.h"
 
 #define MSR_IA32_TSC		0x10
 
@@ -37,6 +38,11 @@ int kvm_set_tss_addr(kvm_context_t kvm, 
 {
 #ifdef KVM_CAP_SET_TSS_ADDR
 	int r;
+        /*
+         * Tell fw_cfg to notify the BIOS to reserve the range.
+         */
+        e820_reserve.addr = addr;
+        e820_reserve.length = 0x4000;
 
 	r = kvm_ioctl(kvm_state, KVM_CHECK_EXTENSION, KVM_CAP_SET_TSS_ADDR);
 	if (r > 0) {
Index: qemu-kvm/target-i386/kvm.c
===================================================================
--- qemu-kvm.orig/target-i386/kvm.c
+++ qemu-kvm/target-i386/kvm.c
@@ -25,6 +25,8 @@
 #include "gdbstub.h"
 #include "host-utils.h"
 
+extern struct fw_cfg_e820_reserve e820_reserve;
+
 #ifdef KVM_UPSTREAM
 //#define DEBUG_KVM
 
@@ -298,6 +300,11 @@ int kvm_arch_init(KVMState *s, int smp_c
      * as unavaible memory.  FIXME, need to ensure the e820 map deals with
      * this?
      */
+    /*
+     * Tell fw_cfg to notify the BIOS to reserve the range.
+     */
+    e820_reserve.addr = 0xfffbc000;
+    e820_reserve.length = 0x4000;
     return kvm_vm_ioctl(s, KVM_SET_TSS_ADDR, 0xfffbd000);
 }
                     

[Qemu-devel] [PATCH] QEMU-KVM - provide e820 reserve through qemu_cfg

[Jes Sorensen]

[-- Attachment #1: Type: text/plain, Size: 100 bytes --]

Hi,

This is the QEMU-KVM bits for providing the e820-reserve space through
qemu-cfg.

Cheers,
Jes


[-- Attachment #2: 0011-qemu-kvm-e820-reserve.patch --]
[-- Type: text/plain, Size: 3549 bytes --]

Use qemu-cfg to notify the BIOS of the location of the TSS range to
reserve in the e820 table, to avoid relying on hard coded values.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

---
 hw/fw_cfg.h       |    5 +++++
 hw/pc.c           |    4 ++++
 kvm.h             |    2 ++
 qemu-kvm-x86.c    |    6 ++++++
 target-i386/kvm.c |    7 +++++++
 5 files changed, 24 insertions(+)

Index: qemu-kvm/hw/fw_cfg.h
===================================================================
--- qemu-kvm.orig/hw/fw_cfg.h
+++ qemu-kvm/hw/fw_cfg.h
@@ -67,4 +67,9 @@ FWCfgState *fw_cfg_init(uint32_t ctl_por
 
 #endif /* NO_QEMU_PROTOS */
 
+struct fw_cfg_e820_reserve {
+    uint32_t addr;
+    uint32_t length;
+};
+
 #endif
Index: qemu-kvm/hw/pc.c
===================================================================
--- qemu-kvm.orig/hw/pc.c
+++ qemu-kvm/hw/pc.c
@@ -66,6 +66,7 @@
 #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_RESERVE (FW_CFG_ARCH_LOCAL + 3)
 
 #define MAX_IDE_BUS 2
 
@@ -73,6 +74,7 @@ static fdctrl_t *floppy_controller;
 static RTCState *rtc_state;
 static PITState *pit;
 static PCII440FXState *i440fx_state;
+struct fw_cfg_e820_reserve e820_reserve;
 
 qemu_irq *ioapic_irq_hack;
 
@@ -475,6 +477,8 @@ static void *bochs_bios_init(void)
     if (smbios_table)
         fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
                          smbios_table, smbios_len);
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_RESERVE, (uint8_t *)&e820_reserve,
+                     sizeof(struct fw_cfg_e820_reserve));
 
     /* allocate memory for the NUMA channel: one (64bit) word for the number
      * of nodes, one word for each VCPU->node and one word for each node to
Index: qemu-kvm/kvm.h
===================================================================
--- qemu-kvm.orig/kvm.h
+++ qemu-kvm/kvm.h
@@ -101,6 +101,8 @@ void kvm_arch_reset_vcpu(CPUState *env);
 struct kvm_guest_debug;
 struct kvm_debug_exit_arch;
 
+extern struct fw_cfg_e820_reserve e820_reserve;
+
 struct kvm_sw_breakpoint {
     target_ulong pc;
     target_ulong saved_insn;
Index: qemu-kvm/qemu-kvm-x86.c
===================================================================
--- qemu-kvm.orig/qemu-kvm-x86.c
+++ qemu-kvm/qemu-kvm-x86.c
@@ -23,6 +23,7 @@
 
 #include "kvm.h"
 #include "hw/pc.h"
+#include "hw/fw_cfg.h"
 
 #define MSR_IA32_TSC		0x10
 
@@ -37,6 +38,11 @@ int kvm_set_tss_addr(kvm_context_t kvm, 
 {
 #ifdef KVM_CAP_SET_TSS_ADDR
 	int r;
+        /*
+         * Tell fw_cfg to notify the BIOS to reserve the range.
+         */
+        e820_reserve.addr = addr;
+        e820_reserve.length = 0x4000;
 
 	r = kvm_ioctl(kvm_state, KVM_CHECK_EXTENSION, KVM_CAP_SET_TSS_ADDR);
 	if (r > 0) {
Index: qemu-kvm/target-i386/kvm.c
===================================================================
--- qemu-kvm.orig/target-i386/kvm.c
+++ qemu-kvm/target-i386/kvm.c
@@ -25,6 +25,8 @@
 #include "gdbstub.h"
 #include "host-utils.h"
 
+extern struct fw_cfg_e820_reserve e820_reserve;
+
 #ifdef KVM_UPSTREAM
 //#define DEBUG_KVM
 
@@ -298,6 +300,11 @@ int kvm_arch_init(KVMState *s, int smp_c
      * as unavaible memory.  FIXME, need to ensure the e820 map deals with
      * this?
      */
+    /*
+     * Tell fw_cfg to notify the BIOS to reserve the range.
+     */
+    e820_reserve.addr = 0xfffbc000;
+    e820_reserve.length = 0x4000;
     return kvm_vm_ioctl(s, KVM_SET_TSS_ADDR, 0xfffbd000);
 }
                     

[PATCH] QEMU - provide e820 reserve through qemu_cfg

[Jes Sorensen]

[-- Attachment #1: Type: text/plain, Size: 98 bytes --]

Hi,

This is the QEMU patch for providing the e820-reserve space through
qemu-cfg.

Cheers,
Jes



[-- Attachment #2: 0011-qemu-e820-reserve.patch --]
[-- Type: text/plain, Size: 2597 bytes --]

Use qemu-cfg to notify the BIOS of the location of the TSS range to
reserve in the e820 table, to avoid relying on hard coded values.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

---
 hw/pc.c           |    5 +++++
 kvm.h             |    7 +++++++
 target-i386/kvm.c |    5 +++++
 3 files changed, 17 insertions(+)

Index: qemu/hw/pc.c
===================================================================
--- qemu.orig/hw/pc.c
+++ qemu/hw/pc.c
@@ -45,6 +45,7 @@
 #include "loader.h"
 #include "elf.h"
 #include "multiboot.h"
+#include "kvm.h"
 
 /* output Bochs bios info messages */
 //#define DEBUG_BIOS
@@ -59,6 +60,7 @@
 #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_RESERVE (FW_CFG_ARCH_LOCAL + 3)
 
 #define MAX_IDE_BUS 2
 
@@ -66,6 +68,7 @@ static fdctrl_t *floppy_controller;
 static RTCState *rtc_state;
 static PITState *pit;
 static PCII440FXState *i440fx_state;
+struct fw_cfg_e820_reserve e820_reserve;
 
 typedef struct isa_irq_state {
     qemu_irq *i8259;
@@ -466,6 +469,8 @@ static void *bochs_bios_init(void)
     if (smbios_table)
         fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
                          smbios_table, smbios_len);
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_RESERVE, (uint8_t *)&e820_reserve,
+                     sizeof(struct fw_cfg_e820_reserve));
 
     /* allocate memory for the NUMA channel: one (64bit) word for the number
      * of nodes, one word for each VCPU->node and one word for each node to
Index: qemu/kvm.h
===================================================================
--- qemu.orig/kvm.h
+++ qemu/kvm.h
@@ -96,6 +96,13 @@ void kvm_arch_reset_vcpu(CPUState *env);
 struct kvm_guest_debug;
 struct kvm_debug_exit_arch;
 
+struct fw_cfg_e820_reserve {
+    uint32_t addr;
+    uint32_t length;
+};
+
+extern struct fw_cfg_e820_reserve e820_reserve;
+
 struct kvm_sw_breakpoint {
     target_ulong pc;
     target_ulong saved_insn;
Index: qemu/target-i386/kvm.c
===================================================================
--- qemu.orig/target-i386/kvm.c
+++ qemu/target-i386/kvm.c
@@ -356,6 +356,11 @@ int kvm_arch_init(KVMState *s, int smp_c
      * as unavaible memory.  FIXME, need to ensure the e820 map deals with
      * this?
      */
+    /*
+     * Tell fw_cfg to notify the BIOS to reserve the range.
+     */
+    e820_reserve.addr = 0xfffbc000;
+    e820_reserve.length = 0x4000;
     return kvm_vm_ioctl(s, KVM_SET_TSS_ADDR, 0xfffbd000);
 }
                     

[Qemu-devel] [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Jes Sorensen]

[-- Attachment #1: Type: text/plain, Size: 98 bytes --]

Hi,

This is the QEMU patch for providing the e820-reserve space through
qemu-cfg.

Cheers,
Jes



[-- Attachment #2: 0011-qemu-e820-reserve.patch --]
[-- Type: text/plain, Size: 2597 bytes --]

Use qemu-cfg to notify the BIOS of the location of the TSS range to
reserve in the e820 table, to avoid relying on hard coded values.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

---
 hw/pc.c           |    5 +++++
 kvm.h             |    7 +++++++
 target-i386/kvm.c |    5 +++++
 3 files changed, 17 insertions(+)

Index: qemu/hw/pc.c
===================================================================
--- qemu.orig/hw/pc.c
+++ qemu/hw/pc.c
@@ -45,6 +45,7 @@
 #include "loader.h"
 #include "elf.h"
 #include "multiboot.h"
+#include "kvm.h"
 
 /* output Bochs bios info messages */
 //#define DEBUG_BIOS
@@ -59,6 +60,7 @@
 #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_RESERVE (FW_CFG_ARCH_LOCAL + 3)
 
 #define MAX_IDE_BUS 2
 
@@ -66,6 +68,7 @@ static fdctrl_t *floppy_controller;
 static RTCState *rtc_state;
 static PITState *pit;
 static PCII440FXState *i440fx_state;
+struct fw_cfg_e820_reserve e820_reserve;
 
 typedef struct isa_irq_state {
     qemu_irq *i8259;
@@ -466,6 +469,8 @@ static void *bochs_bios_init(void)
     if (smbios_table)
         fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
                          smbios_table, smbios_len);
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_RESERVE, (uint8_t *)&e820_reserve,
+                     sizeof(struct fw_cfg_e820_reserve));
 
     /* allocate memory for the NUMA channel: one (64bit) word for the number
      * of nodes, one word for each VCPU->node and one word for each node to
Index: qemu/kvm.h
===================================================================
--- qemu.orig/kvm.h
+++ qemu/kvm.h
@@ -96,6 +96,13 @@ void kvm_arch_reset_vcpu(CPUState *env);
 struct kvm_guest_debug;
 struct kvm_debug_exit_arch;
 
+struct fw_cfg_e820_reserve {
+    uint32_t addr;
+    uint32_t length;
+};
+
+extern struct fw_cfg_e820_reserve e820_reserve;
+
 struct kvm_sw_breakpoint {
     target_ulong pc;
     target_ulong saved_insn;
Index: qemu/target-i386/kvm.c
===================================================================
--- qemu.orig/target-i386/kvm.c
+++ qemu/target-i386/kvm.c
@@ -356,6 +356,11 @@ int kvm_arch_init(KVMState *s, int smp_c
      * as unavaible memory.  FIXME, need to ensure the e820 map deals with
      * this?
      */
+    /*
+     * Tell fw_cfg to notify the BIOS to reserve the range.
+     */
+    e820_reserve.addr = 0xfffbc000;
+    e820_reserve.length = 0x4000;
     return kvm_vm_ioctl(s, KVM_SET_TSS_ADDR, 0xfffbd000);
 }
                     

Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Alexander Graf]

On 25.01.2010, at 17:52, Jes Sorensen wrote:

> Hi,
> 
> This is the QEMU patch for providing the e820-reserve space through
> qemu-cfg.

Howdy. Congratulations to the new mail address - looks neat ;-).


Two comments:

1) I don't see how passing a single region is any help. I'd rather like to see a device tree like table structure
You'd get one variable for len of the table, one with the contents. So for a universal reserved region specifier you'd get:

<u64 base><u64 len>

Then have len=2 and put data in the table:

<u64 base1><u64 len1><u64 base2><u64 len2>

That way we'd get 2 entries and the chance to enhance them later on. In fact, it might even make sense to pass the whole table in such a form. That way qemu generates all of the e820 tables and we can declare whatever we want. Just add a type field in the table.

2) Please inline patches. They showed up as attachments here, making them really hard to comment on.


Alex

[Qemu-devel] Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Alexander Graf]

On 25.01.2010, at 17:52, Jes Sorensen wrote:

> Hi,
> 
> This is the QEMU patch for providing the e820-reserve space through
> qemu-cfg.

Howdy. Congratulations to the new mail address - looks neat ;-).


Two comments:

1) I don't see how passing a single region is any help. I'd rather like to see a device tree like table structure
You'd get one variable for len of the table, one with the contents. So for a universal reserved region specifier you'd get:

<u64 base><u64 len>

Then have len=2 and put data in the table:

<u64 base1><u64 len1><u64 base2><u64 len2>

That way we'd get 2 entries and the chance to enhance them later on. In fact, it might even make sense to pass the whole table in such a form. That way qemu generates all of the e820 tables and we can declare whatever we want. Just add a type field in the table.

2) Please inline patches. They showed up as attachments here, making them really hard to comment on.


Alex

Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Jes Sorensen]

On 01/25/10 17:58, Alexander Graf wrote:
> Howdy. Congratulations to the new mail address - looks neat ;-).

:-)

> Two comments:
>
> 1) I don't see how passing a single region is any help. I'd rather like to see a device tree like table structure
> You'd get one variable for len of the table, one with the contents. So for a universal reserved region specifier you'd get:
>
> <u64 base><u64 len>
>
> Then have len=2 and put data in the table:
>
> <u64 base1><u64 len1><u64 base2><u64 len2>
>
> That way we'd get 2 entries and the chance to enhance them later on. In fact, it might even make sense to pass the whole table in such a form. That way qemu generates all of the e820 tables and we can declare whatever we want. Just add a type field in the table.

I am fine with having QEMU build the e820 tables completely if there is
a consensus to take that path.

> 2) Please inline patches. They showed up as attachments here, making them really hard to comment on.

Sorry Thunderbug doesn't do that well, but they should be attached as
txt?

Cheers,
Jes

[Qemu-devel] Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Jes Sorensen]

On 01/25/10 17:58, Alexander Graf wrote:
> Howdy. Congratulations to the new mail address - looks neat ;-).

:-)

> Two comments:
>
> 1) I don't see how passing a single region is any help. I'd rather like to see a device tree like table structure
> You'd get one variable for len of the table, one with the contents. So for a universal reserved region specifier you'd get:
>
> <u64 base><u64 len>
>
> Then have len=2 and put data in the table:
>
> <u64 base1><u64 len1><u64 base2><u64 len2>
>
> That way we'd get 2 entries and the chance to enhance them later on. In fact, it might even make sense to pass the whole table in such a form. That way qemu generates all of the e820 tables and we can declare whatever we want. Just add a type field in the table.

I am fine with having QEMU build the e820 tables completely if there is
a consensus to take that path.

> 2) Please inline patches. They showed up as attachments here, making them really hard to comment on.

Sorry Thunderbug doesn't do that well, but they should be attached as
txt?

Cheers,
Jes

Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Alexander Graf]

Am 25.01.2010 um 18:13 schrieb Jes Sorensen <Jes.Sorensen@redhat.com>:

> On 01/25/10 17:58, Alexander Graf wrote:
>> Howdy. Congratulations to the new mail address - looks neat ;-).
>
> :-)
>
>> Two comments:
>>
>> 1) I don't see how passing a single region is any help. I'd rather  
>> like to see a device tree like table structure
>> You'd get one variable for len of the table, one with the contents.  
>> So for a universal reserved region specifier you'd get:
>>
>> <u64 base><u64 len>
>>
>> Then have len=2 and put data in the table:
>>
>> <u64 base1><u64 len1><u64 base2><u64 len2>
>>
>> That way we'd get 2 entries and the chance to enhance them later  
>> on. In fact, it might even make sense to pass the whole table in  
>> such a form. That way qemu generates all of the e820 tables and we  
>> can declare whatever we want. Just add a type field in the table.
>
> I am fine with having QEMU build the e820 tables completely if there  
> is
> a consensus to take that path.

I agree. We better get this right :-). I don't want to maintain 5  
versions of an 380 fw_cfg interface.

>
>> 2) Please inline patches. They showed up as attachments here,  
>> making them really hard to comment on.
>
> Sorry Thunderbug doesn't do that well, but they should be attached as
> txt?

I suppose so, but Apple Mail still doesn't show it and definitely  
doesn't let me comment on it.

The easiest way for me so far has been to use git-send-email and a  
bash alias to fill in login information.


Alex
>

[Qemu-devel] Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Alexander Graf]

Am 25.01.2010 um 18:13 schrieb Jes Sorensen <Jes.Sorensen@redhat.com>:

> On 01/25/10 17:58, Alexander Graf wrote:
>> Howdy. Congratulations to the new mail address - looks neat ;-).
>
> :-)
>
>> Two comments:
>>
>> 1) I don't see how passing a single region is any help. I'd rather  
>> like to see a device tree like table structure
>> You'd get one variable for len of the table, one with the contents.  
>> So for a universal reserved region specifier you'd get:
>>
>> <u64 base><u64 len>
>>
>> Then have len=2 and put data in the table:
>>
>> <u64 base1><u64 len1><u64 base2><u64 len2>
>>
>> That way we'd get 2 entries and the chance to enhance them later  
>> on. In fact, it might even make sense to pass the whole table in  
>> such a form. That way qemu generates all of the e820 tables and we  
>> can declare whatever we want. Just add a type field in the table.
>
> I am fine with having QEMU build the e820 tables completely if there  
> is
> a consensus to take that path.

I agree. We better get this right :-). I don't want to maintain 5  
versions of an 380 fw_cfg interface.

>
>> 2) Please inline patches. They showed up as attachments here,  
>> making them really hard to comment on.
>
> Sorry Thunderbug doesn't do that well, but they should be attached as
> txt?

I suppose so, but Apple Mail still doesn't show it and definitely  
doesn't let me comment on it.

The easiest way for me so far has been to use git-send-email and a  
bash alias to fill in login information.


Alex
>

Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Jes Sorensen]

On 01/25/10 18:28, Alexander Graf wrote:
>>> That way we'd get 2 entries and the chance to enhance them later on.
>>> In fact, it might even make sense to pass the whole table in such a
>>> form. That way qemu generates all of the e820 tables and we can
>>> declare whatever we want. Just add a type field in the table.
>>
>> I am fine with having QEMU build the e820 tables completely if there is
>> a consensus to take that path.
>
> I agree. We better get this right :-). I don't want to maintain 5
> versions of an 380 fw_cfg interface.

Looking at the internals, some of the e820 entries are based on compile
time constants for the BIOS, so it will be hard to pass those from
QEMU, but we could do it in a way so we pass a number of additional
e820 entries. Ie. address, length, and type.

What do you think?

Cheers,
Jes

[Qemu-devel] Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Jes Sorensen]

On 01/25/10 18:28, Alexander Graf wrote:
>>> That way we'd get 2 entries and the chance to enhance them later on.
>>> In fact, it might even make sense to pass the whole table in such a
>>> form. That way qemu generates all of the e820 tables and we can
>>> declare whatever we want. Just add a type field in the table.
>>
>> I am fine with having QEMU build the e820 tables completely if there is
>> a consensus to take that path.
>
> I agree. We better get this right :-). I don't want to maintain 5
> versions of an 380 fw_cfg interface.

Looking at the internals, some of the e820 entries are based on compile
time constants for the BIOS, so it will be hard to pass those from
QEMU, but we could do it in a way so we pass a number of additional
e820 entries. Ie. address, length, and type.

What do you think?

Cheers,
Jes

Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Alexander Graf]

On 25.01.2010, at 18:46, Jes Sorensen wrote:

> On 01/25/10 18:28, Alexander Graf wrote:
>>>> That way we'd get 2 entries and the chance to enhance them later on.
>>>> In fact, it might even make sense to pass the whole table in such a
>>>> form. That way qemu generates all of the e820 tables and we can
>>>> declare whatever we want. Just add a type field in the table.
>>> 
>>> I am fine with having QEMU build the e820 tables completely if there is
>>> a consensus to take that path.
>> 
>> I agree. We better get this right :-). I don't want to maintain 5
>> versions of an 380 fw_cfg interface.
> 
> Looking at the internals, some of the e820 entries are based on compile
> time constants for the BIOS, so it will be hard to pass those from
> QEMU, but we could do it in a way so we pass a number of additional
> e820 entries. Ie. address, length, and type.

Yes, sounds good. Should be fairly extensible then. What about memory holes? Do we need to take care of them?

Alex

[Qemu-devel] Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Alexander Graf]

On 25.01.2010, at 18:46, Jes Sorensen wrote:

> On 01/25/10 18:28, Alexander Graf wrote:
>>>> That way we'd get 2 entries and the chance to enhance them later on.
>>>> In fact, it might even make sense to pass the whole table in such a
>>>> form. That way qemu generates all of the e820 tables and we can
>>>> declare whatever we want. Just add a type field in the table.
>>> 
>>> I am fine with having QEMU build the e820 tables completely if there is
>>> a consensus to take that path.
>> 
>> I agree. We better get this right :-). I don't want to maintain 5
>> versions of an 380 fw_cfg interface.
> 
> Looking at the internals, some of the e820 entries are based on compile
> time constants for the BIOS, so it will be hard to pass those from
> QEMU, but we could do it in a way so we pass a number of additional
> e820 entries. Ie. address, length, and type.

Yes, sounds good. Should be fairly extensible then. What about memory holes? Do we need to take care of them?

Alex

Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Anthony Liguori]

On 01/25/2010 02:04 PM, Alexander Graf wrote:
> On 25.01.2010, at 18:46, Jes Sorensen wrote:
>
>    
>> On 01/25/10 18:28, Alexander Graf wrote:
>>      
>>>>> That way we'd get 2 entries and the chance to enhance them later on.
>>>>> In fact, it might even make sense to pass the whole table in such a
>>>>> form. That way qemu generates all of the e820 tables and we can
>>>>> declare whatever we want. Just add a type field in the table.
>>>>>            
>>>> I am fine with having QEMU build the e820 tables completely if there is
>>>> a consensus to take that path.
>>>>          
>>> I agree. We better get this right :-). I don't want to maintain 5
>>> versions of an 380 fw_cfg interface.
>>>        
>> Looking at the internals, some of the e820 entries are based on compile
>> time constants for the BIOS, so it will be hard to pass those from
>> QEMU, but we could do it in a way so we pass a number of additional
>> e820 entries. Ie. address, length, and type.
>>      
> Yes, sounds good. Should be fairly extensible then. What about memory holes? Do we need to take care of them?
>    

It would be nice for QEMU to be able to add additional e820 regions that 
don't necessarily fit the standard layout model.

For instance, I've thought a number of times about using a large 
reserved region as a shared memory mechanism.

But we certainly need to allow the BIOS to define the regions it needs 
to know about.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Anthony Liguori]

On 01/25/2010 02:04 PM, Alexander Graf wrote:
> On 25.01.2010, at 18:46, Jes Sorensen wrote:
>
>    
>> On 01/25/10 18:28, Alexander Graf wrote:
>>      
>>>>> That way we'd get 2 entries and the chance to enhance them later on.
>>>>> In fact, it might even make sense to pass the whole table in such a
>>>>> form. That way qemu generates all of the e820 tables and we can
>>>>> declare whatever we want. Just add a type field in the table.
>>>>>            
>>>> I am fine with having QEMU build the e820 tables completely if there is
>>>> a consensus to take that path.
>>>>          
>>> I agree. We better get this right :-). I don't want to maintain 5
>>> versions of an 380 fw_cfg interface.
>>>        
>> Looking at the internals, some of the e820 entries are based on compile
>> time constants for the BIOS, so it will be hard to pass those from
>> QEMU, but we could do it in a way so we pass a number of additional
>> e820 entries. Ie. address, length, and type.
>>      
> Yes, sounds good. Should be fairly extensible then. What about memory holes? Do we need to take care of them?
>    

It would be nice for QEMU to be able to add additional e820 regions that 
don't necessarily fit the standard layout model.

For instance, I've thought a number of times about using a large 
reserved region as a shared memory mechanism.

But we certainly need to allow the BIOS to define the regions it needs 
to know about.

Regards,

Anthony Liguori

Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Jes Sorensen]

On 01/25/10 21:14, Anthony Liguori wrote:
> On 01/25/2010 02:04 PM, Alexander Graf wrote:
>> Yes, sounds good. Should be fairly extensible then. What about memory
>> holes? Do we need to take care of them?
>
> It would be nice for QEMU to be able to add additional e820 regions that
> don't necessarily fit the standard layout model.
>
> For instance, I've thought a number of times about using a large
> reserved region as a shared memory mechanism.
>
> But we certainly need to allow the BIOS to define the regions it needs
> to know about.

I think it should be easy to accommodate using the scheme I am
suggesting. It would require some basic testing for conflicts in the
BIOS, but otherwise it should pretty much allow you to specify any
region you want as a reserved block.

Only problem is that we don't really have a way to pass back info
saying 'you messed up trying to pinch an area that the BIOS wants
for itself'.

I'll take a look at it.

Cheers,
Jes

[Qemu-devel] Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Jes Sorensen]

On 01/25/10 21:14, Anthony Liguori wrote:
> On 01/25/2010 02:04 PM, Alexander Graf wrote:
>> Yes, sounds good. Should be fairly extensible then. What about memory
>> holes? Do we need to take care of them?
>
> It would be nice for QEMU to be able to add additional e820 regions that
> don't necessarily fit the standard layout model.
>
> For instance, I've thought a number of times about using a large
> reserved region as a shared memory mechanism.
>
> But we certainly need to allow the BIOS to define the regions it needs
> to know about.

I think it should be easy to accommodate using the scheme I am
suggesting. It would require some basic testing for conflicts in the
BIOS, but otherwise it should pretty much allow you to specify any
region you want as a reserved block.

Only problem is that we don't really have a way to pass back info
saying 'you messed up trying to pinch an area that the BIOS wants
for itself'.

I'll take a look at it.

Cheers,
Jes

Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Alexander Graf]

On 25.01.2010, at 22:05, Jes Sorensen wrote:

> On 01/25/10 21:14, Anthony Liguori wrote:
>> On 01/25/2010 02:04 PM, Alexander Graf wrote:
>>> Yes, sounds good. Should be fairly extensible then. What about memory
>>> holes? Do we need to take care of them?
>> 
>> It would be nice for QEMU to be able to add additional e820 regions that
>> don't necessarily fit the standard layout model.
>> 
>> For instance, I've thought a number of times about using a large
>> reserved region as a shared memory mechanism.
>> 
>> But we certainly need to allow the BIOS to define the regions it needs
>> to know about.
> 
> I think it should be easy to accommodate using the scheme I am
> suggesting. It would require some basic testing for conflicts in the
> BIOS, but otherwise it should pretty much allow you to specify any
> region you want as a reserved block.
> 
> Only problem is that we don't really have a way to pass back info
> saying 'you messed up trying to pinch an area that the BIOS wants
> for itself'.

Eh - the BIOS shouldn't even try to use regions that are declared as reserved using this interface.
I guess we're mostly talking about DMI and ACPI tables. They can be anywhere in RAM.

Alex

[Qemu-devel] Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Alexander Graf]

On 25.01.2010, at 22:05, Jes Sorensen wrote:

> On 01/25/10 21:14, Anthony Liguori wrote:
>> On 01/25/2010 02:04 PM, Alexander Graf wrote:
>>> Yes, sounds good. Should be fairly extensible then. What about memory
>>> holes? Do we need to take care of them?
>> 
>> It would be nice for QEMU to be able to add additional e820 regions that
>> don't necessarily fit the standard layout model.
>> 
>> For instance, I've thought a number of times about using a large
>> reserved region as a shared memory mechanism.
>> 
>> But we certainly need to allow the BIOS to define the regions it needs
>> to know about.
> 
> I think it should be easy to accommodate using the scheme I am
> suggesting. It would require some basic testing for conflicts in the
> BIOS, but otherwise it should pretty much allow you to specify any
> region you want as a reserved block.
> 
> Only problem is that we don't really have a way to pass back info
> saying 'you messed up trying to pinch an area that the BIOS wants
> for itself'.

Eh - the BIOS shouldn't even try to use regions that are declared as reserved using this interface.
I guess we're mostly talking about DMI and ACPI tables. They can be anywhere in RAM.

Alex

Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Jes Sorensen]

On 01/25/10 22:08, Alexander Graf wrote:
>
> On 25.01.2010, at 22:05, Jes Sorensen wrote:
>> Only problem is that we don't really have a way to pass back info
>> saying 'you messed up trying to pinch an area that the BIOS wants
>> for itself'.
>
> Eh - the BIOS shouldn't even try to use regions that are declared as reserved using this interface.
> I guess we're mostly talking about DMI and ACPI tables. They can be anywhere in RAM.

What I had in mind with the above was the situation where a user tries
to reserve a region that is hardcoded into the BIOS, such as the address
of the BIOS text/data etc.

I don't think it would be a real problem anyway, if some user wants to
play with it, they have to take the risk of shooting themself in the
foot :)

Cheers,
Jes

[Qemu-devel] Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Jes Sorensen]

On 01/25/10 22:08, Alexander Graf wrote:
>
> On 25.01.2010, at 22:05, Jes Sorensen wrote:
>> Only problem is that we don't really have a way to pass back info
>> saying 'you messed up trying to pinch an area that the BIOS wants
>> for itself'.
>
> Eh - the BIOS shouldn't even try to use regions that are declared as reserved using this interface.
> I guess we're mostly talking about DMI and ACPI tables. They can be anywhere in RAM.

What I had in mind with the above was the situation where a user tries
to reserve a region that is hardcoded into the BIOS, such as the address
of the BIOS text/data etc.

I don't think it would be a real problem anyway, if some user wants to
play with it, they have to take the risk of shooting themself in the
foot :)

Cheers,
Jes

Re: [PATCH] Seabios - read e820 reserve from qemu_cfg

[Kevin O'Connor]

On Mon, Jan 25, 2010 at 05:46:42PM +0100, Jes Sorensen wrote:
> Hi,
> 
> Right now KVM/QEMU relies on hard coded values in Seabios for the
> reserved area for the TSS pages and the EPT page.
> 
> I'd like to suggest we change this to pass the value from QEMU via
> qemu-cfg making it possible to move it around dynamically in the future.
> 
> Attached is a patch to Seabios for this, which defaults to the current
> hard coded value if no value is provided by qemu-cfg. We can remove
> the backwards compatibility later.
> 
> I'll post the QEMU patches for upstream QEMU and QEMU-KVM in a minute.
> 
> Comments most welcome!

I like the idea, but I think it would be better to pass a list of e820
entries explicitly.  That is, pass an array of:

struct e820entry {
    u64 start;
    u64 size;
    u32 type;
};

where 'type' uses the standard e820 definitions.  That way, SeaBIOS
can just walk through the list and add them to its e820 map.  BTW,
this is what SeaBIOS does when running under coreboot (coreboot passes
a memory map as part of the "coreboot tables").  SeaBIOS is already
smart enough to not use any high memory addresses marked as reserved.

-Kevin

[Qemu-devel] Re: [PATCH] Seabios - read e820 reserve from qemu_cfg

[Kevin O'Connor]

On Mon, Jan 25, 2010 at 05:46:42PM +0100, Jes Sorensen wrote:
> Hi,
> 
> Right now KVM/QEMU relies on hard coded values in Seabios for the
> reserved area for the TSS pages and the EPT page.
> 
> I'd like to suggest we change this to pass the value from QEMU via
> qemu-cfg making it possible to move it around dynamically in the future.
> 
> Attached is a patch to Seabios for this, which defaults to the current
> hard coded value if no value is provided by qemu-cfg. We can remove
> the backwards compatibility later.
> 
> I'll post the QEMU patches for upstream QEMU and QEMU-KVM in a minute.
> 
> Comments most welcome!

I like the idea, but I think it would be better to pass a list of e820
entries explicitly.  That is, pass an array of:

struct e820entry {
    u64 start;
    u64 size;
    u32 type;
};

where 'type' uses the standard e820 definitions.  That way, SeaBIOS
can just walk through the list and add them to its e820 map.  BTW,
this is what SeaBIOS does when running under coreboot (coreboot passes
a memory map as part of the "coreboot tables").  SeaBIOS is already
smart enough to not use any high memory addresses marked as reserved.

-Kevin

Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Gleb Natapov]

On Mon, Jan 25, 2010 at 06:13:35PM +0100, Jes Sorensen wrote:
> On 01/25/10 17:58, Alexander Graf wrote:
> >Howdy. Congratulations to the new mail address - looks neat ;-).
> 
> :-)
> 
> >Two comments:
> >
> >1) I don't see how passing a single region is any help. I'd rather like to see a device tree like table structure
> >You'd get one variable for len of the table, one with the contents. So for a universal reserved region specifier you'd get:
> >
> ><u64 base><u64 len>
> >
> >Then have len=2 and put data in the table:
> >
> ><u64 base1><u64 len1><u64 base2><u64 len2>
> >
> >That way we'd get 2 entries and the chance to enhance them later on. In fact, it might even make sense to pass the whole table in such a form. That way qemu generates all of the e820 tables and we can declare whatever we want. Just add a type field in the table.
> 
> I am fine with having QEMU build the e820 tables completely if there is
> a consensus to take that path.
> 
QEMU can't build the e820 map completely. There are things it doesn't
know. Like how much memory ACPI tables take and where they are located.

> >2) Please inline patches. They showed up as attachments here, making them really hard to comment on.
> 
> Sorry Thunderbug doesn't do that well, but they should be attached as
> txt?
> 
> Cheers,
> Jes
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

[Qemu-devel] Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Gleb Natapov]

On Mon, Jan 25, 2010 at 06:13:35PM +0100, Jes Sorensen wrote:
> On 01/25/10 17:58, Alexander Graf wrote:
> >Howdy. Congratulations to the new mail address - looks neat ;-).
> 
> :-)
> 
> >Two comments:
> >
> >1) I don't see how passing a single region is any help. I'd rather like to see a device tree like table structure
> >You'd get one variable for len of the table, one with the contents. So for a universal reserved region specifier you'd get:
> >
> ><u64 base><u64 len>
> >
> >Then have len=2 and put data in the table:
> >
> ><u64 base1><u64 len1><u64 base2><u64 len2>
> >
> >That way we'd get 2 entries and the chance to enhance them later on. In fact, it might even make sense to pass the whole table in such a form. That way qemu generates all of the e820 tables and we can declare whatever we want. Just add a type field in the table.
> 
> I am fine with having QEMU build the e820 tables completely if there is
> a consensus to take that path.
> 
QEMU can't build the e820 map completely. There are things it doesn't
know. Like how much memory ACPI tables take and where they are located.

> >2) Please inline patches. They showed up as attachments here, making them really hard to comment on.
> 
> Sorry Thunderbug doesn't do that well, but they should be attached as
> txt?
> 
> Cheers,
> Jes
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Jes Sorensen]

On 01/26/10 07:46, Gleb Natapov wrote:
> On Mon, Jan 25, 2010 at 06:13:35PM +0100, Jes Sorensen wrote:
>> I am fine with having QEMU build the e820 tables completely if there is
>> a consensus to take that path.
>>
> QEMU can't build the e820 map completely. There are things it doesn't
> know. Like how much memory ACPI tables take and where they are located.

Good point!

I think the conclusion is to do a load-extra-tables kinda interface 
allowing QEMU to pass in a bunch of them, but leaving things like the
ACPI space for the BIOS to reserve.

Cheers,
Jes

[Qemu-devel] Re: [PATCH] QEMU - provide e820 reserve through qemu_cfg

[Jes Sorensen]

On 01/26/10 07:46, Gleb Natapov wrote:
> On Mon, Jan 25, 2010 at 06:13:35PM +0100, Jes Sorensen wrote:
>> I am fine with having QEMU build the e820 tables completely if there is
>> a consensus to take that path.
>>
> QEMU can't build the e820 map completely. There are things it doesn't
> know. Like how much memory ACPI tables take and where they are located.

Good point!

I think the conclusion is to do a load-extra-tables kinda interface 
allowing QEMU to pass in a bunch of them, but leaving things like the
ACPI space for the BIOS to reserve.

Cheers,
Jes

[PATCH] Seabios - read e820 table from qemu_cfg

[Jes Sorensen]

[-- Attachment #1: Type: text/plain, Size: 458 bytes --]

Hi,

Based on the feedback I received over the e820 reserve patch, I have
changed it to have QEMU pass in a list of entries that can cover more
than just the TSS/EPT range. This should provide the flexibility that
people were asking for.

The Seabios portion should allow for unlimited sized tables in theory,
whereas for QEMU I have set a fixed limit for now, but it can easily
be extended.

Please let me know what you think of this version!

Cheers,
Jes


[-- Attachment #2: 0005-seabios-e820-table.patch --]
[-- Type: text/plain, Size: 3359 bytes --]

Read optional table of e820 entries from qemu_cfg

Read optional table of e820 entries through qemu_cfg, allowing QEMU to
provide the location of KVM's switch area etc. rather than rely on
hard coded values.

For now, fall back to the old hard coded values for the TSS and EPT
switch page for compatibility reasons. Compatibility code could
possibly be removed in the future.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

---
 src/paravirt.c |   17 +++++++++++++++++
 src/paravirt.h |    9 +++++++++
 src/post.c     |   23 +++++++++++++++++++----
 3 files changed, 45 insertions(+), 4 deletions(-)

Index: seabios/src/paravirt.c
===================================================================
--- seabios.orig/src/paravirt.c
+++ seabios/src/paravirt.c
@@ -132,6 +132,23 @@ u16 qemu_cfg_smbios_entries(void)
     return cnt;
 }
 
+u32 qemu_cfg_e820_entries(void)
+{
+    u32 cnt;
+
+    if (!qemu_cfg_present)
+        return 0;
+
+    qemu_cfg_read_entry(&cnt, QEMU_CFG_E820_TABLE, sizeof(cnt));
+    return cnt;
+}
+
+void* qemu_cfg_e820_load_next(void *addr)
+{
+    qemu_cfg_read(addr, sizeof(struct e820_entry));
+    return addr;
+}
+
 struct smbios_header {
     u16 length;
     u8 type;
Index: seabios/src/paravirt.h
===================================================================
--- seabios.orig/src/paravirt.h
+++ seabios/src/paravirt.h
@@ -36,6 +36,7 @@ static inline int kvm_para_available(voi
 #define QEMU_CFG_ACPI_TABLES		(QEMU_CFG_ARCH_LOCAL + 0)
 #define QEMU_CFG_SMBIOS_ENTRIES		(QEMU_CFG_ARCH_LOCAL + 1)
 #define QEMU_CFG_IRQ0_OVERRIDE		(QEMU_CFG_ARCH_LOCAL + 2)
+#define QEMU_CFG_E820_TABLE		(QEMU_CFG_ARCH_LOCAL + 3)
 
 extern int qemu_cfg_present;
 
@@ -61,8 +62,16 @@ typedef struct QemuCfgFile {
     char name[56];
 } QemuCfgFile;
 
+struct e820_entry {
+    u64 address;
+    u64 length;
+    u32 type;
+};
+
 u16 qemu_cfg_first_file(QemuCfgFile *entry);
 u16 qemu_cfg_next_file(QemuCfgFile *entry);
 u32 qemu_cfg_read_file(QemuCfgFile *entry, void *dst, u32 maxlen);
+u32 qemu_cfg_e820_entries(void);
+void* qemu_cfg_e820_load_next(void *addr);
 
 #endif
Index: seabios/src/post.c
===================================================================
--- seabios.orig/src/post.c
+++ seabios/src/post.c
@@ -135,10 +135,25 @@ ram_probe(void)
              , E820_RESERVED);
     add_e820(BUILD_BIOS_ADDR, BUILD_BIOS_SIZE, E820_RESERVED);
 
-    if (kvm_para_available())
-        // 4 pages before the bios, 3 pages for vmx tss pages, the
-        // other page for EPT real mode pagetable
-        add_e820(0xfffbc000, 4*4096, E820_RESERVED);
+    if (kvm_para_available()) {
+        u32 count;
+
+        count = qemu_cfg_e820_entries();
+        if (count) {
+            struct e820_entry entry;
+            int i;
+
+            for (i = 0; i < count; i++) {
+                qemu_cfg_e820_load_next(&entry);
+                add_e820(entry.address, entry.length, entry.type);
+            }
+        } else {
+            // Backwards compatibility - provide hard coded range.
+            // 4 pages before the bios, 3 pages for vmx tss pages, the
+            // other page for EPT real mode pagetable
+            add_e820(0xfffbc000, 4*4096, E820_RESERVED);
+        }
+    }
 
     dprintf(1, "Ram Size=0x%08x (0x%08x%08x high)\n"
             , RamSize, (u32)(RamSizeOver4G >> 32), (u32)RamSizeOver4G);

[Qemu-devel] [PATCH] Seabios - read e820 table from qemu_cfg

[Jes Sorensen]

[-- Attachment #1: Type: text/plain, Size: 458 bytes --]

Hi,

Based on the feedback I received over the e820 reserve patch, I have
changed it to have QEMU pass in a list of entries that can cover more
than just the TSS/EPT range. This should provide the flexibility that
people were asking for.

The Seabios portion should allow for unlimited sized tables in theory,
whereas for QEMU I have set a fixed limit for now, but it can easily
be extended.

Please let me know what you think of this version!

Cheers,
Jes


[-- Attachment #2: 0005-seabios-e820-table.patch --]
[-- Type: text/plain, Size: 3359 bytes --]

Read optional table of e820 entries from qemu_cfg

Read optional table of e820 entries through qemu_cfg, allowing QEMU to
provide the location of KVM's switch area etc. rather than rely on
hard coded values.

For now, fall back to the old hard coded values for the TSS and EPT
switch page for compatibility reasons. Compatibility code could
possibly be removed in the future.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

---
 src/paravirt.c |   17 +++++++++++++++++
 src/paravirt.h |    9 +++++++++
 src/post.c     |   23 +++++++++++++++++++----
 3 files changed, 45 insertions(+), 4 deletions(-)

Index: seabios/src/paravirt.c
===================================================================
--- seabios.orig/src/paravirt.c
+++ seabios/src/paravirt.c
@@ -132,6 +132,23 @@ u16 qemu_cfg_smbios_entries(void)
     return cnt;
 }
 
+u32 qemu_cfg_e820_entries(void)
+{
+    u32 cnt;
+
+    if (!qemu_cfg_present)
+        return 0;
+
+    qemu_cfg_read_entry(&cnt, QEMU_CFG_E820_TABLE, sizeof(cnt));
+    return cnt;
+}
+
+void* qemu_cfg_e820_load_next(void *addr)
+{
+    qemu_cfg_read(addr, sizeof(struct e820_entry));
+    return addr;
+}
+
 struct smbios_header {
     u16 length;
     u8 type;
Index: seabios/src/paravirt.h
===================================================================
--- seabios.orig/src/paravirt.h
+++ seabios/src/paravirt.h
@@ -36,6 +36,7 @@ static inline int kvm_para_available(voi
 #define QEMU_CFG_ACPI_TABLES		(QEMU_CFG_ARCH_LOCAL + 0)
 #define QEMU_CFG_SMBIOS_ENTRIES		(QEMU_CFG_ARCH_LOCAL + 1)
 #define QEMU_CFG_IRQ0_OVERRIDE		(QEMU_CFG_ARCH_LOCAL + 2)
+#define QEMU_CFG_E820_TABLE		(QEMU_CFG_ARCH_LOCAL + 3)
 
 extern int qemu_cfg_present;
 
@@ -61,8 +62,16 @@ typedef struct QemuCfgFile {
     char name[56];
 } QemuCfgFile;
 
+struct e820_entry {
+    u64 address;
+    u64 length;
+    u32 type;
+};
+
 u16 qemu_cfg_first_file(QemuCfgFile *entry);
 u16 qemu_cfg_next_file(QemuCfgFile *entry);
 u32 qemu_cfg_read_file(QemuCfgFile *entry, void *dst, u32 maxlen);
+u32 qemu_cfg_e820_entries(void);
+void* qemu_cfg_e820_load_next(void *addr);
 
 #endif
Index: seabios/src/post.c
===================================================================
--- seabios.orig/src/post.c
+++ seabios/src/post.c
@@ -135,10 +135,25 @@ ram_probe(void)
              , E820_RESERVED);
     add_e820(BUILD_BIOS_ADDR, BUILD_BIOS_SIZE, E820_RESERVED);
 
-    if (kvm_para_available())
-        // 4 pages before the bios, 3 pages for vmx tss pages, the
-        // other page for EPT real mode pagetable
-        add_e820(0xfffbc000, 4*4096, E820_RESERVED);
+    if (kvm_para_available()) {
+        u32 count;
+
+        count = qemu_cfg_e820_entries();
+        if (count) {
+            struct e820_entry entry;
+            int i;
+
+            for (i = 0; i < count; i++) {
+                qemu_cfg_e820_load_next(&entry);
+                add_e820(entry.address, entry.length, entry.type);
+            }
+        } else {
+            // Backwards compatibility - provide hard coded range.
+            // 4 pages before the bios, 3 pages for vmx tss pages, the
+            // other page for EPT real mode pagetable
+            add_e820(0xfffbc000, 4*4096, E820_RESERVED);
+        }
+    }
 
     dprintf(1, "Ram Size=0x%08x (0x%08x%08x high)\n"
             , RamSize, (u32)(RamSizeOver4G >> 32), (u32)RamSizeOver4G);

[PATCH] QEMU-KVM - provide e820 table via fw_cfg

[Jes Sorensen]

[-- Attachment #1: Type: text/plain, Size: 140 bytes --]

Hi,

This is the QEMU-KVM part of the patch. If we can agree on this
approach, I will do a version for upstream QEMU as well.

Cheers,
Jes


[-- Attachment #2: 0011-qemu-kvm-e820-table.patch --]
[-- Type: text/plain, Size: 3920 bytes --]

Use qemu-cfg to provide the BIOS with an optional table of e820 entries.

Notify the BIOS of the location of the TSS+EPT range to by reserving
it via the e820 table.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

---
 hw/pc.c           |   35 +++++++++++++++++++++++++++++++++++
 hw/pc.h           |    9 +++++++++
 qemu-kvm-x86.c    |    7 +++++++
 target-i386/kvm.c |    7 +++++++
 4 files changed, 58 insertions(+)

Index: qemu-kvm/hw/pc.c
===================================================================
--- qemu-kvm.orig/hw/pc.c
+++ qemu-kvm/hw/pc.c
@@ -66,6 +66,7 @@
 #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 MAX_IDE_BUS 2
 
@@ -74,6 +75,21 @@ static RTCState *rtc_state;
 static PITState *pit;
 static PCII440FXState *i440fx_state;
 
+#define E820_NR_ENTRIES		16
+
+struct e820_entry {
+    uint64_t address;
+    uint64_t length;
+    uint32_t type;
+};
+
+struct e820_table {
+    uint32_t count;
+    struct e820_entry entry[E820_NR_ENTRIES];
+};
+
+static struct e820_table e820_table;
+
 qemu_irq *ioapic_irq_hack;
 
 typedef struct isa_irq_state {
@@ -444,6 +460,23 @@ static void bochs_bios_write(void *opaqu
     }
 }
 
+int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
+{
+    int index = e820_table.count;
+    struct e820_entry *entry;
+
+    if (index >= E820_NR_ENTRIES)
+        return -EBUSY;
+    entry = &e820_table.entry[index];
+
+    entry->address = address;
+    entry->length = length;
+    entry->type = type;
+
+    e820_table.count++;
+    return e820_table.count;
+}
+
 static void *bochs_bios_init(void)
 {
     void *fw_cfg;
@@ -475,6 +508,8 @@ static void *bochs_bios_init(void)
     if (smbios_table)
         fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
                          smbios_table, smbios_len);
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE, (uint8_t *)&e820_table,
+                     sizeof(struct e820_table));
 
     /* allocate memory for the NUMA channel: one (64bit) word for the number
      * of nodes, one word for each VCPU->node and one word for each node to
Index: qemu-kvm/hw/pc.h
===================================================================
--- qemu-kvm.orig/hw/pc.h
+++ qemu-kvm/hw/pc.h
@@ -169,4 +169,13 @@ void extboot_init(BlockDriverState *bs, 
 
 int cpu_is_bsp(CPUState *env);
 
+/* e820 types */
+#define E820_RAM        1
+#define E820_RESERVED   2
+#define E820_ACPI       3
+#define E820_NVS        4
+#define E820_UNUSABLE   5
+
+int e820_add_entry(uint64_t, uint64_t, uint32_t);
+
 #endif
Index: qemu-kvm/qemu-kvm-x86.c
===================================================================
--- qemu-kvm.orig/qemu-kvm-x86.c
+++ qemu-kvm/qemu-kvm-x86.c
@@ -37,6 +37,13 @@ int kvm_set_tss_addr(kvm_context_t kvm, 
 {
 #ifdef KVM_CAP_SET_TSS_ADDR
 	int r;
+        /*
+         * Tell fw_cfg to notify the BIOS to reserve the range.
+         */
+        if (e820_add_entry(addr, 0x4000, E820_RESERVED) < 0) {
+            perror("e820_add_entry() table is full");
+            exit(1);
+        }
 
 	r = kvm_ioctl(kvm_state, KVM_CHECK_EXTENSION, KVM_CAP_SET_TSS_ADDR);
 	if (r > 0) {
Index: qemu-kvm/target-i386/kvm.c
===================================================================
--- qemu-kvm.orig/target-i386/kvm.c
+++ qemu-kvm/target-i386/kvm.c
@@ -298,6 +298,13 @@ int kvm_arch_init(KVMState *s, int smp_c
      * as unavaible memory.  FIXME, need to ensure the e820 map deals with
      * this?
      */
+    /*
+     * Tell fw_cfg to notify the BIOS to reserve the range.
+     */
+    if (e820_add_entry(0xfffbc000, 0x4000, E820_RESERVED) < 0) {
+        perror("e820_add_entry() table is full");
+        exit(1);
+    }
     return kvm_vm_ioctl(s, KVM_SET_TSS_ADDR, 0xfffbd000);
 }
                     

[Qemu-devel] [PATCH] QEMU-KVM - provide e820 table via fw_cfg

[Jes Sorensen]

[-- Attachment #1: Type: text/plain, Size: 140 bytes --]

Hi,

This is the QEMU-KVM part of the patch. If we can agree on this
approach, I will do a version for upstream QEMU as well.

Cheers,
Jes


[-- Attachment #2: 0011-qemu-kvm-e820-table.patch --]
[-- Type: text/plain, Size: 3920 bytes --]

Use qemu-cfg to provide the BIOS with an optional table of e820 entries.

Notify the BIOS of the location of the TSS+EPT range to by reserving
it via the e820 table.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

---
 hw/pc.c           |   35 +++++++++++++++++++++++++++++++++++
 hw/pc.h           |    9 +++++++++
 qemu-kvm-x86.c    |    7 +++++++
 target-i386/kvm.c |    7 +++++++
 4 files changed, 58 insertions(+)

Index: qemu-kvm/hw/pc.c
===================================================================
--- qemu-kvm.orig/hw/pc.c
+++ qemu-kvm/hw/pc.c
@@ -66,6 +66,7 @@
 #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 MAX_IDE_BUS 2
 
@@ -74,6 +75,21 @@ static RTCState *rtc_state;
 static PITState *pit;
 static PCII440FXState *i440fx_state;
 
+#define E820_NR_ENTRIES		16
+
+struct e820_entry {
+    uint64_t address;
+    uint64_t length;
+    uint32_t type;
+};
+
+struct e820_table {
+    uint32_t count;
+    struct e820_entry entry[E820_NR_ENTRIES];
+};
+
+static struct e820_table e820_table;
+
 qemu_irq *ioapic_irq_hack;
 
 typedef struct isa_irq_state {
@@ -444,6 +460,23 @@ static void bochs_bios_write(void *opaqu
     }
 }
 
+int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
+{
+    int index = e820_table.count;
+    struct e820_entry *entry;
+
+    if (index >= E820_NR_ENTRIES)
+        return -EBUSY;
+    entry = &e820_table.entry[index];
+
+    entry->address = address;
+    entry->length = length;
+    entry->type = type;
+
+    e820_table.count++;
+    return e820_table.count;
+}
+
 static void *bochs_bios_init(void)
 {
     void *fw_cfg;
@@ -475,6 +508,8 @@ static void *bochs_bios_init(void)
     if (smbios_table)
         fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
                          smbios_table, smbios_len);
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE, (uint8_t *)&e820_table,
+                     sizeof(struct e820_table));
 
     /* allocate memory for the NUMA channel: one (64bit) word for the number
      * of nodes, one word for each VCPU->node and one word for each node to
Index: qemu-kvm/hw/pc.h
===================================================================
--- qemu-kvm.orig/hw/pc.h
+++ qemu-kvm/hw/pc.h
@@ -169,4 +169,13 @@ void extboot_init(BlockDriverState *bs, 
 
 int cpu_is_bsp(CPUState *env);
 
+/* e820 types */
+#define E820_RAM        1
+#define E820_RESERVED   2
+#define E820_ACPI       3
+#define E820_NVS        4
+#define E820_UNUSABLE   5
+
+int e820_add_entry(uint64_t, uint64_t, uint32_t);
+
 #endif
Index: qemu-kvm/qemu-kvm-x86.c
===================================================================
--- qemu-kvm.orig/qemu-kvm-x86.c
+++ qemu-kvm/qemu-kvm-x86.c
@@ -37,6 +37,13 @@ int kvm_set_tss_addr(kvm_context_t kvm, 
 {
 #ifdef KVM_CAP_SET_TSS_ADDR
 	int r;
+        /*
+         * Tell fw_cfg to notify the BIOS to reserve the range.
+         */
+        if (e820_add_entry(addr, 0x4000, E820_RESERVED) < 0) {
+            perror("e820_add_entry() table is full");
+            exit(1);
+        }
 
 	r = kvm_ioctl(kvm_state, KVM_CHECK_EXTENSION, KVM_CAP_SET_TSS_ADDR);
 	if (r > 0) {
Index: qemu-kvm/target-i386/kvm.c
===================================================================
--- qemu-kvm.orig/target-i386/kvm.c
+++ qemu-kvm/target-i386/kvm.c
@@ -298,6 +298,13 @@ int kvm_arch_init(KVMState *s, int smp_c
      * as unavaible memory.  FIXME, need to ensure the e820 map deals with
      * this?
      */
+    /*
+     * Tell fw_cfg to notify the BIOS to reserve the range.
+     */
+    if (e820_add_entry(0xfffbc000, 0x4000, E820_RESERVED) < 0) {
+        perror("e820_add_entry() table is full");
+        exit(1);
+    }
     return kvm_vm_ioctl(s, KVM_SET_TSS_ADDR, 0xfffbd000);
 }
                     

Re: [PATCH] QEMU-KVM - provide e820 table via fw_cfg

[Alexander Graf]

On 26.01.2010, at 22:53, Jes Sorensen wrote:

> Hi,
> 
> This is the QEMU-KVM part of the patch. If we can agree on this
> approach, I will do a version for upstream QEMU as well.

It shows as attachment again :(.


Alex

> 
> Cheers,
> Jes
> 
> <0011-qemu-kvm-e820-table.patch>

[Qemu-devel] Re: [PATCH] QEMU-KVM - provide e820 table via fw_cfg

[Alexander Graf]

On 26.01.2010, at 22:53, Jes Sorensen wrote:

> Hi,
> 
> This is the QEMU-KVM part of the patch. If we can agree on this
> approach, I will do a version for upstream QEMU as well.

It shows as attachment again :(.


Alex

> 
> Cheers,
> Jes
> 
> <0011-qemu-kvm-e820-table.patch>

Re: [PATCH] Seabios - read e820 table from qemu_cfg

[Kevin O'Connor]

On Tue, Jan 26, 2010 at 10:52:12PM +0100, Jes Sorensen wrote:
> Read optional table of e820 entries from qemu_cfg
[...]
> --- seabios.orig/src/paravirt.c
> +++ seabios/src/paravirt.c
> @@ -132,6 +132,23 @@ u16 qemu_cfg_smbios_entries(void)
>      return cnt;
>  }
>  
> +u32 qemu_cfg_e820_entries(void)
> +{
> +    u32 cnt;
> +
> +    if (!qemu_cfg_present)
> +        return 0;
> +
> +    qemu_cfg_read_entry(&cnt, QEMU_CFG_E820_TABLE, sizeof(cnt));
> +    return cnt;
> +}
> +
> +void* qemu_cfg_e820_load_next(void *addr)
> +{
> +    qemu_cfg_read(addr, sizeof(struct e820_entry));
> +    return addr;
> +}

I think defining accessor functions for every piece of data passed
through qemu-cfg interface is going to get tiring.  I'd prefer to
extend the existing qemu-cfg "file" interface for new content.

For example, add a helper with something like:

int qemu_cfg_get_file(const char *name, void *dest, int maxsize);

> -    if (kvm_para_available())
> -        // 4 pages before the bios, 3 pages for vmx tss pages, the
> -        // other page for EPT real mode pagetable
> -        add_e820(0xfffbc000, 4*4096, E820_RESERVED);
> +    if (kvm_para_available()) {
> +        u32 count;
> +
> +        count = qemu_cfg_e820_entries();
> +        if (count) {
> +            struct e820_entry entry;
> +            int i;
> +
> +            for (i = 0; i < count; i++) {
> +                qemu_cfg_e820_load_next(&entry);
> +                add_e820(entry.address, entry.length, entry.type);
> +            }

and then this becomes:

struct e820entry map[128];
int len = qemu_cfg_get_file("e820map", &map, sizeof(map));
if (len >= 0)
    for (i=0; i<len / sizeof(map[0]); i++)
        add_e820(map[i].start, map[i].size, map[i].type);

The advantage being that it should be possible to write one set of
helper functions in both qemu and seabios that can then be used to
pass arbitrary content.

As a side note, it should probably do the e820 map check even for qemu
users (ie, not just kvm).

-Kevin

[Qemu-devel] Re: [PATCH] Seabios - read e820 table from qemu_cfg

[Kevin O'Connor]

On Tue, Jan 26, 2010 at 10:52:12PM +0100, Jes Sorensen wrote:
> Read optional table of e820 entries from qemu_cfg
[...]
> --- seabios.orig/src/paravirt.c
> +++ seabios/src/paravirt.c
> @@ -132,6 +132,23 @@ u16 qemu_cfg_smbios_entries(void)
>      return cnt;
>  }
>  
> +u32 qemu_cfg_e820_entries(void)
> +{
> +    u32 cnt;
> +
> +    if (!qemu_cfg_present)
> +        return 0;
> +
> +    qemu_cfg_read_entry(&cnt, QEMU_CFG_E820_TABLE, sizeof(cnt));
> +    return cnt;
> +}
> +
> +void* qemu_cfg_e820_load_next(void *addr)
> +{
> +    qemu_cfg_read(addr, sizeof(struct e820_entry));
> +    return addr;
> +}

I think defining accessor functions for every piece of data passed
through qemu-cfg interface is going to get tiring.  I'd prefer to
extend the existing qemu-cfg "file" interface for new content.

For example, add a helper with something like:

int qemu_cfg_get_file(const char *name, void *dest, int maxsize);

> -    if (kvm_para_available())
> -        // 4 pages before the bios, 3 pages for vmx tss pages, the
> -        // other page for EPT real mode pagetable
> -        add_e820(0xfffbc000, 4*4096, E820_RESERVED);
> +    if (kvm_para_available()) {
> +        u32 count;
> +
> +        count = qemu_cfg_e820_entries();
> +        if (count) {
> +            struct e820_entry entry;
> +            int i;
> +
> +            for (i = 0; i < count; i++) {
> +                qemu_cfg_e820_load_next(&entry);
> +                add_e820(entry.address, entry.length, entry.type);
> +            }

and then this becomes:

struct e820entry map[128];
int len = qemu_cfg_get_file("e820map", &map, sizeof(map));
if (len >= 0)
    for (i=0; i<len / sizeof(map[0]); i++)
        add_e820(map[i].start, map[i].size, map[i].type);

The advantage being that it should be possible to write one set of
helper functions in both qemu and seabios that can then be used to
pass arbitrary content.

As a side note, it should probably do the e820 map check even for qemu
users (ie, not just kvm).

-Kevin

Re: [PATCH] Seabios - read e820 table from qemu_cfg

[Jes Sorensen]

On 01/28/10 05:39, Kevin O'Connor wrote:
> I think defining accessor functions for every piece of data passed
> through qemu-cfg interface is going to get tiring.  I'd prefer to
> extend the existing qemu-cfg "file" interface for new content.
>
> For example, add a helper with something like:
>
> int qemu_cfg_get_file(const char *name, void *dest, int maxsize);

Hi Kevin,

I think switching qemu_cfg to use a file name based interface would be
a nice feature, but I think it should be independent of this patch. I am
CC'ing Gleb on this as he did the original design I believe.

>> -    if (kvm_para_available())
>> -        // 4 pages before the bios, 3 pages for vmx tss pages, the
>> -        // other page for EPT real mode pagetable
>> -        add_e820(0xfffbc000, 4*4096, E820_RESERVED);
>> +    if (kvm_para_available()) {
>> +        u32 count;
>> +
>> +        count = qemu_cfg_e820_entries();
>> +        if (count) {
>> +            struct e820_entry entry;
>> +            int i;
>> +
>> +            for (i = 0; i<  count; i++) {
>> +                qemu_cfg_e820_load_next(&entry);
>> +                add_e820(entry.address, entry.length, entry.type);
>> +            }
>
> and then this becomes:
>
> struct e820entry map[128];
> int len = qemu_cfg_get_file("e820map",&map, sizeof(map));
> if (len>= 0)
>      for (i=0; i<len / sizeof(map[0]); i++)
>          add_e820(map[i].start, map[i].size, map[i].type);
>
> The advantage being that it should be possible to write one set of
> helper functions in both qemu and seabios that can then be used to
> pass arbitrary content.

The only issue here is that I designed the Seabios portion to not rely
on the size of the struct, to avoid having to statically reserve it like
in your example. Having the qemu_cfg_get_file() function return a
pointer to a file descriptor and then have a qemu_cfg_read() helper that
takes the descriptor as it's first argument would avoid this problem.

> As a side note, it should probably do the e820 map check even for qemu
> users (ie, not just kvm).

Ah I didn't realize Seabios would try to use the fw_cfg interface if it
wasn't running on top of QEMU. That would be good to do.

Cheers,
Jes

[Qemu-devel] Re: [PATCH] Seabios - read e820 table from qemu_cfg

[Jes Sorensen]

On 01/28/10 05:39, Kevin O'Connor wrote:
> I think defining accessor functions for every piece of data passed
> through qemu-cfg interface is going to get tiring.  I'd prefer to
> extend the existing qemu-cfg "file" interface for new content.
>
> For example, add a helper with something like:
>
> int qemu_cfg_get_file(const char *name, void *dest, int maxsize);

Hi Kevin,

I think switching qemu_cfg to use a file name based interface would be
a nice feature, but I think it should be independent of this patch. I am
CC'ing Gleb on this as he did the original design I believe.

>> -    if (kvm_para_available())
>> -        // 4 pages before the bios, 3 pages for vmx tss pages, the
>> -        // other page for EPT real mode pagetable
>> -        add_e820(0xfffbc000, 4*4096, E820_RESERVED);
>> +    if (kvm_para_available()) {
>> +        u32 count;
>> +
>> +        count = qemu_cfg_e820_entries();
>> +        if (count) {
>> +            struct e820_entry entry;
>> +            int i;
>> +
>> +            for (i = 0; i<  count; i++) {
>> +                qemu_cfg_e820_load_next(&entry);
>> +                add_e820(entry.address, entry.length, entry.type);
>> +            }
>
> and then this becomes:
>
> struct e820entry map[128];
> int len = qemu_cfg_get_file("e820map",&map, sizeof(map));
> if (len>= 0)
>      for (i=0; i<len / sizeof(map[0]); i++)
>          add_e820(map[i].start, map[i].size, map[i].type);
>
> The advantage being that it should be possible to write one set of
> helper functions in both qemu and seabios that can then be used to
> pass arbitrary content.

The only issue here is that I designed the Seabios portion to not rely
on the size of the struct, to avoid having to statically reserve it like
in your example. Having the qemu_cfg_get_file() function return a
pointer to a file descriptor and then have a qemu_cfg_read() helper that
takes the descriptor as it's first argument would avoid this problem.

> As a side note, it should probably do the e820 map check even for qemu
> users (ie, not just kvm).

Ah I didn't realize Seabios would try to use the fw_cfg interface if it
wasn't running on top of QEMU. That would be good to do.

Cheers,
Jes

Re: [PATCH] Seabios - read e820 table from qemu_cfg

[Gleb Natapov]

On Fri, Jan 29, 2010 at 10:03:55AM +0100, Jes Sorensen wrote:
> On 01/28/10 05:39, Kevin O'Connor wrote:
> >I think defining accessor functions for every piece of data passed
> >through qemu-cfg interface is going to get tiring.  I'd prefer to
> >extend the existing qemu-cfg "file" interface for new content.
> >
> >For example, add a helper with something like:
> >
> >int qemu_cfg_get_file(const char *name, void *dest, int maxsize);
> 
> Hi Kevin,
> 
> I think switching qemu_cfg to use a file name based interface would be
> a nice feature, but I think it should be independent of this patch. I am
> CC'ing Gleb on this as he did the original design I believe.
> 
There is already file like interface on top of fw_cfg. Look for
qemu_cfg_read_file(). I am not sure this is a good idea to start using
it for something that is not actually a file. I have no problem with
adding accessors for each new data time. As you noted below this way we
don't need to load the whole e820 map into the memory, but can do entry
by entry.

--
			Gleb.

[Qemu-devel] Re: [PATCH] Seabios - read e820 table from qemu_cfg

[Gleb Natapov]

On Fri, Jan 29, 2010 at 10:03:55AM +0100, Jes Sorensen wrote:
> On 01/28/10 05:39, Kevin O'Connor wrote:
> >I think defining accessor functions for every piece of data passed
> >through qemu-cfg interface is going to get tiring.  I'd prefer to
> >extend the existing qemu-cfg "file" interface for new content.
> >
> >For example, add a helper with something like:
> >
> >int qemu_cfg_get_file(const char *name, void *dest, int maxsize);
> 
> Hi Kevin,
> 
> I think switching qemu_cfg to use a file name based interface would be
> a nice feature, but I think it should be independent of this patch. I am
> CC'ing Gleb on this as he did the original design I believe.
> 
There is already file like interface on top of fw_cfg. Look for
qemu_cfg_read_file(). I am not sure this is a good idea to start using
it for something that is not actually a file. I have no problem with
adding accessors for each new data time. As you noted below this way we
don't need to load the whole e820 map into the memory, but can do entry
by entry.

--
			Gleb.

Re: [PATCH] Seabios - read e820 table from qemu_cfg

[Kevin O'Connor]

On Fri, Jan 29, 2010 at 10:03:55AM +0100, Jes Sorensen wrote:
> On 01/28/10 05:39, Kevin O'Connor wrote:
> >The advantage being that it should be possible to write one set of
> >helper functions in both qemu and seabios that can then be used to
> >pass arbitrary content.
> 
> The only issue here is that I designed the Seabios portion to not rely
> on the size of the struct, to avoid having to statically reserve it like
> in your example. Having the qemu_cfg_get_file() function return a
> pointer to a file descriptor and then have a qemu_cfg_read() helper that
> takes the descriptor as it's first argument would avoid this problem.

SeaBIOS already has a maximum size for the e820 map (32) - see
CONFIG_MAX_E820.

> >As a side note, it should probably do the e820 map check even for qemu
> >users (ie, not just kvm).
> 
> Ah I didn't realize Seabios would try to use the fw_cfg interface if it
> wasn't running on top of QEMU. That would be good to do.

Your patch only used it for kvm.  SeaBIOS will use fw_cfg on both qemu
and kvm.

-Kevin

[Qemu-devel] Re: [PATCH] Seabios - read e820 table from qemu_cfg

[Kevin O'Connor]

On Fri, Jan 29, 2010 at 10:03:55AM +0100, Jes Sorensen wrote:
> On 01/28/10 05:39, Kevin O'Connor wrote:
> >The advantage being that it should be possible to write one set of
> >helper functions in both qemu and seabios that can then be used to
> >pass arbitrary content.
> 
> The only issue here is that I designed the Seabios portion to not rely
> on the size of the struct, to avoid having to statically reserve it like
> in your example. Having the qemu_cfg_get_file() function return a
> pointer to a file descriptor and then have a qemu_cfg_read() helper that
> takes the descriptor as it's first argument would avoid this problem.

SeaBIOS already has a maximum size for the e820 map (32) - see
CONFIG_MAX_E820.

> >As a side note, it should probably do the e820 map check even for qemu
> >users (ie, not just kvm).
> 
> Ah I didn't realize Seabios would try to use the fw_cfg interface if it
> wasn't running on top of QEMU. That would be good to do.

Your patch only used it for kvm.  SeaBIOS will use fw_cfg on both qemu
and kvm.

-Kevin

Re: [PATCH] Seabios - read e820 table from qemu_cfg

[Jes Sorensen]

[-- Attachment #1: Type: text/plain, Size: 517 bytes --]

On 01/28/10 05:39, Kevin O'Connor wrote:
> As a side note, it should probably do the e820 map check even for qemu
> users (ie, not just kvm).

Hi Kevin,

Here is an updated version of the patch which does the e820 read
unconditionally of  the return from kvm_para_available() so it should
work for coreboot too.

I haven't touched the file descriptor issue as I find it's a different
discussion.

Let me know what you think.

Cheers,
Jes

PS: I tried to subscribe to the seabios list but I never got an ack for
it :(

[-- Attachment #2: 0005-seabios-e820-table-v2.patch --]
[-- Type: text/plain, Size: 3087 bytes --]

Read optional table of e820 entries from qemu_cfg

Read optional table of e820 entries through qemu_cfg, allowing QEMU to
provide the location of KVM's switch area etc. rather than rely on
hard coded values.

For now, fall back to the old hard coded values for the TSS and EPT
switch page for compatibility reasons. Compatibility code could
possibly be removed in the future.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

---
 src/paravirt.c |   17 +++++++++++++++++
 src/paravirt.h |    9 +++++++++
 src/post.c     |   13 ++++++++++++-
 3 files changed, 38 insertions(+), 1 deletion(-)

Index: seabios/src/paravirt.c
===================================================================
--- seabios.orig/src/paravirt.c
+++ seabios/src/paravirt.c
@@ -132,6 +132,23 @@ u16 qemu_cfg_smbios_entries(void)
     return cnt;
 }
 
+u32 qemu_cfg_e820_entries(void)
+{
+    u32 cnt;
+
+    if (!qemu_cfg_present)
+        return 0;
+
+    qemu_cfg_read_entry(&cnt, QEMU_CFG_E820_TABLE, sizeof(cnt));
+    return cnt;
+}
+
+void* qemu_cfg_e820_load_next(void *addr)
+{
+    qemu_cfg_read(addr, sizeof(struct e820_entry));
+    return addr;
+}
+
 struct smbios_header {
     u16 length;
     u8 type;
Index: seabios/src/paravirt.h
===================================================================
--- seabios.orig/src/paravirt.h
+++ seabios/src/paravirt.h
@@ -36,6 +36,7 @@ static inline int kvm_para_available(voi
 #define QEMU_CFG_ACPI_TABLES		(QEMU_CFG_ARCH_LOCAL + 0)
 #define QEMU_CFG_SMBIOS_ENTRIES		(QEMU_CFG_ARCH_LOCAL + 1)
 #define QEMU_CFG_IRQ0_OVERRIDE		(QEMU_CFG_ARCH_LOCAL + 2)
+#define QEMU_CFG_E820_TABLE		(QEMU_CFG_ARCH_LOCAL + 3)
 
 extern int qemu_cfg_present;
 
@@ -61,8 +62,16 @@ typedef struct QemuCfgFile {
     char name[56];
 } QemuCfgFile;
 
+struct e820_entry {
+    u64 address;
+    u64 length;
+    u32 type;
+};
+
 u16 qemu_cfg_first_file(QemuCfgFile *entry);
 u16 qemu_cfg_next_file(QemuCfgFile *entry);
 u32 qemu_cfg_read_file(QemuCfgFile *entry, void *dst, u32 maxlen);
+u32 qemu_cfg_e820_entries(void);
+void* qemu_cfg_e820_load_next(void *addr);
 
 #endif
Index: seabios/src/post.c
===================================================================
--- seabios.orig/src/post.c
+++ seabios/src/post.c
@@ -135,10 +135,21 @@ ram_probe(void)
              , E820_RESERVED);
     add_e820(BUILD_BIOS_ADDR, BUILD_BIOS_SIZE, E820_RESERVED);
 
-    if (kvm_para_available())
+    u32 count = qemu_cfg_e820_entries();
+    if (count) {
+        struct e820_entry entry;
+        int i;
+
+        for (i = 0; i < count; i++) {
+            qemu_cfg_e820_load_next(&entry);
+            add_e820(entry.address, entry.length, entry.type);
+        }
+    } else if (kvm_para_available()) {
+        // Backwards compatibility - provide hard coded range.
         // 4 pages before the bios, 3 pages for vmx tss pages, the
         // other page for EPT real mode pagetable
         add_e820(0xfffbc000, 4*4096, E820_RESERVED);
+    }
 
     dprintf(1, "Ram Size=0x%08x (0x%08x%08x high)\n"
             , RamSize, (u32)(RamSizeOver4G >> 32), (u32)RamSizeOver4G);

[Qemu-devel] Re: [PATCH] Seabios - read e820 table from qemu_cfg

[Jes Sorensen]

[-- Attachment #1: Type: text/plain, Size: 517 bytes --]

On 01/28/10 05:39, Kevin O'Connor wrote:
> As a side note, it should probably do the e820 map check even for qemu
> users (ie, not just kvm).

Hi Kevin,

Here is an updated version of the patch which does the e820 read
unconditionally of  the return from kvm_para_available() so it should
work for coreboot too.

I haven't touched the file descriptor issue as I find it's a different
discussion.

Let me know what you think.

Cheers,
Jes

PS: I tried to subscribe to the seabios list but I never got an ack for
it :(

[-- Attachment #2: 0005-seabios-e820-table-v2.patch --]
[-- Type: text/plain, Size: 3087 bytes --]

Read optional table of e820 entries from qemu_cfg

Read optional table of e820 entries through qemu_cfg, allowing QEMU to
provide the location of KVM's switch area etc. rather than rely on
hard coded values.

For now, fall back to the old hard coded values for the TSS and EPT
switch page for compatibility reasons. Compatibility code could
possibly be removed in the future.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

---
 src/paravirt.c |   17 +++++++++++++++++
 src/paravirt.h |    9 +++++++++
 src/post.c     |   13 ++++++++++++-
 3 files changed, 38 insertions(+), 1 deletion(-)

Index: seabios/src/paravirt.c
===================================================================
--- seabios.orig/src/paravirt.c
+++ seabios/src/paravirt.c
@@ -132,6 +132,23 @@ u16 qemu_cfg_smbios_entries(void)
     return cnt;
 }
 
+u32 qemu_cfg_e820_entries(void)
+{
+    u32 cnt;
+
+    if (!qemu_cfg_present)
+        return 0;
+
+    qemu_cfg_read_entry(&cnt, QEMU_CFG_E820_TABLE, sizeof(cnt));
+    return cnt;
+}
+
+void* qemu_cfg_e820_load_next(void *addr)
+{
+    qemu_cfg_read(addr, sizeof(struct e820_entry));
+    return addr;
+}
+
 struct smbios_header {
     u16 length;
     u8 type;
Index: seabios/src/paravirt.h
===================================================================
--- seabios.orig/src/paravirt.h
+++ seabios/src/paravirt.h
@@ -36,6 +36,7 @@ static inline int kvm_para_available(voi
 #define QEMU_CFG_ACPI_TABLES		(QEMU_CFG_ARCH_LOCAL + 0)
 #define QEMU_CFG_SMBIOS_ENTRIES		(QEMU_CFG_ARCH_LOCAL + 1)
 #define QEMU_CFG_IRQ0_OVERRIDE		(QEMU_CFG_ARCH_LOCAL + 2)
+#define QEMU_CFG_E820_TABLE		(QEMU_CFG_ARCH_LOCAL + 3)
 
 extern int qemu_cfg_present;
 
@@ -61,8 +62,16 @@ typedef struct QemuCfgFile {
     char name[56];
 } QemuCfgFile;
 
+struct e820_entry {
+    u64 address;
+    u64 length;
+    u32 type;
+};
+
 u16 qemu_cfg_first_file(QemuCfgFile *entry);
 u16 qemu_cfg_next_file(QemuCfgFile *entry);
 u32 qemu_cfg_read_file(QemuCfgFile *entry, void *dst, u32 maxlen);
+u32 qemu_cfg_e820_entries(void);
+void* qemu_cfg_e820_load_next(void *addr);
 
 #endif
Index: seabios/src/post.c
===================================================================
--- seabios.orig/src/post.c
+++ seabios/src/post.c
@@ -135,10 +135,21 @@ ram_probe(void)
              , E820_RESERVED);
     add_e820(BUILD_BIOS_ADDR, BUILD_BIOS_SIZE, E820_RESERVED);
 
-    if (kvm_para_available())
+    u32 count = qemu_cfg_e820_entries();
+    if (count) {
+        struct e820_entry entry;
+        int i;
+
+        for (i = 0; i < count; i++) {
+            qemu_cfg_e820_load_next(&entry);
+            add_e820(entry.address, entry.length, entry.type);
+        }
+    } else if (kvm_para_available()) {
+        // Backwards compatibility - provide hard coded range.
         // 4 pages before the bios, 3 pages for vmx tss pages, the
         // other page for EPT real mode pagetable
         add_e820(0xfffbc000, 4*4096, E820_RESERVED);
+    }
 
     dprintf(1, "Ram Size=0x%08x (0x%08x%08x high)\n"
             , RamSize, (u32)(RamSizeOver4G >> 32), (u32)RamSizeOver4G);

Re: [PATCH] Seabios - read e820 table from qemu_cfg

[Kevin O'Connor]

On Mon, Feb 08, 2010 at 11:31:40AM +0100, Jes Sorensen wrote:
> On 01/28/10 05:39, Kevin O'Connor wrote:
> >As a side note, it should probably do the e820 map check even for qemu
> >users (ie, not just kvm).
> 
> Hi Kevin,
> 
> Here is an updated version of the patch which does the e820 read
> unconditionally of  the return from kvm_para_available() so it should
> work for coreboot too.
> 
> I haven't touched the file descriptor issue as I find it's a different
> discussion.

I'd prefer to use the "file" method - but I wont hold up your patch
for it.  If the host part of your patch is committed to qemu, I'll
commit the SeaBIOS part.

[...]
> +struct e820_entry {
> +    u64 address;
> +    u64 length;
> +    u32 type;
> +};

I find this struct to be easily confused with 'struct e820entry' in
memmap.h.  Both code should use the same struct, or the new struct
should clearly indicate it's for qemu (eg, "qemu_e820_entry").

Thanks,
-Kevin

[Qemu-devel] Re: [PATCH] Seabios - read e820 table from qemu_cfg

[Kevin O'Connor]

On Mon, Feb 08, 2010 at 11:31:40AM +0100, Jes Sorensen wrote:
> On 01/28/10 05:39, Kevin O'Connor wrote:
> >As a side note, it should probably do the e820 map check even for qemu
> >users (ie, not just kvm).
> 
> Hi Kevin,
> 
> Here is an updated version of the patch which does the e820 read
> unconditionally of  the return from kvm_para_available() so it should
> work for coreboot too.
> 
> I haven't touched the file descriptor issue as I find it's a different
> discussion.

I'd prefer to use the "file" method - but I wont hold up your patch
for it.  If the host part of your patch is committed to qemu, I'll
commit the SeaBIOS part.

[...]
> +struct e820_entry {
> +    u64 address;
> +    u64 length;
> +    u32 type;
> +};

I find this struct to be easily confused with 'struct e820entry' in
memmap.h.  Both code should use the same struct, or the new struct
should clearly indicate it's for qemu (eg, "qemu_e820_entry").

Thanks,
-Kevin