[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' c
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command |
Date: |
Wed, 15 Feb 2017 20:04:40 +0200 |
On Wed, Feb 15, 2017 at 06:43:09PM +0100, Igor Mammedov wrote:
> On Wed, 15 Feb 2017 18:39:06 +0200
> "Michael S. Tsirkin" <address@hidden> 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" <address@hidden> 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 <address@hidden> 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
> > > > > > > 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 | 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;
As long as all users pass in 0 though there's a real possibility guests
will implement this incorrectly. I guess we can put in the offset just
behind the zero-filled padding we have there.
I'm mostly concerned we are adding new features to something
that has been through 25 revisions already.
> >
> > > >
> > > >
> > > > >
> > > > >
> > > > > > (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 <address@hidden>
> > > > > >
> > > > > > (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
> > > > > > >
> > > > > >
> >
- Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command, (continued)
- Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command, Igor Mammedov, 2017/02/15
- Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command, Michael S. Tsirkin, 2017/02/15
- Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command, Laszlo Ersek, 2017/02/15
- Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command, Igor Mammedov, 2017/02/15
- Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command, Ben Warren, 2017/02/15
- Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command, Michael S. Tsirkin, 2017/02/15
- Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command, Ben Warren, 2017/02/15
- Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command, Igor Mammedov, 2017/02/15
- Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command, Ben Warren, 2017/02/15
- Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command, Michael S. Tsirkin, 2017/02/15
- Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command, Igor Mammedov, 2017/02/15
- Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command, Ben Warren, 2017/02/15
- Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command, Ben Warren, 2017/02/15
- Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command, Igor Mammedov, 2017/02/16
- Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command, Eric Blake, 2017/02/16
- Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command, Michael S. Tsirkin, 2017/02/15
- Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command, Igor Mammedov, 2017/02/16
- Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command, Laszlo Ersek, 2017/02/16
[Qemu-devel] [PATCH v6 5/7] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands, ben, 2017/02/15