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 {
On Feb 8, 2017, at 4:30 AM, Laszlo Ersek lersek@redhat.com wrote:
On 02/05/17 18:09, ben@skyportsystems.com mailto: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
OK, sometimes I take minimalism a bit far :)
@@ -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 {
-
Ben Warren -
Laszlo Ersek