On Tue, Oct 15, 2013 at 07:21:34AM -0700, Anthony Liguori wrote: > On Tue, Oct 15, 2013 at 7:20 AM, Michael S. Tsirkin <m...@redhat.com> wrote: > > 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. > > This specific tree or some previous version of the series?
This specific tree + updated seabios. > A full run > or just a restricted run? All tests I normally use for PCI: install guest, migrate, virtio net+blk. If you want more just give me a list: last I looked full run has lots of known failures, that's one of the issues with autotest. I think that ACPI tables being exactly identical when used with seabios is also a convincing argument. > >> 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. > > I cannot reasonable revert a series like this before we cut GA. But we can *very* easily disable the new stuff from being exported to guests. Just tweak machine definitions in i386/pc. > We > would have to delay the release until everything was fixed. The > release is a month away and most of us will lose at least a week to > KVM Forum so our ducks need to be in a row here. > > Please put together a summary of the testing this series has gone > through. I still think there should be automated testing as part of > this but if the manual testing is sufficiently thorough I'll > reconsider for 1.7. OK this goes for bridge as well or just for the infrastructure? > Regards, > > Anthony Liguori > > > > > -- > > MST
