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. > > > > > > > > > > (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 > >