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

[Laszlo Ersek]

On 02/16/17 09:25, Igor Mammedov wrote:
> 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

To be clear, I still disagree with the introduction of "src_offset"
(please let's discuss that in the other sub-thread). Still I can comment
on this question in isolation:

The individual command structures, and the "pad" field (which is 124
bytes), are all members of a *union*. They all start at offset zero in
the union, and the union's size is determined by the largest member,
plus any additional padding at the end that the compiler wishes to
append. (In this case, becasue we use QEMU_PACKED, the compiler doesn't
do that.)

Therefore, as long as you don't exceed the size of

  char pad[124];

you can extend any of the command structures freely -- "pad" will remain
the largest union member, and thereby dictate the size of the union.

In short, "pad" does not *follow* any of the command structures; they
are all laid over each other, from offset 0 in the union.

(If you are curious about the magic constant 124, that comes from 128
minus the size of the "command" field in the outermost
BiosLinkerLoaderEntry structure. The guiding principle here is that the
outermost BiosLinkerLoaderEntry structure be 128 bytes in size.)

--*--

Again: I disagree with the introduction of "src_offset", and I'd like to
hear your opinion about my counter-argument in the other subthread.

Thanks
Laszlo

> 
>>
>>>      };
>>>  } 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.
>>
>