Re: [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