| Message ID | 4d92b9f92d2f5b702c23bf135222dfb226ec94a7.1487224954.git.ben@skyportsystems.com |
|---|---|
| State | New |
| Headers | show |
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>
On Thu, Feb 16, 2017 at 10:43:10AM +0100, Igor Mammedov wrote: > 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? 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 <imammedo@redhat.com>
On 02/16/2017 03:43 AM, Igor Mammedov wrote: >> +++ 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? No, because you are in the middle of a union rather than a struct (the outer BiosLinkerLoaderEntry struct size is determined by the largest member of the union, which is 'char pad[124]'; the new wr_pointer addition to the union does not change the size of the union).
On 02/16/17 07:18, 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]; > }; > @@ -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); (1) Off by one, as pointed out by Igor. > + 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: > 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 <lersek@redhat.com> I also tested this series (with the assignment under (2) fixed up, of course), as documented earlier in <https://www.mail-archive.com/qemu-devel@nongnu.org/msg430075.html> (msgid <678c203f-3768-7e65-6e48-6729473b6ffa@redhat.com>). Hence, with (1) and (2) fixed, you can also add Tested-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo > + 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 >
> On Feb 16, 2017, at 9:01 AM, Laszlo Ersek <lersek@redhat.com> wrote: > > On 02/16/17 07:18, ben@skyportsystems.com <mailto: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]; >> }; >> @@ -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); > > (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 <lersek@redhat.com <mailto:lersek@redhat.com>> > > I also tested this series (with the assignment under (2) fixed up, of course), as documented earlier in <https://www.mail-archive.com/qemu-devel@nongnu.org/msg430075.html <https://www.mail-archive.com/qemu-devel@nongnu.org/msg430075.html>> (msgid <678c203f-3768-7e65-6e48-6729473b6ffa@redhat.com <mailto:678c203f-3768-7e65-6e48-6729473b6ffa@redhat.com>>). > > Hence, with (1) and (2) fixed, you can also add > > Tested-by: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>> > > 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
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 + * @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); + 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