> 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 <mailto: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
>> >>>> >>>> >>>>> >>>>> >>>>>> (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