Seabios - read e820 reserve from qemu_cfg

PATCH In review Jes Sorensen 5 participants 24 messages Jan 25, 2010 – Feb 14, 2010 KVM

lore

Share & tools

lore

Jes Sorensen

author

2010-01-25 16:46 UTC #0 lore raw

PATCH 1/1 Seabios - read e820 reserve from qemu_cfg

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

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 <[email protected]>

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

src/paravirt.c +9 -0

Index: 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;

src/paravirt.h +7 -0

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

src/post.c +10 -4

Index: 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);

Jes Sorensen

author

2010-01-25 16:49 UTC #1 lore raw

PATCH 1/1 QEMU-KVM - provide e820 reserve through qemu_cfg

Hi,

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

Cheers,
Jes

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 <[email protected]>

---
 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(+)

hw/fw_cfg.h +5 -0

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

hw/pc.c +4 -0

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

kvm.h +2 -0

Index: 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;

qemu-kvm-x86.c +6 -0

Index: 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) {

target-i386/kvm.c +7 -0

Index: 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);
 }

Jes Sorensen

author

2010-01-25 16:52 UTC #2 lore raw

PATCH 1/1 QEMU - provide e820 reserve through qemu_cfg

Hi,

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

Cheers,
Jes

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 <[email protected]>

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

hw/pc.c +5 -0

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

kvm.h +7 -0

Index: 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;

target-i386/kvm.c +5 -0

Index: 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);
 }

Alexander Graf

2010-01-25 16:58 UTC #3 lore raw

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

Quoted 4 lines · Jes Sorensen

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:

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

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

Jes Sorensen

author

2010-01-25 17:13 UTC #4 lore raw

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

Quoted 1 line · Alexander Graf

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

:-)

Quoted 5 lines

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:

Quoted 7 lines

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

Quoted 1 line

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

Alexander Graf

2010-01-25 17:28 UTC #5 lore raw

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

Am 25.01.2010 um 18:13 schrieb Jes Sorensen <[email protected]>:

Quoted 12 lines · Alexander Graf

Quoted 1 line
Howdy. Congratulations to the new mail address - looks neat ;-).
:-)
Quoted 7 lines
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>

Quoted 8 lines

Quoted 5 lines
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.

Quoted 5 lines

Quoted 2 lines
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

Quoted 1 line

Jes Sorensen

author

2010-01-25 17:46 UTC #6 lore raw

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

Quoted 10 lines · Alexander Graf

Quoted 7 lines
Quoted 4 lines
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

Alexander Graf

2010-01-25 20:04 UTC #7 lore raw

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

Quoted 9 lines · Jes Sorensen

Quoted 8 lines · Alexander Graf
Quoted 7 lines
Quoted 4 lines
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.

Quoted 7 lines

Quoted 2 lines
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

Anthony Liguori

2010-01-25 20:14 UTC #8 lore raw

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

Quoted 16 lines · Alexander Graf

Quoted 13 lines · Jes Sorensen
Quoted 11 lines · Alexander Graf
Quoted 8 lines
Quoted 5 lines
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.

Quoted 7 lines

Quoted 5 lines
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

Jes Sorensen

author

2010-01-25 21:05 UTC #9 lore raw

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

Quoted 4 lines · Anthony Liguori

Quoted 2 lines · Alexander Graf
Yes, sounds good. Should be fairly extensible then. What about memory holes? Do we need to take care of them?

Quoted 8 lines

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

Alexander Graf

2010-01-25 21:08 UTC #10 lore raw

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

Quoted 14 lines · Jes Sorensen

Quoted 4 lines · Anthony Liguori
Quoted 2 lines · Alexander Graf
Yes, sounds good. Should be fairly extensible then. What about memory holes? Do we need to take care of them?
Quoted 8 lines
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.

Quoted 8 lines

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

Jes Sorensen

author

2010-01-25 21:24 UTC #11 lore raw

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

Quoted 8 lines · Alexander Graf

Quoted 3 lines · Jes Sorensen
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

Gleb Natapov

2010-01-26 06:46 UTC #12 lore raw

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

In reply to Jes Sorensen

Quoted 12 lines · Jes Sorensen

Quoted 1 line · Alexander Graf
Howdy. Congratulations to the new mail address - looks neat ;-).
:-)
Quoted 7 lines
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>

Quoted 9 lines

Quoted 5 lines
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.

Quoted 5 lines

Quoted 1 line
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?

Quoted 7 lines

Cheers,
Jes
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Gleb.

Jes Sorensen

author

2010-01-26 08:37 UTC #13 lore raw

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

Quoted 6 lines · Gleb Natapov

Quoted 3 lines · Jes Sorensen
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

Kevin O'Connor

2010-01-26 00:24 UTC #14 lore raw

In reply to Jes Sorensen

Quoted 8 lines · Jes Sorensen

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.

Quoted 7 lines

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

Jes Sorensen

author

2010-01-26 21:52 UTC #15 lore raw

PATCH 1/1 Seabios - read e820 table from qemu_cfg

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

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 <[email protected]>

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

src/paravirt.c +17 -0

Index: 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;

src/paravirt.h +9 -0

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

src/post.c +19 -4

Index: 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);

Jes Sorensen

author

2010-01-26 21:54 UTC #16 lore raw

PATCH 1/1 QEMU-KVM - provide e820 table via fw_cfg

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

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 <[email protected]>

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

hw/pc.c +35 -0

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

hw/pc.h +9 -0

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

qemu-kvm-x86.c +7 -0

Index: 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) {

target-i386/kvm.c +7 -0

Index: 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);
 }

Alexander Graf

2010-01-26 21:55 UTC #17 lore raw

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

Quoted 4 lines · Jes Sorensen

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

Quoted 5 lines

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

Kevin O'Connor

2010-01-28 04:39 UTC #18 lore raw

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

In reply to Jes Sorensen

Quoted 1 line · Jes Sorensen

Read optional table of e820 entries from qemu_cfg

[...]

Quoted 12 lines

src/paravirt.c +6 -0

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

src/paravirt.c u16 qemu_cfg_smbios_entries(void) · line 150

Quoted 10 lines

src/paravirt.c +10 -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);

Quoted 6 lines

src/paravirt.c +2 -4

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

src/paravirt.c u16 qemu_cfg_smbios_entries(void)

Quoted 10 lines

src/paravirt.c +10 -0

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

Jes Sorensen

author

2010-01-29 09:04 UTC #19 lore raw

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

Quoted 7 lines · Kevin O'Connor

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.

Quoted 22 lines

Quoted 6 lines

src/post.c +2 -4

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

src/post.c ram_probe(void) · line 149

Quoted 10 lines

src/post.c +10 -0

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

Quoted 6 lines

     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.

Quoted 2 lines

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

Gleb Natapov

2010-01-29 16:08 UTC #20 lore raw

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

Quoted 7 lines · Jes Sorensen

Quoted 6 lines · Kevin O'Connor
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:

Quoted 8 lines

Quoted 1 line
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.

Kevin O'Connor

2010-01-30 03:35 UTC #21 lore raw

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

In reply to Jes Sorensen

Quoted 10 lines · Jes Sorensen

Quoted 3 lines · Kevin O'Connor
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.

Quoted 5 lines

Quoted 2 lines
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

Jes Sorensen

author

2010-02-08 10:31 UTC #22 lore raw

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

In reply to Kevin O'Connor

Quoted 2 lines · Kevin O'Connor

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

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 <[email protected]>

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

src/paravirt.c +17 -0

Index: 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;

src/paravirt.h +9 -0

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

src/post.c +12 -1

Index: 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);

Kevin O'Connor

2010-02-14 03:17 UTC #23 lore raw

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

Quoted 4 lines · Jes Sorensen

Quoted 2 lines · Kevin O'Connor
As a side note, it should probably do the e820 map check even for qemu users (ie, not just kvm).

Quoted 8 lines

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.

[...]

src/paravirt.h typedef struct QemuCfgFile { · line 69

Quoted 5 lines

src/paravirt.h +5 -0

+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