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

[Igor Mammedov]

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.
        * 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