Re: [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  wrote:
>
> On 02/05/17 18:09, [e-mail]  wrote:
> > From: Ben Warren
> >
> > 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
> >
> > 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 {