[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: |
Ben Warren |
|
Subject: |
Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command |
|
Date: |
Wed, 15 Feb 2017 10:44:05 -0800 |
> On Feb 15, 2017, at 10:35 AM, Igor Mammedov <address@hidden> wrote:
>
> On Wed, 15 Feb 2017 10:14:55 -0800
> Ben Warren <address@hidden <mailto:address@hidden>> wrote:
>
>>> On Feb 15, 2017, at 10:06 AM, Michael S. Tsirkin <address@hidden> wrote:
>>>
>>> On Wed, Feb 15, 2017 at 09:54:08AM -0800, Ben Warren wrote:
>>>>
>>>> On Feb 15, 2017, at 9:43 AM, Igor Mammedov <address@hidden> 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;
>>>>
>>>>
>>>> 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));
>
OK, the more places I can get rid of this goofy offset the better. Just to be
clear, the address that gets patched into AML (via the add_pointer() call)
remains at the beginning of /etc/vmgenid_guid, right?
>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> (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
smime.p7s
Description: S/MIME cryptographic signature
- 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, 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 <=
- 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, 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, 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