> 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)
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> (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 
smime.p7s
Description: S/MIME cryptographic signature
