Re: [Qemu-devel] [PATCH v5 02/10] linker-loader: Add new 'write pointer' command

[Laszlo Ersek]

On 02/07/17 13:11, Igor Mammedov wrote:
> On Mon, 6 Feb 2017 19:31:24 +0200
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
>> On Mon, Feb 06, 2017 at 09:16:25AM -0800, Ben Warren wrote:
>>>>> @@ -257,8 +263,11 @@ void bios_linker_loader_add_pointer(BIOSLinker 
>>>>> *linker,
>>>>>     const BiosLinkerFileEntry *source_file =
>>>>>         bios_linker_find_file(linker, src_file);
>>>>>
>>>>> -    assert(dst_patched_offset < dst_file->blob->len);
>>>>> -    assert(dst_patched_offset + dst_patched_size <= dst_file->blob->len);
>>>>> +    /* dst_file need not exist if writing back */  
>>>>
>>>> Why not?  
>>> Because WRITE_POINTER can be called without having first called
>>> ALLOCATE.  In the Vm Generation ID example, there’s no reason for BIOS
>>> to allocate memory for the address fw_cfg, since it’s a construct that
>>> only matters to QEMU.  This is something that was requested by Laszlo.  
>>
>> Well all other commands require you to first allocate.
>> How does bios know e.g. which zone to put it in then?
> There is no need to know which zone to put it in as file
> is writeback only, i.e. there is no need to allocate
> RAM for file (shadow it) which won't be read by guest side ever
> if I got idea right.
> Issue in the patch is combining new command with existing API
> bios_linker_loader_add_pointer(),
> as you suggested new API function + good description what it does
> would be better than reusing.

Agreed on all points (and sorry about the delayed response):

- The writeable fw_cfg file that receives the address from the guest
firmware need not and should not be downloaded by the guest firmware,
into guest memory. Such a blob, in guest memory, is entirely useless to
the guest.

- I also strongly prefer a separate command structure, and function
name, for the new command. (The structure can be shared by a typedef or
another sub-structure, but it should have its own separate struct tag /
type name.)

Thanks!
Laszlo

> 
> 
>>
>> I don't like the asymmetry ... Laszlo? 
> 
>