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
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios
