Re: [Qemu-devel] [PATCH v7 1/8] linker-loader: Add new 'write pointer' command

[Michael S. Tsirkin]

On Thu, Feb 16, 2017 at 10:43:10AM +0100, Igor Mammedov wrote:
> On Wed, 15 Feb 2017 22:18:11 -0800
> address@hidden wrote:
> 
> > From: Ben Warren <address@hidden>
> > 
> > This is similar to the existing 'add pointer' functionality, but instead
> > of instructing the guest (BIOS or UEFI) to patch memory, it instructs
> > the guest to write the pointer back to QEMU via a writeable fw_cfg file.
> > 
> > Signed-off-by: Ben Warren <address@hidden>
> > ---
> >  hw/acpi/bios-linker-loader.c         | 66 
> > ++++++++++++++++++++++++++++++++++--
> >  include/hw/acpi/bios-linker-loader.h |  7 ++++
> >  2 files changed, 70 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
> > index d963ebe..d5fb703 100644
> > --- a/hw/acpi/bios-linker-loader.c
> > +++ b/hw/acpi/bios-linker-loader.c
> > @@ -78,6 +78,21 @@ struct BiosLinkerLoaderEntry {
> >              uint32_t length;
> >          } cksum;
> >  
> > +        /*
> > +         * COMMAND_WRITE_POINTER - write the fw_cfg file (originating from
> > +         * @dest_file) at @wr_pointer.offset, by adding a pointer to
> > +         * @src_offset within the table originating from @src_file.
> > +         * 1,2,4 or 8 byte unsigned addition is used depending on
> > +         * @wr_pointer.size.
> > +         */
> > +        struct {
> > +            char dest_file[BIOS_LINKER_LOADER_FILESZ];
> > +            char src_file[BIOS_LINKER_LOADER_FILESZ];
> > +            uint32_t dst_offset;
> > +            uint32_t src_offset;
> > +            uint8_t size;
> > +        } wr_pointer;
> > +
> >          /* padding */
> >          char pad[124];
> Shouldn't padding be reduced by 4 bytes to keep 
> sizeof(BiosLinkerLoaderEntry) the same as before patch,
> so that old bios would be able to skip this unknown command
> and read the next at the right offset?

IMHO no - because it's a union.


> >      };
> > @@ -85,9 +100,10 @@ struct BiosLinkerLoaderEntry {
> >  typedef struct BiosLinkerLoaderEntry BiosLinkerLoaderEntry;
> >  
> >  enum {
> > -    BIOS_LINKER_LOADER_COMMAND_ALLOCATE     = 0x1,
> > -    BIOS_LINKER_LOADER_COMMAND_ADD_POINTER  = 0x2,
> > -    BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
> > +    BIOS_LINKER_LOADER_COMMAND_ALLOCATE          = 0x1,
> > +    BIOS_LINKER_LOADER_COMMAND_ADD_POINTER       = 0x2,
> > +    BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM      = 0x3,
> > +    BIOS_LINKER_LOADER_COMMAND_WRITE_POINTER     = 0x4,
> >  };
> >  
> >  enum {
> > @@ -278,3 +294,47 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
> >  
> >      g_array_append_vals(linker->cmd_blob, &entry, sizeof entry);
> >  }
> > +
> > +/*
> > + * bios_linker_loader_write_pointer: ask guest to write a pointer to the
> > + * source file into the destination file, and write it back to QEMU via
> > + * fw_cfg DMA.
> > + *
> > + * @linker: linker object instance
> > + * @dest_file: destination file that must be written
> > + * @dst_patched_offset: location within destination file blob to be patched
> > + *                      with the pointer to @src_file, in bytes
> > + * @dst_patched_offset_size: size of the pointer to be patched
> > + *                      at @dst_patched_offset in @dest_file blob, in bytes
> > + * @src_file: source file who's address must be taken
> > + * @src_offset: location within source file blob to which
> > + *              @address@hidden will point to after
> > + *              firmware's executed WRITE_POINTER command
> > + */
> > +void bios_linker_loader_write_pointer(BIOSLinker *linker,
> > +                                    const char *dest_file,
> > +                                    uint32_t dst_patched_offset,
> > +                                    uint8_t dst_patched_size,
> > +                                    const char *src_file,
> > +                                    uint32_t src_offset)
> > +{
> > +    BiosLinkerLoaderEntry entry;
> > +    const BiosLinkerFileEntry *source_file =
> > +        bios_linker_find_file(linker, src_file);
> > +
> > +    assert(source_file);
> > +    assert(src_offset <= source_file->blob->len);
> off by one, should be '<'
> 
> > +    memset(&entry, 0, sizeof entry);
> > +    strncpy(entry.wr_pointer.dest_file, dest_file,
> > +            sizeof entry.wr_pointer.dest_file - 1);
> > +    strncpy(entry.wr_pointer.src_file, src_file,
> > +            sizeof entry.wr_pointer.src_file - 1);
> > +    entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_WRITE_POINTER);
> > +    entry.wr_pointer.dst_offset = cpu_to_le32(dst_patched_offset);
> > +    entry.wr_pointer.src_offset = cpu_to_le32(dst_patched_offset);
> > +    entry.wr_pointer.size = dst_patched_size;
> > +    assert(dst_patched_size == 1 || dst_patched_size == 2 ||
> > +           dst_patched_size == 4 || dst_patched_size == 8);
> > +
> > +    g_array_append_vals(linker->cmd_blob, &entry, sizeof entry);
> > +}
> > diff --git a/include/hw/acpi/bios-linker-loader.h 
> > b/include/hw/acpi/bios-linker-loader.h
> > index fa1e5d1..efe17b0 100644
> > --- a/include/hw/acpi/bios-linker-loader.h
> > +++ b/include/hw/acpi/bios-linker-loader.h
> > @@ -26,5 +26,12 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
> >                                      const char *src_file,
> >                                      uint32_t src_offset);
> >  
> > +void bios_linker_loader_write_pointer(BIOSLinker *linker,
> > +                                      const char *dest_file,
> > +                                      uint32_t dst_patched_offset,
> > +                                      uint8_t dst_patched_size,
> > +                                      const char *src_file,
> > +                                      uint32_t src_offset);
> > +
> >  void bios_linker_loader_cleanup(BIOSLinker *linker);
> >  #endif
> 
> Taking in account comments above are corner cases.
>  1: old bios running on new QEMU with vmgenid device and OLD/not supported 
> bios
>  2: assert check
> 
> It's ok fixes for above issues being fixed in follow up patch
> or as fixup while patch is staged in pci tree
> 
> Reviewed-by: Igor Mammedov <address@hidden>