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