[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 1/8] linker-loader: Add new 'write pointer' c
From: |
Ben Warren |
Subject: |
Re: [Qemu-devel] [PATCH v7 1/8] linker-loader: Add new 'write pointer' command |
Date: |
Thu, 16 Feb 2017 09:04:16 -0800 |
> On Feb 16, 2017, at 9:01 AM, Laszlo Ersek <address@hidden> wrote:
>
> On 02/16/17 07:18, address@hidden <mailto: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 | 66
>> ++++++++++++++++++++++++++++++++++--
>> include/hw/acpi/bios-linker-loader.h | 7 ++++
>> 2 files changed, 70 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
>> index d963ebe..d5fb703 100644
>> --- a/hw/acpi/bios-linker-loader.c
>> +++ b/hw/acpi/bios-linker-loader.c
>> @@ -78,6 +78,21 @@ 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
>> + * @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];
>> };
>> @@ -85,9 +100,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 +294,47 @@ 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
>> + * @src_offset: location within source file blob to which
>> + * @address@hidden will point to after
>> + * firmware's executed WRITE_POINTER command
>> + */
>> +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,
>> + uint32_t src_offset)
>> +{
>> + BiosLinkerLoaderEntry entry;
>> + const BiosLinkerFileEntry *source_file =
>> + bios_linker_find_file(linker, src_file);
>> +
>> + assert(source_file);
>> + assert(src_offset <= source_file->blob->len);
>
> (1) Off by one, as pointed out by Igor.
Oh, “off-by-one” errors. One of the three biggest sources of bugs in
programming. The other being recursion. Sorry, couldn’t resist :)
>
>> + 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.dst_offset = cpu_to_le32(dst_patched_offset);
>> + entry.wr_pointer.src_offset = cpu_to_le32(dst_patched_offset);
>
> (2) copy/paste error, you should be assigning "src_offset", as in:
>
Oops.
>> diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
>> index d5fb7033a0be..d1a9f2f33dcc 100644
>> --- a/hw/acpi/bios-linker-loader.c
>> +++ b/hw/acpi/bios-linker-loader.c
>> @@ -331,7 +331,7 @@ void bios_linker_loader_write_pointer(BIOSLinker *linker,
>> sizeof entry.wr_pointer.src_file - 1);
>> entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_WRITE_POINTER);
>> entry.wr_pointer.dst_offset = cpu_to_le32(dst_patched_offset);
>> - entry.wr_pointer.src_offset = cpu_to_le32(dst_patched_offset);
>> + entry.wr_pointer.src_offset = cpu_to_le32(src_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);
>
> With these errors fixed:
>
> Reviewed-by: Laszlo Ersek <address@hidden <mailto:address@hidden>>
>
> I also tested this series (with the assignment under (2) fixed up, of
> course), as documented earlier in
> <https://www.mail-archive.com/address@hidden/msg430075.html
> <https://www.mail-archive.com/address@hidden/msg430075.html>> (msgid
> <address@hidden <mailto:address@hidden>>).
>
> Hence, with (1) and (2) fixed, you can also add
>
> Tested-by: Laszlo Ersek <address@hidden <mailto:address@hidden>>
>
> Thanks
> Laszlo
>
Thanks a lot!
—Ben
>> + 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..efe17b0 100644
>> --- a/include/hw/acpi/bios-linker-loader.h
>> +++ b/include/hw/acpi/bios-linker-loader.h
>> @@ -26,5 +26,12 @@ 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,
>> + uint32_t src_offset);
>> +
>> void bios_linker_loader_cleanup(BIOSLinker *linker);
>> #endif
smime.p7s
Description: S/MIME cryptographic signature
[Qemu-devel] [PATCH v7 5/8] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands, ben, 2017/02/16