[SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Tue, Apr 23, 2013 at 08:54:17PM -0400, Kevin O'Connor wrote:
> On Tue, Apr 23, 2013 at 06:22:12PM +0300, Michael S. Tsirkin wrote:
> > So here's the version with shift, that should do it
> > correctly. Add mere 8 lines of code.
> > I'll look into alignment and fseg now.
> [...]
> > --- /dev/null
> > +++ b/src/linker.h
> > @@ -0,0 +1,29 @@
> > +#include "types.h" // u8
> > +#include "util.h" // romfile_s
> > +
> > +/* ROM file linker interface. Linker uses little endian format */
> > +struct linker_entry_s {
> > +    u8 size; /* size in bytes including the header */
> > +    u8 bytes; /* How many bytes to change. Can be 1,2,4 or 8. */
> > +    u8 shift; /* Shift source address right by this many bits. 0-63. */
> > +    u8 type;
> > +    u8 format;
> > +    u8 reserved1;
> > +    u16 reserved2;
> > +    u64 dst_offset; /* Offset in destination. Little endian format. */
> > +    /* Followed by source and destination, optionally padded by
> > +     * 0, up to the total of entry_len - 4 bytes.
> > +     * Strings are 0-terminated. */
> > +    char src_dst[];
> > +} PACKED;
> 
> As in my previous email, I think the main emphasis should be on
> getting the content into QEMU, so I don't want to nitpick on the
> details of the seabios side.  However, if I understand your proposal,
> an RSDT with 10 table entries would require 50 "linker" entries (10 to
> add the pointers and 40 to update the checksum) - that seems
> excessive.

Well that's not a lot of memory, is it?
And it's freed immediately after we link.

> If the goal is to do a checksum - might as well just
> define a checksum action.

I agree it makes the interface more more straight-forward
to use.

> Also, the variable length filenames aren't
> needed - the QemuCfgFile interface already limits the filename to a
> max of 56 bytes.

True. Two reasons I did not do it this way:
- this makes the code generic. Maybe others will want to reuse this?
- a bit less memory - we dont have long file names in practice.
Valid concerns?

> 
> If you really feel QEMU should direct the pointer and checksum
> updates, you might want to consider something like:
> 
> struct tabledeploy_s {
>     u32 command;
>     union {
>         // COMMAND_ALLOC - allocate a table from the given "file"
>         struct {
>             char alloc_file[FILESZ];
>             u32 alloc_align;
>             u8 alloc_zone;
>         };
> 
>         // COMMAND_PATCH - patch the table (originating from
>         // "dest_file") to a pointer to the table originating from
>         // "src_file".
>         struct {
>             char patch_dest_file[FILESZ];
>             char patch_src_file[FILESZ];
>             u32 patch_offset;
>             u8 patch_size;
>         };
> 

I think ADD is better since this way we can use not the start of table,
but something in the middle. For example, a single fw_cfg can give us
multiple tables, it should be up to QEMU.

It's equivalent to PATCH if the original data is 0.

>         // COMMAND_CHECKSUM - Update a checksum in a table
>         struct {
>             char cksum_file[FILESZ];
>             u32 cksum_offset;
>             u32 cksum_start;
>             u32 cksum_length;
>         };
> 

Same here. So I would do:

	COMMAND_ADD_POINTER

	COMMAND_ADD_CHECKSUM

>         // PAD
>         char pad[124];
>     };
> } PACKED;
> 
> -Kevin