On Wed, 15 Feb 2017 21:34:45 +0200
"Michael S. Tsirkin" <m...@redhat.com> wrote:
> 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?
so total size won't change, due to +4 to account for src_offest in wr_pointer
>
> > };
> > } 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.
>
