On Feb 8, 2017, at 4:30 AM, Laszlo Ersek <lersek@redhat.com> wrote:

On 02/05/17 18:09, ben@skyportsystems.com wrote:

From: Ben Warren <ben@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@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 {