Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Laszlo Ersek]

On 01/26/17 19:59, Michael S. Tsirkin wrote:
> On Thu, Jan 26, 2017 at 07:25:22PM +0100, Laszlo Ersek wrote:
>> On 01/26/17 19:15, Michael S. Tsirkin wrote:
>>> On Thu, Jan 26, 2017 at 06:43:22PM +0100, Laszlo Ersek wrote:
>>>> On 01/26/17 16:20, Michael S. Tsirkin wrote:
>>>>> On Thu, Jan 26, 2017 at 01:48:37AM +0100, Laszlo Ersek wrote:
>>>>
>>>>>> But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide.
>>>>>
>>>>>
>>>>> What is COMMAND_ALLOCATE_RETURN_ADDR? I'm only familiar with
>>>>> COMMAND_ALLOCATE.
>>>>
>>>> It's a new command being introduced in this series, at my suggestion. It
>>>> does the exact same thing as COMMAND_ALLOCATE, except once the
>>>> allocation / download is carried out by the firmware, the firmware
>>>> writes back the allocation address to the fw_cfg file that is named in
>>>> an additional field of the COMMAND_ALLOCATE_RETURN_ADDR structure. (This
>>>> is how QEMU learns where the blob in GPA space was placed by the
>>>> firmware.) The format for this address-receiving fw_cfg file is supposed
>>>> to be 8-byte, little endian.
>>>
>>> I see. I really think it's better as a separate command though.
>>> E.g. COMMAND_WRITE_PTR?
>>
>> Sure, but please provide specifics, otherwise Ben & myself will have a
>> hard time divining & implementing your intent :)
>>
>> Thanks,
>> Laszlo
> 
> I would say a variant of ADD_POINTER:
> 
>         /*
>        * COMMAND_WRITE_POINTER - update a writeable file named
>        * @pointer.dest_file at @pointer.offset, by writing pointer to
>        * the table originating from @src_file. 1,2,4 or 8 byte
>        * unsigned write is used depending on @pointer.size.
>          */
>         struct {
>             char dest_file[BIOS_LINKER_LOADER_FILESZ];
>             char src_file[BIOS_LINKER_LOADER_FILESZ];
>             uint32_t offset;
>             uint8_t size;
>         } pointer;

This will require more work in OVMF, but it does seem feasible, and I
agree this command is significantly more flexible than what I proposed.
In particular, it allows us to receive multiple guest-side allocation
addresses into the same writeable fw_cfg file, just at different offsets
of the fw_cfg file.

For the comment block above, I have one suggestion: replace

  pointer to the table originating from @src_file

with

  pointer to the blob originating from @src_file

"table" is quite overloaded in this context. As we normally pack several
ACPI tables in a single fw_cfg blob, I like to be pedantic about "blob"
vs. "table" (restricting the latter to "ACPI table"). E.g., structured
buffer coming from "etc/vmgenid" is not an ACPI table, but "blob"
definitely covers it.

If Ben is okay with it, I think this command is a good fit. I'll make an
effort to write the OVMF patches in time to test them with one of Ben's
next patch set versions (so we can determine if the QEMU stuff works
with both SeaBIOS and OVMF before committing the QEMU patches).

Thanks!
Laszlo