Re: [Qemu-devel] [PATCH] qemu: piix: PCI bridge ACPI hotplug support

[Anthony Liguori <anthony@codemonkey.ws>]

Laszlo Ersek <lersek@redhat.com> writes:

> On 06/10/13 20:58, Anthony Liguori wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>>> This adds support for device hotplug behind
>>> pci bridges. Bridge devices themselves need
>>> to be pre-configured on qemu command line.
>>>
>>> One of the goals of this project was to
>>> demonstrate the advantages of generating
>>> ACPI tables in QEMU.
>>> In my opinion, it does this successfully.
>> 
>> Since you've gone out of your way to make this difficult to actually
>> review...
>
> I haven't tried to review the patch yet, but why would you say such a
> thing? Michael wants to convince you. There's no way he could sneak past
> you in this patch. You surely won't say "it's too hard to review, I'll
> yield", and he's obviously aware.

The patch is very large and depends on another large series.  It's not
split up as a logical series of changes and more importantly doesn't
solve significant problems (like live migration).

It makes it very difficult to properly evaluate the approach.  But even
taking those sort cuts, it looks like a lot of code in QEMU to avoid
what appears to be a significantly smaller amount of code in SeaBIOS.

There isn't deep dependency on information only available in QEMU.
AFAICT the only advantage of being in QEMU is being able to use a couple
data types from glib.

> [snip]
>
>>>        * No need for yet another interface to bios to detect qemu version
>>>          to check if it's safe to activate new code,
>>>          and to ship multiple table versions:
>> 
>> We only care about supporting one version of SeaBIOS.  SeaBIOS should
>> only care about supporting one version of QEMU.  We're not asking it to
>> support multiple versions.
>
> Kevin requires cross-compat between {old, new} qemu and {old, new} SeaBIOS:
>
> http://thread.gmane.org/gmane.comp.emulators.qemu/202005/focus=202072
>
>>>          we generated code so we know this is new qemu
>>>        * Use of glib's GArray makes it much easier to build
>>>          up tables in code without need for iasl and code patching
>> 
>> Adding a dynamic array to SeaBIOS isn't rocket science.
>
> Please take a look at my series for OVMF that adds basic support for
> SMBIOS tables. It took me three days of basically uninterrupted coding
> and two approaches to throw away until I got something submittable (with
> default tables for only type 0 and type 1).
>
> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/3037
>
> I don't want to invite accusations like "this is a strawman argument,
> noone was speaking about SMBIOS", but the selective, dynamic patching is
> somewhat similar between AML and SMBIOS in any given boot firmware. You
> got a big bunch of named offsets that must be mangled individually.
>
> Allowing the firmware to only install blobs verbatim, or maybe patch
> them but only in a completely programmatic manner (ie. without specific
> field names & offsets in the firmware, which I did try for SMBIOS in
> OVMF, and failed, (*)), would be a big help.
>
> ((*) because the fw_cfg format of individual SMBIOS fields doesn't
> distinguish formatted fields from strings in the unformatted area)

I don't understand this piece.

Other than TianoCore being a weird environment, what made this more
difficult than it is to generate the tables in QEMU?

Regards,

Anthony Liguori

>
> Not rocket science, just churn and busywork.
>
> [snip]
>
>> So TL;DR, you break a bunch of stuff and introduce a mess of code.  It
>> would be less code and wouldn't break anything to add this logic to
>> SeaBIOS.
>> 
>> How is this even a discussion?
>
> Well it isn't for me any more; count me out. I'll go with the flow.
>
> Cheers,
> Laszlo