Re: [Qemu-devel] [PATCH 00/11] build ACPI MADT for fw_cfg clients

[Laszlo Ersek]

(adding Kevin)

On 04/03/13 22:05, Anthony Liguori wrote:
> Laszlo Ersek <address@hidden> writes:
> 
>> On 03/21/13 00:23, Laszlo Ersek wrote:
>>> This series reworks the internals of the -acpitable command line option,
>>> and if that option is not specified, produces the APIC (MADT) table
>>> inside qemu, to be consumed over fw_cfg (alongside the DSDT).
>>
>>> Laszlo Ersek (11):
>>>   strip some whitespace
>>>   change element type from "char" to "unsigned char" in ACPI table data
>>>   acpi_table_add(): report fatal errors through an internal Error
>>>     object
>>>   qapi schema: add AcpiTableOptions
>>>   acpi_table_add(): accept QemuOpts and parse it with OptsVisitor
>>>   acpi_table_add(): extract and reimplement internals
>>>   like acpi_table_install(), acpi_table_add() should propagate Errors
>>>   extract/unify the constant 0xfee00000 as APIC_DEFAULT_ADDRESS
>>>   Introduce IO_APIC_DEFAULT_ADDRESS for 0xfec00000
>>>   pc_acpi_init(): don't bail as soon as failing to find default DSDT
>>>   i386/pc: build ACPI MADT for fw_cfg clients
>>
>> Any chance patches 01 to 09 could be considered? Esp. 06 which removes
>> an out-of-bounds access (an innocent-looking one, admittedly).
>>
>> I'm OK too if the series is dropped (patch 11 was the main motivation,
>> but the interface that it extends was deemed unsuitable going forward on
>> the seabios list). I'd just like to hear the maintainer with
>> jurisdiction say the NAK. ("Too expensive even to review for too little
>> gain" is a good reason.)
> 
> The whole thing looks pretty nice to me.

That's awesome, thank you very much!

>  I'll merge the full series in
> a day or so unless anyone objects.

For transparency's sake: Kevin, this is where you'd object to patch 11:
it adds an MADT to the existing fw_cfg blob, which, combined with an
older (=current) SeaBIOS, leads to a duplicated MADT; see also the blurb
in 00/11 which quotes that from
<http://www.seabios.org/pipermail/seabios/2013-March/005960.html>. (I'm
adding this paragraph because you can argue this better; I yet have to
go through your detailed design
<http://www.seabios.org/pipermail/seabios/2013-March/006020.html> once
more.)

Anyway, if you could tolerate such a duplicate MADT for a very short
time, I'd send a qemu patch that reworks pc_acpi_madt() (in 11/11), such
that it's final call is not made to acpi_table_install() -- ie.
appending the MADT to the existing fw_cfg ACPI blob --, but installs it
as a separate fw_cfg file as you're suggesting (/etc/acpi/APIC or so) --
the existing "-acpitable" switch would still benefit from 01-10, and I'd
salvage the meat of 11/11 as well. I'd then attempt to write a SeaBIOS
patch too for /etc/acpi/APIC. Would that be acceptable for you?

> This is an under maintained area of QEMU so it's very nice to see
> improvements!
> 
> Reviewed-by: Anthony Liguori <address@hidden>

Can't thank enough for your quick response, Anthony.

Laszlo