Re: [Qemu-devel] [PATCH v6 0/7] Add support for VM Generation ID

[Laszlo Ersek]

On 02/16/17 13:08, Igor Mammedov wrote:
> On Wed, 15 Feb 2017 21:52:55 +0100
> Laszlo Ersek <address@hidden> wrote:
> 
>> On 02/15/17 21:09, Michael S. Tsirkin wrote:
>>> On Wed, Feb 15, 2017 at 08:47:48PM +0100, Laszlo Ersek wrote:  
>>
>> [snip]
>>
>>>> For patches #1, #3, #4 and #5:
>>>>
>>>> Tested-by: Laszlo Ersek <address@hidden>
>>>>
>>>> I'll soon post the OVMF patches.
>>>>
>>>> Thanks!
>>>> Laszlo  
>>>
>>>
>>> How do you feel about Igor's request to change WRITE_POINTER to add
>>> offset in there, so guest can pass in the address of GUID and
>>> not start of table? Would that be a lot of work to add?  
>>
>> I think it's doable in practice: simply add a constant from the command
>> itself, for passing the value back to QEMU, and also for saving the
>> fw_cfg write commend for S3 resume time.
>>
>> But, I disagree with it from a design POV.
>>
>> Igor's point is:
>>
>>> Math complicates QEMU code though and not only QMEMU but AML code as
>>> well.  
>>
>> As I understand it, the goal is to push the addition to the firmware
>> (which is "one place"), rather than having to implement it twice in
>> QEMU, i.e., in two places ((a) native QEMU logic, (b) AML generator).
>>
>> Here's my counter-argument:
>>
>> (a) As I mentioned earlier, assume a complex communication structure
>> between the guest OS and QEMU. Currently our shared structure consists
>> of a single field (the GUID), but next time it might contain several fields.
>>
>> For such a multi-field shared structure, QEMU will have to do manual
>> offsetting into the guest RAM anyway, for accessing fields F1, F2, and
>> F3. We will not create three separate WRITE_POINTER commands and let the
>> firmware calculate and return the absolute GPAs of the fields F1, F2 and
>> F3. Instead, there will be one WRITE_POINTER command, and QEMU will do
>> the offsetting manually, minimally for fields F2 and F3.
>>
>> "src_offset" looks tempting now only because we have a shared structure
>> with only one field, the GUID at offset 40 decimal.
> 
> benefits of having src_offset from QEMU POV I see are:
>  a) (biggest one) firmware and device code are clearly separated where:
>      - VMGENID_GUID_OFFSET would be used only by firmware side, such as:
>          WRITE_POINTER and AML addition to help OVMF detect non ACPI blob
>      - device doesn't have to assume/or have a knowledge about
>        layout of GUID blob except of size of data it's needs
>        to access at location provided by WRITE_POINTER as v7 shows it.
> 
>  b) wrt shared blob I've envisioned slightly different approach,
>     where multiple WRITE_POINTER commands are used instead of one
>     with following workflow to extend shared blob:
>      1) firmware part of QEMU (acpi-build.c):
>           if (device_foo_present) {
>              fw_cfg_add_file_callback('/etc/device_foo_addr', 
> device_foo->addr_storage)
> 
>              shared_off = device_foo->align(next_free_shared_offset)
>              WRITE_POINTER('/etc/device_foo_addr', 0,
>                            '/etc/shared_blob, shared_off)
> 
>              next_free_shared_offset = shared_off + device_foo->data_size;
>           }
>      2) device_foo accesses data at device_foo->addr_storage directly
>         * there is no need to spread knowledge of shared_blob
>           layout to device code anymore.

This is where I disagree, I think. Above you mention
device_foo->data_size. If "data_size" covers multiple fields, then the
device code itself will have to add relative offsets, for accessing
those different fields in guest RAM.

With the current command, the only difference is that the device code
has to receive a "base offset" from the outside, pointing into the
shared blob, and then add the field offsets to that. Thus the addition
cannot be avoided anyway.

You do have a point about sharing the same area between different
devices though. The above pseudo-code looks like a good pattern. This
way "acpi-build.c" won't have to hand out the shared blob offsets to
existing device instances directly; instead, the blob offsets are handed
down to the firmware, and the devices will get their struct base
addresses from the firmware. Using one WRITE_POINTER command for that,
per device, seems fine.

I'll update the OVMF patches.

Thanks
Laszlo

>         * no need to care where in shared_blob data will be placed,
>         * shared space is used only when device is present
>         * since there is no shared_writeback_blob, there isn't 
>           need in mechanism to propagate written data to device
>           or notify device about write
>      
>    drawback in this approach is that a device would consume
>    a file slot if fw_cfg and space for WRITE_POINTER in
>    linker file when present.
> 
>  
>> (b) Regarding the runtime addition in the AML code:
> as you pointed out WRITE_POINTER has nothing to do with addition
> on AML side which is influenced by ADD_POINTER and OVMF and could
> be fixed with flags down the road, so there is nothing to argue
> about on this bullet.
> 
> 
>> As discussed before, the main reason *now*, for not pointing VGIA (and
>> other named integer objects) with ADD_POINTER commands directly to
>> "meaningful" fields, is that OVMF probes the targets of ADD_POINTER
>> commands for patterns that look like ACPI table headers. And, for the
>> time being, we want to suppress any mis-recognitions by prepending some
>> padding.
>>
>> Igor was right to dislike this, and we agreed that *down the road* we
>> should add allocation flags, or further allocation commands, to supplant
>> this kind of heuristics in OVMF. But:
>>
>> - we don't have time to do it now, plus
>>
>> - please observe that the runtime addition in AML relates to the
>>   ADD_POINTER and the ALLOCATE commands. It does not relate to
>>   WRITE_POINTER at all.
>>
>>   Whatever we change on WRITE_POINTER will do nothing for suppressing
>>   OVMF's table header probing -- because that is tied to ADD_POINTER
>>   --, therefore WRITE_POINTER tweaks cannot eliminate the "need to add"
>>   in AML.
>>
>>
>> In summary, I think the proposed WRITE_POINTER modification is
>> implementable, but I think it will not pay off, because:
>>
>> (a) for QEMU logic, it will not prove useful as soon as we have a
>> multi-field shared structure (QEMU will have to add field offsets anyway),
>>
>> (b) and for eliminating the AML addition (which is a consequence of the
>> current ADD_POINTER handling in OVMF), it does nothing.
>>
>> Thanks
>> Laszlo
>