[SeaBIOS] [PATCH v3 2/2] QEMU fw_cfg: Add command to write back address of file
Laszlo Ersek
lersek at redhat.com
Wed Feb 8 13:30:51 CET 2017
On 02/05/17 18:09, ben at skyportsystems.com wrote:
> From: Ben Warren <ben at skyportsystems.com>
>
> This command is similar to ADD_POINTER, but instead of patching
> memory, it writes the pointer back to QEMU over the DMA interface.
>
> Signed-off-by: Ben Warren <ben at skyportsystems.com>
> ---
> src/fw/romfile_loader.c | 37 +++++++++++++++++++++++++++++++++++++
> src/fw/romfile_loader.h | 16 ++++++++++------
> 2 files changed, 47 insertions(+), 6 deletions(-)
>
> diff --git a/src/fw/romfile_loader.c b/src/fw/romfile_loader.c
> index f4b17ff..d0ae42b 100644
> --- a/src/fw/romfile_loader.c
> +++ b/src/fw/romfile_loader.c
> @@ -5,6 +5,7 @@
> #include "romfile.h" // struct romfile_s
> #include "malloc.h" // Zone*, _malloc
> #include "output.h" // warn_*
> +#include "paravirt.h" // qemu_cfg_write_file
>
> struct romfile_loader_file {
> struct romfile_s *file;
> @@ -98,7 +99,39 @@ static void romfile_loader_add_pointer(struct romfile_loader_entry_s *entry,
> pointer += (unsigned long)src_file->data;
> pointer = cpu_to_le64(pointer);
> memcpy(dest_file->data + offset, &pointer, entry->pointer_size);
> + return;
> +err:
> + warn_internalerror();
> +}
> +
> +static void romfile_loader_write_pointer(struct romfile_loader_entry_s *entry,
> + struct romfile_loader_files *files)
> +{
> + struct romfile_s *dest_file;
> + struct romfile_loader_file *src_file;
> + unsigned offset = le32_to_cpu(entry->pointer_offset);
> + u64 pointer = 0;
There's no need to zero-init "pointer" here. It doesn't hurt of course;
I just like to avoid initialization if it is useless.
> +
> + /* Writing back to a file that may not be loaded in RAM */
> + dest_file = romfile_find(entry->pointer_dest_file);
> + src_file = romfile_loader_find(entry->pointer_src_file, files);
>
> + if (!dest_file || !src_file || !src_file->data ||
> + offset + entry->pointer_size < offset ||
> + offset + entry->pointer_size > dest_file->size ||
> + entry->pointer_size < 1 || entry->pointer_size > 8 ||
> + entry->pointer_size & (entry->pointer_size - 1)) {
> + goto err;
> + }
> +
> + pointer = (unsigned long)src_file->data;
> + pointer = cpu_to_le64(pointer);
> +
> + /* Only supported on QEMU */
> + if (qemu_cfg_write_file(&pointer, dest_file, offset,
> + entry->pointer_size) != entry->pointer_size) {
> + goto err;
> + }
> return;
> err:
> warn_internalerror();
> @@ -161,6 +194,10 @@ int romfile_loader_execute(const char *name)
> break;
> case ROMFILE_LOADER_COMMAND_ADD_CHECKSUM:
> romfile_loader_add_checksum(entry, files);
> + break;
> + case ROMFILE_LOADER_COMMAND_WRITE_POINTER:
> + romfile_loader_write_pointer(entry, files);
> + break;
> default:
> /* Skip commands that we don't recognize. */
> break;
> diff --git a/src/fw/romfile_loader.h b/src/fw/romfile_loader.h
> index 15eab2a..0c0782c 100644
> --- a/src/fw/romfile_loader.h
> +++ b/src/fw/romfile_loader.h
> @@ -25,10 +25,13 @@ struct romfile_loader_entry_s {
> };
>
> /*
> - * COMMAND_ADD_POINTER - patch the table (originating from
> - * @dest_file) at @pointer_offset, by adding a pointer to the table
> + * COMMAND_ADD_POINTER &
> + * COMMAND_WRITE_POINTER - patch memory (originating from
> + * @dest_file) at @pointer.offset, by adding a pointer to the memory
> * originating from @src_file. 1,2,4 or 8 byte unsigned
> - * addition is used depending on @pointer_size.
> + * addition is used depending on @pointer.size.
> + * Instead of patching memory, COMMAND_WRITE_POINTER writes the changes
> + * to @dest_file back to the host via DMA
> */
> struct {
> char pointer_dest_file[ROMFILE_LOADER_FILESZ];
In general I think the patch is good, but (similarly to the QEMU case) I
quite dislike this kind of repurposing.
At the least, please leave the original comment block for
COMMAND_ADD_POINTER intact, and add a separate, standalone comment block
for COMMAND_WRITE_POINTER.
Thanks!
Laszlo
> @@ -57,9 +60,10 @@ struct romfile_loader_entry_s {
> };
>
> enum {
> - ROMFILE_LOADER_COMMAND_ALLOCATE = 0x1,
> - ROMFILE_LOADER_COMMAND_ADD_POINTER = 0x2,
> - ROMFILE_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
> + ROMFILE_LOADER_COMMAND_ALLOCATE = 0x1,
> + ROMFILE_LOADER_COMMAND_ADD_POINTER = 0x2,
> + ROMFILE_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
> + ROMFILE_LOADER_COMMAND_WRITE_POINTER = 0x4,
> };
>
> enum {
>
More information about the SeaBIOS
mailing list