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

[Michael S. Tsirkin]

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