> On Feb 15, 2017, at 10:24 AM, Igor Mammedov <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> 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; >> >> >> 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 <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
