On Wed, Feb 15, 2017 at 07:24:36PM +0100, Igor Mammedov wrote:
> > As long as all users pass in 0 though there's a real possibility guests
> > will implement this incorrectly.
> We are here to ensure that at least Seabios (I'll review it)
> and OVMF (Laszlo would take care of it I suppose) do it right,
> and if there are other firmwares, they should do it correctly
> as described fix their own bugs later wrt randomly written
> implementation.
As long as it's untested, it can always do the wrong thing
even if it looks right, or it can bitrot.
> > I guess we can put in the offset just
> > behind the zero-filled padding we have there.
> I've assumed padding was there to make commands fixed size and give
> a room for future extensions
you mean
char pad[124];
?
Right. So you can easily add fields, but if you do firmware
will just assume it does not know how to handle these
commands and skip them.
> so hunk changing BiosLinkerLoaderEntry
> would look like:
>
> diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
> index d963ebe..6983713 100644
> --- a/hw/acpi/bios-linker-loader.c
> +++ b/hw/acpi/bios-linker-loader.c
> @@ -49,6 +49,7 @@ struct BiosLinkerLoaderEntry {
> char file[BIOS_LINKER_LOADER_FILESZ];
> uint32_t align;
> uint8_t zone;
> + uint32_t padding;
> } alloc;
>
> /*
> @@ -62,6 +63,7 @@ struct BiosLinkerLoaderEntry {
> char src_file[BIOS_LINKER_LOADER_FILESZ];
> uint32_t offset;
> uint8_t size;
> + uint32_t padding;
> } pointer;
>
> /*
> @@ -76,10 +78,25 @@ struct BiosLinkerLoaderEntry {
> uint32_t offset;
> uint32_t start;
> uint32_t length;
> + uint32_t padding;
> } cksum;
why is this necessary?
> + /*
> + * COMMAND_WRITE_POINTER - write the fw_cfg file (originating from
> + * @dest_file) at @wr_pointer.offset, by writing a pointer to
> @src_offset
> + * within the table originating from @src_file. 1,2,4 or 8 byte
> unsigned
> + * addition is used depending on @wr_pointer.size.
> + */
> + struct {
> + char dest_file[BIOS_LINKER_LOADER_FILESZ];
> + char src_file[BIOS_LINKER_LOADER_FILESZ];
> + uint32_t dst_offset;
> + uint32_t src_offset;
> + uint8_t size;
> + } wr_pointer;
> +
> /* padding */
> - char pad[124];
> + char pad[120];
and this shrinks entry size by 4 bytes. Why?
> };
> } QEMU_PACKED;
> typedef struct BiosLinkerLoaderEntry BiosLinkerLoaderEntry;
>
>
> > I'm mostly concerned we are adding new features to something
> > that has been through 25 revisions already.
> It's ABI so it's worth extra effort,
There's always yet another possible enhancement you can add.
I see it as a bit of a feature creep frankly.
> it looks like only one more revision is left and there is
> about a week left to post and merge it.
If we can do it quickly, fine, but I think we should merge ASAP.
--
MST
