Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command

[Igor Mammedov]

On Wed, 15 Feb 2017 21:34:45 +0200
"Michael S. Tsirkin" <address@hidden> 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.
>