[SeaBIOS] [PATCH v3 2/2] QEMU fw_cfg: Add command to write back address of file

[Ben Warren]

> On Feb 8, 2017, at 4:30 AM, Laszlo Ersek <lersek at redhat.com> wrote:
> 
> On 02/05/17 18:09, ben at skyportsystems.com <mailto: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
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 {

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.coreboot.org/pipermail/seabios/attachments/20170208/6834d3a6/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3583 bytes
Desc: not available
URL: <http://www.coreboot.org/pipermail/seabios/attachments/20170208/6834d3a6/attachment-0001.p7s>