| Message ID | 1383567431-13540-2-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
Hi, On Mon, Nov 04, 2013 at 01:17:10PM +0100, Gerd Hoffmann wrote: > Unlike the existing FW_CFG_E820_TABLE entry which carries reservations > only the new etc/e820 file also has entries for RAM. Acked, it looks the best the way to go if the objective is to keep backwards compatibility with older seabios protocol. I have to trust you on why we need to stay backwards compatible at all times and why we can't commit an updated bios.bin before a new seabios stable release happened. Otherwise we could have just fixed FW_CFG_E820_TABLE breaking backwards compatibility without introducing a partially overlapping fw_cfg. About the file, I wonder what happens if too many people starts to use files and we'll run out of FW_CFG_FILE_SLOTS at runtime (assert(index < FW_CFG_FILE_SLOTS);). To me seeing the list of all fw_cfg IDs in a header file to define all possible paravirt APIs seabios need to know about, looked not so bad, but then grepping for fw_cfg_add_file is equivalent. The main issue is that if we use files from now, it would be nicer not to limit those to FW_CFG_FILE_SLOTS but to allocate them a bit more dynamically without asserts but this is offtopic (I just happened to read how files are created to review this patch). Probably one patch could be added to s/FW_CFG_E820_TABLE/FW_CFG_E820_TABLE_OLD/, otherwise if somebody read fw_cfg.h, it won't be apparent that the grepping shouldn't stop there to reach the real e820 map. Thanks! Andrea
On Di, 2013-11-05 at 18:48 +0100, Andrea Arcangeli wrote: > Hi, > > On Mon, Nov 04, 2013 at 01:17:10PM +0100, Gerd Hoffmann wrote: > > Unlike the existing FW_CFG_E820_TABLE entry which carries reservations > > only the new etc/e820 file also has entries for RAM. > > Acked, it looks the best the way to go if the objective is to keep > backwards compatibility with older seabios protocol. > > I have to trust you on why we need to stay backwards compatible at all > times and why we can't commit an updated bios.bin before a new seabios > stable release happened. It's seabios policy, for good reasons. Basically because it is a PITA in certain cases if there are strict version requirements. Also note that seabios isn't the only possible firmware for qemu. > About the file, I wonder what happens if too many people starts to use > files and we'll run out of FW_CFG_FILE_SLOTS at runtime (assert(index > < FW_CFG_FILE_SLOTS);). Probably we should simply raise the number of slots. We don't have allocated any FW_CFG slots behind the file slot range yet, so there is space. Also there is a count in the file list struct and firmware uses that to figure how many files are there and dynamically allocate memory for the list, so growing the list shouldn't cause problems on the firmware side too. > Probably one patch could be added to > s/FW_CFG_E820_TABLE/FW_CFG_E820_TABLE_OLD/, otherwise if somebody read > fw_cfg.h, it won't be apparent that the grepping shouldn't stop there > to reach the real e820 map. Yep, either that or a comment. Also there are more obsolete fw_cfg entries I think. Isn't the num_cpus entry superseded by numa info? cheers, Gerd
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index dee409d..a653ae4 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -90,7 +90,9 @@ struct e820_table { struct e820_entry entry[E820_NR_ENTRIES]; } QEMU_PACKED __attribute((__aligned__(4))); -static struct e820_table e820_table; +static struct e820_table e820_reserve; +static struct e820_entry *e820_table; +static unsigned e820_entries; struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX}; void gsi_handler(void *opaque, int n, int level) @@ -577,19 +579,32 @@ static void handle_a20_line_change(void *opaque, int irq, int level) int e820_add_entry(uint64_t address, uint64_t length, uint32_t type) { - int index = le32_to_cpu(e820_table.count); + int index = le32_to_cpu(e820_reserve.count); struct e820_entry *entry; - if (index >= E820_NR_ENTRIES) - return -EBUSY; - entry = &e820_table.entry[index++]; + if (type != E820_RAM) { + /* old FW_CFG_E820_TABLE entry -- reservations only */ + if (index >= E820_NR_ENTRIES) { + return -EBUSY; + } + entry = &e820_reserve.entry[index++]; + + entry->address = cpu_to_le64(address); + entry->length = cpu_to_le64(length); + entry->type = cpu_to_le32(type); + + e820_reserve.count = cpu_to_le32(index); + } - entry->address = cpu_to_le64(address); - entry->length = cpu_to_le64(length); - entry->type = cpu_to_le32(type); + /* new "etc/e820" file -- include ram too */ + e820_table = g_realloc(e820_table, + sizeof(struct e820_entry) * (e820_entries+1)); + e820_table[e820_entries].address = cpu_to_le64(address); + e820_table[e820_entries].length = cpu_to_le64(length); + e820_table[e820_entries].type = cpu_to_le32(type); + e820_entries++; - e820_table.count = cpu_to_le32(index); - return index; + return e820_entries; } /* Calculates the limit to CPU APIC ID values @@ -640,7 +655,9 @@ static FWCfgState *bochs_bios_init(void) fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES, smbios_table, smbios_len); fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE, - &e820_table, sizeof(e820_table)); + &e820_reserve, sizeof(e820_reserve)); + fw_cfg_add_file(fw_cfg, "etc/e820", e820_table, + sizeof(struct e820_entry) * e820_entries); fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_cfg, sizeof(hpet_cfg)); /* allocate memory for the NUMA channel: one (64bit) word for the number
Unlike the existing FW_CFG_E820_TABLE entry which carries reservations only the new etc/e820 file also has entries for RAM. Format is simliar to the FW_CFG_E820_TABLE, it is a simple list of e820_entry structs. Unlike FW_CFG_E820_TABLE it has no count though as the number of entries can be figured from the file size. Cc: Andrea Arcangeli <aarcange@redhat.com> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/i386/pc.c | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-)