On Tue, Oct 15, 2013 at 06:51:30AM -0700, Anthony Liguori wrote: > On Mon, Oct 14, 2013 at 10:28 PM, Michael S. Tsirkin <m...@redhat.com> wrote: > > On Mon, Oct 14, 2013 at 03:42:37PM -0700, Anthony Liguori wrote: > >> "Michael S. Tsirkin" <m...@redhat.com> writes: > >> > >> > Anthony, I know you wanted to review some of the patches, > >> > since you didn't respond either all's well or you > >> > could not find the time. > >> > I think we are better off merging them for 1.7 and then - worst case, > >> > if major issues surface - disabling the functionality at the last minute > >> > than delaying the merge even more. > >> > >> There is no way I'll pull this for 1.7. Changes like this aren't going > >> to get merged at the last minute. > > > > Last minute? This has been on list for months. > > It doesn't matter how long the patches have been on the list. We have > a very short testing cycle for releases. > > This pull request lacks any automated testing. Something like this > really should come with at least some qtest validation that we are > still generating the right ACPI tables but certainly could have > simpler unit tests too.
It did go through autotest testing though. > There is no statement about what manual testing you actually did. Manually I loaded tables and verified that they match the bios bit for bit except pointer values. > Have you run kvm autotest? Have you tested a variety of Windows > guests? Yes, both. > The pull request has a patch with a binary diff and a comment of: > > "update generated file, not sure what changed" > > And that didn't concern you prior to sending the pull request? Sorry, I forgot to update the description. V2 has it right: IASL sticks its own version when it builds tables, this is what changed. > This series is not ready to merge. Because a single commit message was out of date? > >> A good chunk of the series lacks > >> any Reviewed-bys including the actual hotplug behind a pci bridge bits > >> which is the whole point of the series. > > > > It isn't. The point is getting ACPI out of seabios. > > OK what if I drop the bridge hotplug part? > > How does that address the above? It addresses the issues you have raised which was with the bridge. > >> This is a huge series and I still am not convinced this is the right > >> path forward. The alternative to this series is a small set of changes > >> to SeaBIOS to support PCI bridge hotplug, no? > > > > No, we also get alternative firmwares working correctly with QEMU. > > > >> Or 10k SLOC of code into QEMU that includes breaking migration > >> compatibility. > > > > AFAIK it doesn't break migration compatibility. > > >From 41/43: > > "The interface is actually backwards-compatible with > existing PIIX4 ACPI (though not migration compatible)." > > And does "AFAIK" translate to, "I have tested migration from new and > old and old and new with this series"? I suspect the answer is no. > > Regards, > > Anthony Liguori But the code to handle it is there, at least. I will test it but I think minor fixes like this can go in after soft freeze. -- MST
