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

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 15 Feb 2017 22:18:11 -0800
ben@skyportsystems.com wrote:

> From: Ben Warren <ben@skyportsystems.com>
> 
> 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 <ben@skyportsystems.com>
> ---
>  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];
Shouldn't padding be reduced by 4 bytes to keep 
sizeof(BiosLinkerLoaderEntry) the same as before patch,
so that old bios would be able to skip this unknown command
and read the next at the right offset?

>      };
> @@ -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
> + *              @dest_file+@dst_patched_offset 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);
off by one, should be '<'

> +    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);
> +    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

Taking in account comments above are corner cases.
 1: old bios running on new QEMU with vmgenid device and OLD/not supported bios
 2: assert check

It's ok fixes for above issues being fixed in follow up patch
or as fixup while patch is staged in pci tree

Reviewed-by: Igor Mammedov <imammedo@redhat.com>