> On Feb 15, 2017, at 12:52 PM, 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. > > (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. > OK Laszlo, in v7 (imminent) I went ahead and implemented this src_offset. If you are truly dead-set against it, it’s not very hard to remove. To me it seems pretty harmless.
> Thanks > Laszlo
smime.p7s
Description: S/MIME cryptographic signature