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

[Andrea Arcangeli]

Hi,

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

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

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

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

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

Thanks!
Andrea