On 03/28/14 04:43, Jordan Justen wrote:
> On Thu, Mar 27, 2014 at 2:00 PM, Laszlo Ersek <[email protected]> wrote:
>> On 03/27/14 19:05, Jordan Justen wrote:
>>> On Tue, Mar 25, 2014 at 4:58 PM, Laszlo Ersek <[email protected]> wrote:
>>>> Qemu v1.7.0 features an ACPI linker/loader interface, available over
>>>> fw_cfg, written by Michael Tsirkin.
>>>
>>> Can we add something like:
>>> (See 1.7.0 compatibility information below.)
>>
>> Yep.
>>
>>
>>>> - The patch causes a regression in PCI resource allocation for some guest
>>>> RAM sizes (eg. 2560MB) when OVMF runs on qemu v1.7.0 precisely, and the
>>>> "pc-i440fx-1.7" (ie. default) machine type is selected. This bug has
>>>> been fixed in qemu v1.7.1 (commit 03bc4f6 -- "piix: fix 32bit pci
>>>> hole").
>>>
>>> Doesn't this cause a hang in certain situations? Could you more
>>> clearly indicate the symptom seen on 1.7.0?
>>
>> I hope this one's clear enough :) :
>>
>> http://thread.gmane.org/gmane.comp.emulators.qemu/257366/focus=257529
>>
>> Dependent on guest, it may cause a nonfunctional display (blank screen),
>> but (as linked above) some Linux guests like to crash too.
>
> So, that's what I'd like to make clear in the commit message:
> * After this change, using QEMU 1.7.0 with OVMF might cause
> your OS boot to crash. Don't use QEMU 1.7.0!
>
> Right now it looks like:
> * Hey we support something added in QEMU 1.7.0! Oh, yeah there's
> some kind of regression related to QEMU 1.7.0.
>
You are right. I'll probably remove the reference to 1.7.0 from the
beginning of the commit message and replace it with one to the compat
info block. And I'll make the 1.7.0 part down there sound more
threatening ^W honest.
>>> Depending on the symptom, we might want to update the README.
>>
>> That's a good idea. Can you suggest a wording? I can include it as a
>> separate patch with your S-o-b.
>
> I'll try to get something to you, but we probably can add it sometime
> after this patch.
That would be very convenient to me, thank you.
[snip]
>>>> + if (Processed == TablesFileSize || Status == EFI_PROTOCOL_ERROR) {
>>>
>>> PROTOCOL_ERROR seems odd here. Why does this situation get turned into
>>> 'success'?
>>>
>>> Maybe just set Processed = TablesFileSize (with a comment) up above
>>> before the break?
>>
>> Well this is kinda messy. The fw_cfg file in question is padded with a
>> bunch of NULs. The reason being something about forward compat for VM
>> migration -- the fw_cfg file is also mapped as a ROM, and optimally (for
>> migratability) its size shouldn't grow even if the ACPI payload grows.
>>
>> Whatever the reason for NUL-padding, it means that when parsing the
>> fw_cfg file, at one point the probe probes into the trailing set of
>> zeros, and catches that as invalid table header, with the
>>
>> (Probe->Length < sizeof *Probe)
>>
>> sub-condition:
>>
>> InstallQemuLinkedTables: invalid ACPI table header at offset 0x1B38
>>
>> It really means EOF, but it is undistinguishable from genuinely
>> nonsensical blob contents. (Just because Probe->Length ends up zero it
>> doesn't imply it's not the result of some garbled qemu code; so I don't
>> feel convenient treating Probe->Length==0 as a clear-cut EOF situation.)
>>
>> Hence, undetectable EOF and data corruption can't be really told apart.
>
> You could check that the remaining data is all zero assuming QEMU is
> consistent about this like you mentioned above.
OK. And...
>
>> If I reject EFI_PROTOCOL_ERROR, then in (current) practice the parsing
>> will always fail, because right now there's a quite sizeable trailing
>> portion of NULs.
>>
>> If you have a suggestion I'm glad to listen.
>
> How about up above doing:
> //
> // QEMU adds some junk data to fw_cfg file etc/acpi/tables.
> //
> // We just skip the remaining data once we encounter this.
> //
> Processed = TablesFileSize;
> break;
... how about I keep the EFI_PROTOCOL_ERROR, and in general, the loop
body intact (except error messages on EFI_PROTOCOL_ERROR -- I'd delay
those). But, after the loop, where I have now
if (Processed == TablesFileSize || Status == EFI_PROTOCOL_ERROR) {
I'd add special treatment for EFI_PROTOCOL_ERROR: "Processed" points to
where we found the problem, so I could check the remaining data for
zeros-only *there*, after the loop. Because I break out with
EFI_PROTOCOL_ERROR in two spots from the loop, and this way I could
concentrate the check-for-zeros-only after the loop.
Thanks
Laszlo
------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
