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

[Laszlo Ersek <lersek@redhat.com>]

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 <lersek@redhat.com>
>>
>> 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.

Thanks
Laszlo