From: "Michael S. Tsirkin" <mst@redhat.com>
To: Ben Warren <ben@skyportsystems.com>
Cc: Igor Mammedov <imammedo@redhat.com>,
Laszlo Ersek <lersek@redhat.com>,
qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command
Date: Wed, 15 Feb 2017 20:06:39 +0200 [thread overview]
Message-ID: <20170215200506-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <5216BB96-2CB8-4E83-8950-40E44EE5264D@skyportsystems.com>
On Wed, Feb 15, 2017 at 09:54:08AM -0800, Ben Warren wrote:
>
> On Feb 15, 2017, at 9:43 AM, Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Wed, 15 Feb 2017 18:39:06 +0200
> "Michael S. Tsirkin" <mst@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" <mst@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 <lersek@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
> ben@skyportsystems.com wrote:
>
>
> From: Ben Warren <ben@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 <
> ben@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.
>
>
>
>
>
>
>
>
>
> (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 <lersek@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
>
>
next prev parent reply other threads:[~2017-02-15 18:06 UTC|newest]
Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-15 6:15 [Qemu-devel] [PATCH v6 0/7] Add support for VM Generation ID ben
2017-02-15 6:15 ` [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command ben
2017-02-15 10:57 ` Igor Mammedov
2017-02-15 14:13 ` Laszlo Ersek
2017-02-15 14:17 ` Laszlo Ersek
2017-02-15 15:22 ` Igor Mammedov
2017-02-15 15:30 ` Michael S. Tsirkin
2017-02-15 15:56 ` Igor Mammedov
2017-02-15 16:39 ` Michael S. Tsirkin
2017-02-15 17:19 ` Laszlo Ersek
2017-02-15 17:43 ` Igor Mammedov
2017-02-15 17:54 ` Ben Warren
2017-02-15 18:06 ` Michael S. Tsirkin [this message]
2017-02-15 18:14 ` Ben Warren
2017-02-15 18:35 ` Igor Mammedov
2017-02-15 18:44 ` Ben Warren
2017-02-15 21:09 ` Michael S. Tsirkin
2017-02-15 18:04 ` Michael S. Tsirkin
2017-02-15 18:24 ` Igor Mammedov
2017-02-15 19:14 ` Ben Warren
2017-02-15 19:19 ` Ben Warren
2017-02-16 11:10 ` Igor Mammedov
2017-02-16 15:38 ` Eric Blake
2017-02-15 19:34 ` Michael S. Tsirkin
2017-02-16 8:25 ` Igor Mammedov
2017-02-16 9:49 ` Laszlo Ersek
2017-02-15 6:15 ` [Qemu-devel] [PATCH v6 2/7] docs: VM Generation ID device description ben
2017-02-15 11:07 ` Igor Mammedov
2017-02-15 14:26 ` Laszlo Ersek
2017-02-15 6:15 ` [Qemu-devel] [PATCH v6 3/7] ACPI: Add vmgenid blob storage to the build tables ben
2017-02-15 11:15 ` Igor Mammedov
2017-02-15 14:30 ` Laszlo Ersek
2017-02-16 6:11 ` Ben Warren
2017-02-15 6:15 ` [Qemu-devel] [PATCH v6 4/7] ACPI: Add Virtual Machine Generation ID support ben
2017-02-15 12:19 ` Igor Mammedov
2017-02-15 15:24 ` Laszlo Ersek
2017-02-15 16:07 ` Igor Mammedov
2017-02-15 16:40 ` Michael S. Tsirkin
2017-02-15 17:12 ` Ben Warren
2017-02-16 6:13 ` Ben Warren
2017-02-15 17:11 ` Ben Warren
2017-02-15 6:15 ` [Qemu-devel] [PATCH v6 5/7] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands ben
2017-02-15 15:36 ` Laszlo Ersek
2017-02-16 6:13 ` Ben Warren
2017-02-15 6:15 ` [Qemu-devel] [PATCH v6 6/7] tests: Move reusable ACPI macros into a new header file ben
2017-02-15 12:54 ` Igor Mammedov
2017-02-15 21:35 ` Eric Blake
2017-02-15 21:58 ` Ben Warren
2017-02-15 22:56 ` Eric Blake
2017-02-15 23:05 ` Ben Warren
2017-02-15 6:15 ` [Qemu-devel] [PATCH v6 7/7] tests: Add unit tests for the VM Generation ID feature ben
2017-02-15 13:13 ` Igor Mammedov
2017-02-16 6:15 ` Ben Warren
2017-02-15 19:47 ` [Qemu-devel] [PATCH v6 0/7] Add support for VM Generation ID Laszlo Ersek
2017-02-15 20:09 ` Michael S. Tsirkin
2017-02-15 20:15 ` Ben Warren
2017-02-15 20:52 ` Laszlo Ersek
2017-02-16 6:10 ` Ben Warren
2017-02-16 9:36 ` Laszlo Ersek
2017-02-16 12:08 ` Igor Mammedov
2017-02-16 13:29 ` Laszlo Ersek
2017-02-16 14:27 ` Igor Mammedov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170215200506-mutt-send-email-mst@kernel.org \
--to=mst@redhat.com \
--cc=ben@skyportsystems.com \
--cc=imammedo@redhat.com \
--cc=lersek@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).