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

[Ben Warren]

> On Feb 15, 2017, at 12:52 PM, 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.
> 
> (b) Regarding the runtime addition in the AML code:
> 
> 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.
> 
OK Laszlo, in v7 (imminent)  I went ahead and implemented this src_offset.  If 
you are truly dead-set against it, it’s not very hard to remove.  To me it 
seems pretty harmless.

> Thanks
> Laszlo

smime.p7s
Description: S/MIME cryptographic signature