On 02/16/17 07:10, Ben Warren wrote: > >> 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.
I'd like to hear Igor's opinion about my counter-argument above. It would be a fair bit of work in OVMF: - it introduces a new addition which has to be range checked (the sum cannot overflow either the pointer object or the size of the pointed-to blob), - if the range check fails, that's a new error condition that needs handling, - in edk2 we document error conditions carefully, so it affects documentation too, for the function that processes WRITE_POINTER, - I'd have to redo all the testing. After I post the OVMF patches, we can count on about one week of review time (assuming I don't have to post a v2, that is!). That comes pretty close to the end of February, which is when I want to do a downstream OVMF rebase. (I can't do it in March due to a conference, and then it's too late.) The downstream rebase will take a few days too. I would have preferred to post the OVMF patches near the beginning of this week. So, as I said, it is implementable, I'm not "dead-set" against it, but the price for me to pay is definitely not zero. And, as it stands, I disagree with the design -- there don't seem to be any general benefits. I'd like to hear back from Igor. If he convincingly refutes my argument, I'm game. Thanks, Laszlo
