| Message ID | 20170215192436.2695b9f6@nial.brq.redhat.com |
|---|---|
| State | New |
| Headers | show |
> On Feb 15, 2017, at 10:24 AM, Igor Mammedov <imammedo@redhat.com> wrote: > > On Wed, 15 Feb 2017 20:04:40 +0200 > "Michael S. Tsirkin" <mst@redhat.com <mailto:mst@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" <mst@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" <mst@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 <lersek@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 >>>>>>>>> ben@skyportsystems.com wrote: >>>>>>>>> >>>>>>>>>> From: Ben Warren <ben@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 <ben@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. > }; > } 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 <lersek@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
> On Feb 15, 2017, at 11:14 AM, Ben Warren <ben@skyportsystems.com> wrote: > >> >> On Feb 15, 2017, at 10:24 AM, Igor Mammedov <imammedo@redhat.com <mailto:imammedo@redhat.com>> wrote: >> >> On Wed, 15 Feb 2017 20:04:40 +0200 >> "Michael S. Tsirkin" <mst@redhat.com <mailto:mst@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" <mst@redhat.com <mailto:mst@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" <mst@redhat.com <mailto:mst@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 <lersek@redhat.com <mailto:lersek@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 >>>>>>>>>> ben@skyportsystems.com <mailto:ben@skyportsystems.com> wrote: >>>>>>>>>> >>>>>>>>>>> From: Ben Warren <ben@skyportsystems.com <mailto:ben@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 <ben@skyportsystems.com <mailto:ben@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 :) >> }; >> } 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 <lersek@redhat.com <mailto:lersek@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
On Wed, Feb 15, 2017 at 07:24:36PM +0100, Igor Mammedov wrote: > > 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. As long as it's untested, it can always do the wrong thing even if it looks right, or it can bitrot. > > 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 you mean char pad[124]; ? Right. So you can easily add fields, but if you do firmware will just assume it does not know how to handle these commands and skip them. > so hunk changing BiosLinkerLoaderEntry > would look like: > > 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; > } 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; why is this necessary? > + /* > + * 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]; and this shrinks entry size by 4 bytes. Why? > }; > } 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, There's always yet another possible enhancement you can add. I see it as a bit of a feature creep frankly. > it looks like only one more revision is left and there is > about a week left to post and merge it. If we can do it quickly, fine, but I think we should merge ASAP.
On Wed, 15 Feb 2017 21:34:45 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Feb 15, 2017 at 07:24:36PM +0100, Igor Mammedov wrote: > > > 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. > > As long as it's untested, it can always do the wrong thing > even if it looks right, or it can bitrot. > > > > 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 > > you mean > char pad[124]; > ? > > Right. So you can easily add fields, but if you do firmware > will just assume it does not know how to handle these > commands and skip them. > > > so hunk changing BiosLinkerLoaderEntry > > would look like: > > > > 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; > > } 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; > > why is this necessary? > > > + /* > > + * 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]; > > and this shrinks entry size by 4 bytes. Why? so total size won't change, due to +4 to account for src_offest in wr_pointer > > > }; > > } 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, > > There's always yet another possible enhancement you can add. > I see it as a bit of a feature creep frankly. > > > it looks like only one more revision is left and there is > > about a week left to post and merge it. > > If we can do it quickly, fine, but I think we should merge ASAP. >
On 02/16/17 09:25, Igor Mammedov wrote: > On Wed, 15 Feb 2017 21:34:45 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > >> On Wed, Feb 15, 2017 at 07:24:36PM +0100, Igor Mammedov wrote: >>>> 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. >> >> As long as it's untested, it can always do the wrong thing >> even if it looks right, or it can bitrot. >> >>>> 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 >> >> you mean >> char pad[124]; >> ? >> >> Right. So you can easily add fields, but if you do firmware >> will just assume it does not know how to handle these >> commands and skip them. >> >>> so hunk changing BiosLinkerLoaderEntry >>> would look like: >>> >>> 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; >>> } 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; >> >> why is this necessary? >> >>> + /* >>> + * 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]; >> >> and this shrinks entry size by 4 bytes. Why? > so total size won't change, due to +4 to account for src_offest in wr_pointer To be clear, I still disagree with the introduction of "src_offset" (please let's discuss that in the other sub-thread). Still I can comment on this question in isolation: The individual command structures, and the "pad" field (which is 124 bytes), are all members of a *union*. They all start at offset zero in the union, and the union's size is determined by the largest member, plus any additional padding at the end that the compiler wishes to append. (In this case, becasue we use QEMU_PACKED, the compiler doesn't do that.) Therefore, as long as you don't exceed the size of char pad[124]; you can extend any of the command structures freely -- "pad" will remain the largest union member, and thereby dictate the size of the union. In short, "pad" does not *follow* any of the command structures; they are all laid over each other, from offset 0 in the union. (If you are curious about the magic constant 124, that comes from 128 minus the size of the "command" field in the outermost BiosLinkerLoaderEntry structure. The guiding principle here is that the outermost BiosLinkerLoaderEntry structure be 128 bytes in size.) --*-- Again: I disagree with the introduction of "src_offset", and I'd like to hear your opinion about my counter-argument in the other subthread. Thanks Laszlo > >> >>> }; >>> } 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, >> >> There's always yet another possible enhancement you can add. >> I see it as a bit of a feature creep frankly. >> >>> it looks like only one more revision is left and there is >>> about a week left to post and merge it. >> >> If we can do it quickly, fine, but I think we should merge ASAP. >> >
On Wed, 15 Feb 2017 11:19:44 -0800 Ben Warren <ben@skyportsystems.com> wrote: > > On Feb 15, 2017, at 11:14 AM, Ben Warren <ben@skyportsystems.com> wrote: > > > >> > >> On Feb 15, 2017, at 10:24 AM, Igor Mammedov <imammedo@redhat.com <mailto:imammedo@redhat.com>> wrote: > >> > >> On Wed, 15 Feb 2017 20:04:40 +0200 > >> "Michael S. Tsirkin" <mst@redhat.com <mailto:mst@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" <mst@redhat.com <mailto:mst@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" <mst@redhat.com <mailto:mst@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 <lersek@redhat.com <mailto:lersek@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 > >>>>>>>>>> ben@skyportsystems.com <mailto:ben@skyportsystems.com> wrote: > >>>>>>>>>> > >>>>>>>>>>> From: Ben Warren <ben@skyportsystems.com <mailto:ben@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 <ben@skyportsystems.com <mailto:ben@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 <lersek@redhat.com <mailto:lersek@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 >
[meta-comment] On 02/16/2017 05:10 AM, Igor Mammedov wrote: > On Wed, 15 Feb 2017 11:19:44 -0800 > Ben Warren <ben@skyportsystems.com> wrote: > >>> On Feb 15, 2017, at 11:14 AM, Ben Warren <ben@skyportsystems.com> wrote: ... >>>>>>>>>>>>> This is similar to the existing 'add pointer' functionality, but instead 13 levels of quoting... >>>> + 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. ...and several pages of scrolling before I find the 3-line real content of the message... >>>>>>>>>>>>> #endif ...buried by yet more scrolling through excessive quoting. It's not only okay, but ENCOURAGED, to trim your replies down to the relevant portions (as I've done here), to quickly call attention to the content you are adding to the thread. Publicly archived lists have the benefit that you don't have to full-quote the entire thread, because readers interested in trimmed content can refer to the archives.
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; } 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]; }; } QEMU_PACKED; typedef struct BiosLinkerLoaderEntry BiosLinkerLoaderEntry;