On Thu, 16 Feb 2017 14:29:28 +0100
Laszlo Ersek <ler...@redhat.com> wrote:
> On 02/16/17 13:08, Igor Mammedov wrote:
> > On Wed, 15 Feb 2017 21:52:55 +0100
> > Laszlo Ersek <ler...@redhat.com> 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 <ler...@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.
> >
> > 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
> >
>
