On Thu, Feb 16, 2017 at 10:43:10AM +0100, Igor Mammedov wrote:
> On Wed, 15 Feb 2017 22:18:11 -0800
> b...@skyportsystems.com wrote:
>
> > From: Ben Warren <b...@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 <b...@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?
IMHO no - because it's a union.
> > };
> > @@ -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 <imamm...@redhat.com>
