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

[Laszlo Ersek]

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.

(1) So, the above looks correct, but please replace "adding" with
"storing", and "unsigned addition" with "store".

Side point: the case for ADD_POINTER is different; there we patch
several individual ACPI objects. The fact that I requested explicit
addition within the ADDR method, as opposed to pre-setting VGIA to a
nonzero offset, is an *incidental* limitation (coming from the OVMF ACPI
SDT header probe suppressor), and we'll likely fix that up later, with
ALLOCATE command hints or something like that. However, in
WRITE_POINTER, asking for the exact allocation address of "src_file" is
an *inherent* characteristic.

For reference, this is the command's description from the (not as yet
posted) OVMF series:

// QemuLoaderCmdWritePointer: the bytes at
// [PointerOffset..PointerOffset+PointerSize) in the writeable fw_cfg
// file PointerFile are to receive the absolute address of PointeeFile,
// as allocated and downloaded by the firmware. Store the base address
// of where PointeeFile's contents have been placed (when
// QemuLoaderCmdAllocate has been executed for PointeeFile) to this
// portion of PointerFile.
//
// This command is similar to QemuLoaderCmdAddPointer; the difference is
// that the "pointer to patch" does not exist in guest-physical address
// space, only in "fw_cfg file space". In addition, the "pointer to
// patch" is not initialized by QEMU with a possibly nonzero offset
// value: the base address of the memory allocated for downloading
// PointeeFile shall not increment the pointer, but overwrite it.

In the last SeaBIOS patch series, namely

[SeaBIOS] [PATCH v3 2/2] QEMU fw_cfg: Add command to write back address
                         of file

function romfile_loader_write_pointer() implemented just that plain
store (not an addition), and that was exactly right.

Continuing:

>> +        struct {
>> +            char dest_file[BIOS_LINKER_LOADER_FILESZ];
>> +            char src_file[BIOS_LINKER_LOADER_FILESZ];
>> +            uint32_t offset;
>> +            uint8_t size;
>> +        } wr_pointer;
>> +
>>          /* padding */
>>          char pad[124];
>>      };
>> @@ -85,9 +98,10 @@ struct BiosLinkerLoaderEntry {
>>  typedef struct BiosLinkerLoaderEntry BiosLinkerLoaderEntry;
>>  
>>  enum {
>> -    BIOS_LINKER_LOADER_COMMAND_ALLOCATE     = 0x1,
>> -    BIOS_LINKER_LOADER_COMMAND_ADD_POINTER  = 0x2,
>> -    BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
>> +    BIOS_LINKER_LOADER_COMMAND_ALLOCATE          = 0x1,
>> +    BIOS_LINKER_LOADER_COMMAND_ADD_POINTER       = 0x2,
>> +    BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM      = 0x3,
>> +    BIOS_LINKER_LOADER_COMMAND_WRITE_POINTER     = 0x4,
>>  };
>>  
>>  enum {
>> @@ -278,3 +292,41 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
>>  
>>      g_array_append_vals(linker->cmd_blob, &entry, sizeof entry);
>>  }
>> +
>> +/*
>> + * bios_linker_loader_write_pointer: ask guest to write a pointer to the
>> + * source file into the destination file, and write it back to QEMU via
>> + * fw_cfg DMA.
>> + *
>> + * @linker: linker object instance
>> + * @dest_file: destination file that must be written
>> + * @dst_patched_offset: location within destination file blob to be patched
>> + *                      with the pointer to @src_file, in bytes
>> + * @dst_patched_offset_size: size of the pointer to be patched
>> + *                      at @dst_patched_offset in @dest_file blob, in bytes
>> + * @src_file: source file who's address must be taken
>> + */
>> +void bios_linker_loader_write_pointer(BIOSLinker *linker,
>> +                                    const char *dest_file,
>> +                                    uint32_t dst_patched_offset,
>> +                                    uint8_t dst_patched_size,
>> +                                    const char *src_file)
> API is missing "src_offset" even though it's not used in this series,
> a patch on top to fix it up is ok for me as far as Seabios/OVMF
> counterpart can handle src_offset correctly from starters.

According to the above, it is the right thing not to add "src_offset"
here. The documentation on the command is slightly incorrect (and causes
confusion), but the helper function's signature and comments are okay.

> 
>> +{
>> +    BiosLinkerLoaderEntry entry;
>> +    const BiosLinkerFileEntry *source_file =
>> +        bios_linker_find_file(linker, src_file);
>> +
>> +    assert(source_file);

I wish we kept the following asserts from bios_linker_loader_add_pointer():

    assert(dst_patched_offset < dst_file->blob->len);
    assert(dst_patched_offset + dst_patched_size <= dst_file->blob->len);

Namely, just because the dst_file is never supposed to be downloaded by
the firmware, it still remains a requirement that the "dst file offset
range" that is to be rewritten *do fall* within the dst file.

Nonetheless, this is not critical. (OVMF at least verifies it anyway.)

Summary (from my side anyway): I feel that the documentation of the new
command is very important. Please fix it up as suggested under (1), in
v7. Regarding the asserts, I'll let you decide.

With the documentation fixed up:

Reviewed-by: Laszlo Ersek <address@hidden>

(If you don't wish to post a v7, I'm also completely fine if Michael or
someone else fixes up the docs as proposed in (1), before committing the
patch.)

Thanks!
Laszlo

>> +    memset(&entry, 0, sizeof entry);
>> +    strncpy(entry.wr_pointer.dest_file, dest_file,
>> +            sizeof entry.wr_pointer.dest_file - 1);
>> +    strncpy(entry.wr_pointer.src_file, src_file,
>> +            sizeof entry.wr_pointer.src_file - 1);
>> +    entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_WRITE_POINTER);
>> +    entry.wr_pointer.offset = cpu_to_le32(dst_patched_offset);
>> +    entry.wr_pointer.size = dst_patched_size;
>> +    assert(dst_patched_size == 1 || dst_patched_size == 2 ||
>> +           dst_patched_size == 4 || dst_patched_size == 8);
>> +
>> +    g_array_append_vals(linker->cmd_blob, &entry, sizeof entry);
>> +}
>> diff --git a/include/hw/acpi/bios-linker-loader.h 
>> b/include/hw/acpi/bios-linker-loader.h
>> index fa1e5d1..f9ba5d6 100644
>> --- a/include/hw/acpi/bios-linker-loader.h
>> +++ b/include/hw/acpi/bios-linker-loader.h
>> @@ -26,5 +26,11 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
>>                                      const char *src_file,
>>                                      uint32_t src_offset);
>>  
>> +void bios_linker_loader_write_pointer(BIOSLinker *linker,
>> +                                      const char *dest_file,
>> +                                      uint32_t dst_patched_offset,
>> +                                      uint8_t dst_patched_size,
>> +                                      const char *src_file);
>> +
>>  void bios_linker_loader_cleanup(BIOSLinker *linker);
>>  #endif
>