On Wed, 15 Feb 2017 10:14:55 -0800
Ben Warren <b...@skyportsystems.com> wrote:
> > On Feb 15, 2017, at 10:06 AM, Michael S. Tsirkin <m...@redhat.com> wrote:
> >
> > On Wed, Feb 15, 2017 at 09:54:08AM -0800, Ben Warren wrote:
> >>
> >> On Feb 15, 2017, at 9:43 AM, Igor Mammedov <imamm...@redhat.com> wrote:
> >>
> >> On Wed, 15 Feb 2017 18:39:06 +0200
> >> "Michael S. Tsirkin" <m...@redhat.com> wrote:
> >>
> >>
> >> On Wed, Feb 15, 2017 at 04:56:02PM +0100, Igor Mammedov wrote:
> >>
> >> On Wed, 15 Feb 2017 17:30:00 +0200
> >> "Michael S. Tsirkin" <m...@redhat.com> wrote:
> >>
> >>
> >> On Wed, Feb 15, 2017 at 04:22:25PM +0100, Igor Mammedov
> >> wrote:
> >>
> >>
> >> On Wed, 15 Feb 2017 15:13:20 +0100
> >> Laszlo Ersek <ler...@redhat.com> wrote:
> >>
> >>
> >> Commenting under Igor's reply for simplicity
> >>
> >> On 02/15/17 11:57, Igor Mammedov wrote:
> >>
> >> On Tue, 14 Feb 2017 22:15:43 -0800
> >> b...@skyportsystems.com wrote:
> >>
> >>
> >> From: Ben Warren <b...@skyportsystems.com>
> >>
> >> 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 <
> >> b...@skyportsystems.com>
> >> ---
> >> hw/acpi/bios-linker-loader.c | 58
> >> ++++++++++++++++++++++++++++++++++--
> >> include/hw/acpi/bios-linker-loader.h | 6
> >> ++++
> >> 2 files changed, 61 insertions(+), 3
> >> deletions
> >> (-)
> >>
> >> diff --git a/hw/acpi/bios-linker-loader.c
> >> b/hw/
> >> acpi/bios-linker-loader.c
> >> index d963ebe..5030cf1 100644
> >> --- a/hw/acpi/bios-linker-loader.c
> >> +++ b/hw/acpi/bios-linker-loader.c
> >> @@ -78,6 +78,19 @@ 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 the table
> >> + * originating from @src_file.
> >> 1,2,4
> >> or 8 byte unsigned
> >> + * addition is used depending on
> >> @wr_pointer.size.
> >> + */
> >>
> >>
> >> The words "adding" and "addition" are causing
> >> confusion
> >> here.
> >>
> >> In all of the previous discussion, *addition* was
> >> out
> >> of scope from
> >> WRITE_POINTER. Again, the firmware is specifically
> >> not
> >> required to
> >> *read* any part of the fw_cfg blob identified by
> >> "dest_file".
> >>
> >> WRITE_POINTER instructs the firmware to return the
> >> allocation address of
> >> the downloaded "src_file" to QEMU. Any necessary
> >> runtime subscripting
> >> within "src_file" is to be handled by QEMU code
> >> dynamically.
> >>
> >> For example, consider that "src_file" has *several*
> >> fields that QEMU
> >> wants to massage; in that case, indexing within QEMU
> >> code with field
> >> offsets is simply unavoidable.
> >>
> >> what I don't like here is that this indexing would be
> >> rather fragile
> >> and has to be done in different parts of QEMU /device,
> >> AML
> >> /.
> >>
> >> I'd prefer this helper function to have the same
> >> @src_offset
> >> behavior as ADD_POINTER where patched address could
> >> point
> >> to
> >> any part of src_file i.e. not just beginning.
> >>
> >>
> >>
> >>
> >> /*
> >> * COMMAND_ADD_POINTER - patch the table (originating
> >> from
> >> * @dest_file) at @pointer.offset, by adding a
> >> pointer
> >> to the table
> >> * originating from @src_file. 1,2,4 or 8 byte
> >> unsigned
> >> * addition is used depending on @pointer.size.
> >> */
> >>
> >> so the way ADD works is
> >> read at offset
> >> add table address
> >> write result at offset
> >>
> >> in other words it is always beginning of table that is
> >> added.
> >>
> >> more exactly it's, read at
> >> src_offset = *(dst_blob_ptr+dst_offset)
> >> *(dst_blob+dst_offset) = src_blob_ptr + src_offset
> >>
> >>
> >> Would the following be acceptable?
> >>
> >>
> >> * COMMAND_WRITE_POINTER - update the fw_cfg file
> >> (originating from
> >> * @dest_file) at @wr_pointer.offset, by writing a
> >> pointer to the table
> >> * originating from @src_file. 1,2,4 or 8 byte
> >> unsigned
> >> value
> >> * is written depending on @wr_pointer.size.
> >>
> >> it looses 'adding' part of ADD_POINTER command which handles
> >> src_offset,
> >> however implementing adding part looks a bit complicated
> >> as patched blob (dst) is not in guest memory but in QEMU and
> >> on reset *(dst_blob+dst_offset) should be reset to src_offset.
> >> Considering dst file could be device specific memory
> >> (field/blob/
> >> whatever)
> >> it could be hard to track/notice proper reset behavior.
> >>
> >> So now I'm not sure if src_offset is worth adding.
> >>
> >>
> >> Right. Let's just do this math in QEMU if we have to.
> >>
> >> Math complicates QEMU code though and not only QMEMU but AML code as
> >> well.
> >> Considering that we are adding a new command and don't have to keep
> >> any sort of compatibility we can pass src_offset as part
> >> of command instead of hiding it inside of dst_file.
> >> Something like this:
> >>
> >> /*
> >> * COMMAND_WRITE_POINTER - write the fw_cfg file (originating
> >> from
> >> * @dest_file) at @wr_pointer.offset, by writing 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 offset;
> >> + uint32_t dst_offset;
> >> + uint32_t src_offset;
> >> uint8_t size;
> >> } wr_pointer;
> >>
> >>
> >> OK, this is easy enough to do and maybe we’ll have a use case in the
> >> future.
> >> I’ll make this change in v7
> >
> >
> > So if you do, you want to set it to VMGENID_GUID_OFFSET.
> >
> Oh, I was going to set it to 0 since that doesn’t require any other changes
> (other than to SeaBIOS)
it should be changed in following places:
bios_linker_loader_write_pointer(linker,
VMGENID_ADDR_FW_CFG_FILE, 0, sizeof(uint64_t),
- VMGENID_GUID_FW_CFG_FILE);
+ VMGENID_GUID_FW_CFG_FILE, VMGENID_GUID_OFFSET);
...
- cpu_physical_memory_write(vmgenid_addr + VMGENID_GUID_OFFSET,
+ cpu_physical_memory_write(vmgenid_addr,
guid_le.data, sizeof(guid_le.data));
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >> (1) So, the above looks correct, but please replace
> >> "adding" with
> >> "storing", and "unsigned addition" with "store".
> >>
> >> Side point: the case for ADD_POINTER is different;
> >> there we patch
> >> several individual ACPI objects. The fact that I
> >> requested explicit
> >> addition within the ADDR method, as opposed to
> >> pre-setting VGIA to a
> >> nonzero offset, is an *incidental* limitation
> >> (coming
> >> from the OVMF ACPI
> >> SDT header probe suppressor), and we'll likely fix
> >> that
> >> up later, with
> >> ALLOCATE command hints or something like that.
> >> However,
> >> in
> >> WRITE_POINTER, asking for the exact allocation
> >> address
> >> of "src_file" is
> >> an *inherent* characteristic.
> >>
> >> For reference, this is the command's description
> >> from
> >> the (not as yet
> >> posted) OVMF series:
> >>
> >> // QemuLoaderCmdWritePointer: the bytes at
> >> // [PointerOffset..PointerOffset+PointerSize) in the
> >> writeable fw_cfg
> >> // file PointerFile are to receive the absolute
> >> address
> >> of PointeeFile,
> >> // as allocated and downloaded by the firmware.
> >> Store
> >> the base address
> >> // of where PointeeFile's contents have been placed
> >> (when
> >> // QemuLoaderCmdAllocate has been executed for
> >> PointeeFile) to this
> >> // portion of PointerFile.
> >> //
> >> // This command is similar to
> >> QemuLoaderCmdAddPointer;
> >> the difference is
> >> // that the "pointer to patch" does not exist in
> >> guest-physical address
> >> // space, only in "fw_cfg file space". In addition,
> >> the
> >> "pointer to
> >> // patch" is not initialized by QEMU with a possibly
> >> nonzero offset
> >> // value: the base address of the memory allocated
> >> for
> >> downloading
> >> // PointeeFile shall not increment the pointer, but
> >> overwrite it.
> >>
> >> In the last SeaBIOS patch series, namely
> >>
> >> [SeaBIOS] [PATCH v3 2/2] QEMU fw_cfg: Add command to
> >> write back address
> >> of file
> >>
> >> function romfile_loader_write_pointer() implemented
> >> just that plain
> >> store (not an addition), and that was exactly right.
> >>
> >> Continuing:
> >>
> >>
> >> + struct {
> >> + char dest_file
> >> [BIOS_LINKER_LOADER_FILESZ];
> >> + char src_file
> >> [BIOS_LINKER_LOADER_FILESZ];
> >> + uint32_t offset;
> >> + uint8_t size;
> >> + } wr_pointer;
> >> +
> >> /* padding */
> >> char pad[124];
> >> };
> >> @@ -85,9 +98,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 +292,41 @@ 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
> >> + */
> >> +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)
> >>
> >> API is missing "src_offset" even though it's not
> >> used in this series,
> >> a patch on top to fix it up is ok for me as far
> >> as
> >> Seabios/OVMF
> >> counterpart can handle src_offset correctly from
> >> starters.
> >>
> >>
> >> According to the above, it is the right thing not to
> >> add "src_offset"
> >> here. The documentation on the command is slightly
> >> incorrect (and causes
> >> confusion), but the helper function's signature and
> >> comments are okay.
> >>
> >>
> >>
> >>
> >> +{
> >> + BiosLinkerLoaderEntry entry;
> >> + const BiosLinkerFileEntry *source_file
> >> =
> >> + bios_linker_find_file(linker,
> >> src_file);
> >> +
> >> + assert(source_file);
> >>
> >>
> >> I wish we kept the following asserts from
> >> bios_linker_loader_add_pointer():
> >>
> >> assert(dst_patched_offset < dst_file->blob->len);
> >> assert(dst_patched_offset + dst_patched_size <=
> >> dst_file->blob->len);
> >>
> >> Namely, just because the dst_file is never supposed
> >> to
> >> be downloaded by
> >> the firmware, it still remains a requirement that
> >> the
> >> "dst file offset
> >> range" that is to be rewritten *do fall* within the
> >> dst
> >> file.
> >>
> >> Nonetheless, this is not critical. (OVMF at least
> >> verifies it anyway.)
> >>
> >> Summary (from my side anyway): I feel that the
> >> documentation of the new
> >> command is very important. Please fix it up as
> >> suggested under (1), in
> >> v7. Regarding the asserts, I'll let you decide.
> >>
> >> With the documentation fixed up:
> >>
> >> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
> >>
> >> (If you don't wish to post a v7, I'm also completely
> >> fine if Michael or
> >> someone else fixes up the docs as proposed in (1),
> >> before committing the
> >> patch.)
> >>
> >> Thanks!
> >> Laszlo
> >>
> >>
> >> + 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.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..f9ba5d6 100644
> >> --- a/include/hw/acpi/bios-linker-loader.h
> >> +++ b/include/hw/acpi/bios-linker-loader.h
> >> @@ -26,5 +26,11 @@ 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);
> >> +
> >> void bios_linker_loader_cleanup(BIOSLinker
> >> *linker);
> >> #endif
>
