> On Feb 15, 2017, at 10:35 AM, Igor Mammedov <imamm...@redhat.com> wrote: > > On Wed, 15 Feb 2017 10:14:55 -0800 > Ben Warren <b...@skyportsystems.com <mailto: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)); > 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 <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
