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

[Igor Mammedov]

On Thu, 16 Feb 2017 14:29:28 +0100
Laszlo Ersek <address@hidden> wrote:

> 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.
instead of explicit offsets for base offset, device will probably use
struct cast to access fields.


> 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!

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