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

[Laszlo Ersek]

On 02/15/17 17:39, Michael S. Tsirkin wrote:
> On Wed, Feb 15, 2017 at 04:56:02PM +0100, Igor Mammedov wrote:
>> On Wed, 15 Feb 2017 17:30:00 +0200
>> "Michael S. Tsirkin" <address@hidden> wrote:
>>
>>> On Wed, Feb 15, 2017 at 04:22:25PM +0100, Igor Mammedov wrote:
>>>> On Wed, 15 Feb 2017 15:13:20 +0100
>>>> Laszlo Ersek <address@hidden> wrote:
>>>>   
>>>>> Commenting under Igor's reply for simplicity
>>>>>
>>>>> On 02/15/17 11:57, Igor Mammedov wrote:  
>>>>>> On Tue, 14 Feb 2017 22:15:43 -0800
>>>>>> address@hidden wrote:
>>>>>>     
>>>>>>> From: Ben Warren <address@hidden>
>>>>>>>
>>>>>>> This is similar to the existing 'add pointer' functionality, but instead
>>>>>>> of instructing the guest (BIOS or UEFI) to patch memory, it instructs
>>>>>>> the guest to write the pointer back to QEMU via a writeable fw_cfg file.
>>>>>>>
>>>>>>> Signed-off-by: Ben Warren <address@hidden>
>>>>>>> ---
>>>>>>>  hw/acpi/bios-linker-loader.c         | 58 
>>>>>>> ++++++++++++++++++++++++++++++++++--
>>>>>>>  include/hw/acpi/bios-linker-loader.h |  6 ++++
>>>>>>>  2 files changed, 61 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
>>>>>>> index d963ebe..5030cf1 100644
>>>>>>> --- a/hw/acpi/bios-linker-loader.c
>>>>>>> +++ b/hw/acpi/bios-linker-loader.c
>>>>>>> @@ -78,6 +78,19 @@ struct BiosLinkerLoaderEntry {
>>>>>>>              uint32_t length;
>>>>>>>          } cksum;
>>>>>>>  
>>>>>>> +        /*
>>>>>>> +         * COMMAND_WRITE_POINTER - write the fw_cfg file (originating 
>>>>>>> from
>>>>>>> +         * @dest_file) at @wr_pointer.offset, by adding a pointer to 
>>>>>>> the table
>>>>>>> +         * originating from @src_file. 1,2,4 or 8 byte unsigned
>>>>>>> +         * addition is used depending on @wr_pointer.size.
>>>>>>> +         */    
>>>>>
>>>>> The words "adding" and "addition" are causing confusion here.
>>>>>
>>>>> In all of the previous discussion, *addition* was out of scope from
>>>>> WRITE_POINTER. Again, the firmware is specifically not required to
>>>>> *read* any part of the fw_cfg blob identified by "dest_file".
>>>>>
>>>>> WRITE_POINTER instructs the firmware to return the allocation address of
>>>>> the downloaded "src_file" to QEMU. Any necessary runtime subscripting
>>>>> within "src_file" is to be handled by QEMU code dynamically.
>>>>>
>>>>> For example, consider that "src_file" has *several* fields that QEMU
>>>>> wants to massage; in that case, indexing within QEMU code with field
>>>>> offsets is simply unavoidable.  
>>>> what I don't like here is that this indexing would be rather fragile
>>>> and has to be done in different parts of QEMU /device, AML/.
>>>>
>>>> I'd prefer this helper function to have the same @src_offset
>>>> behavior as ADD_POINTER where patched address could point to
>>>> any part of src_file i.e. not just beginning.  
>>>
>>>
>>>
>>>         /*
>>>          * COMMAND_ADD_POINTER - patch the table (originating from
>>>          * @dest_file) at @pointer.offset, by adding a pointer to the table
>>>          * originating from @src_file. 1,2,4 or 8 byte unsigned
>>>          * addition is used depending on @pointer.size.
>>>          */
>>>  
>>> so the way ADD works is
>>>     read at offset
>>>     add table address
>>>     write result at offset
>>>
>>> in other words it is always beginning of table that is added.
>> more exactly it's, read at 
>>   src_offset = *(dst_blob_ptr+dst_offset)
>>   *(dst_blob+dst_offset) = src_blob_ptr + src_offset
>>
>>> Would the following be acceptable?
>>>
>>>
>>>          * COMMAND_WRITE_POINTER - update the fw_cfg file (originating from
>>>          * @dest_file) at @wr_pointer.offset, by writing a pointer to the 
>>> table
>>>          * originating from @src_file. 1,2,4 or 8 byte unsigned value
>>>          * is written depending on @wr_pointer.size.
>> it looses 'adding' part of ADD_POINTER command which handles src_offset,
>> however implementing adding part looks a bit complicated
>> as patched blob (dst) is not in guest memory but in QEMU and
>> on reset *(dst_blob+dst_offset) should be reset to src_offset.
>> Considering dst file could be device specific memory (field/blob/whatever)
>> it could be hard to track/notice proper reset behavior.
>>
>> So now I'm not sure if src_offset is worth adding.
> 
> Right. Let's just do this math in QEMU if we have to.

Deal. :)

Thanks
Laszlo