On Wed, 15 Feb 2017 11:19:44 -0800 Ben Warren <b...@skyportsystems.com> wrote:
> > On Feb 15, 2017, at 11:14 AM, Ben Warren <b...@skyportsystems.com> wrote: > > > >> > >> On Feb 15, 2017, at 10:24 AM, Igor Mammedov <imamm...@redhat.com > >> <mailto:imamm...@redhat.com>> wrote: > >> > >> On Wed, 15 Feb 2017 20:04:40 +0200 > >> "Michael S. Tsirkin" <m...@redhat.com <mailto:m...@redhat.com>> wrote: > >> > >>> On Wed, Feb 15, 2017 at 06:43:09PM +0100, Igor Mammedov 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 <mailto: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 <mailto: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 <mailto:b...@skyportsystems.com> wrote: > >>>>>>>>>> > >>>>>>>>>>> From: Ben Warren <b...@skyportsystems.com > >>>>>>>>>>> <mailto: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 > >>>>>>>>>>> <mailto: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; > >>> > >>> > >>> As long as all users pass in 0 though there's a real possibility guests > >>> will implement this incorrectly. > >> We are here to ensure that at least Seabios (I'll review it) > >> and OVMF (Laszlo would take care of it I suppose) do it right, > >> and if there are other firmwares, they should do it correctly > >> as described fix their own bugs later wrt randomly written > >> implementation. > >> > >>> I guess we can put in the offset just > >>> behind the zero-filled padding we have there. > >> I've assumed padding was there to make commands fixed size and give > >> a room for future extensions so hunk changing BiosLinkerLoaderEntry > >> would look like: > >> > > I can’t say I follow the logic of these extra paddings. The sizes of the > > structs are all over the place, so adding 4 bytes doesn’t do much. I > > assume you have a good reason, though. > > > >> diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c > >> index d963ebe..6983713 100644 > >> --- a/hw/acpi/bios-linker-loader.c > >> +++ b/hw/acpi/bios-linker-loader.c > >> @@ -49,6 +49,7 @@ struct BiosLinkerLoaderEntry { > >> char file[BIOS_LINKER_LOADER_FILESZ]; > >> uint32_t align; > >> uint8_t zone; > >> + uint32_t padding; > > I’m a little wary of doing this - in a packed structure this new field will > > be mis-aligned. > >> } alloc; > >> > >> /* > >> @@ -62,6 +63,7 @@ struct BiosLinkerLoaderEntry { > >> char src_file[BIOS_LINKER_LOADER_FILESZ]; > >> uint32_t offset; > >> uint8_t size; > >> + uint32_t padding; > >> } pointer; > >> > >> /* > >> @@ -76,10 +78,25 @@ struct BiosLinkerLoaderEntry { > >> uint32_t offset; > >> uint32_t start; > >> uint32_t length; > >> + uint32_t padding; > >> } cksum; > >> > >> + /* > >> + * 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 dst_offset; > >> + uint32_t src_offset; > >> + uint8_t size; > >> + } wr_pointer; > >> + > >> /* padding */ > >> - char pad[124]; > >> + char pad[120]; > > wr_pointer is 121 (56 + 56 + 32 + 32 + 1), so 124 still makes sense, > > doesn’t it? (also, 124 + 4 from command) % 8 == 0, so it’s nicely aligned. > I mean (56 + 56 + 4 + 4 + 1), of course :) I' was wrong, as Laszlo pointed out pad is a member of union so it as the biggest member defines total size of union/struct as far as new command doesn't exceed 124 bytes size. > >> }; > >> } QEMU_PACKED; > >> typedef struct BiosLinkerLoaderEntry BiosLinkerLoaderEntry; > >> > >> > >>> I'm mostly concerned we are adding new features to something > >>> that has been through 25 revisions already. > >> It's ABI so it's worth extra effort, > >> it looks like only one more revision is left and there is > >> about a week left to post and merge it. > >> > >>> > >>> > >>>>> > >>>>>>> > >>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>>> (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 > >>>>>>>>> <mailto: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 >
