From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54768) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VW5RU-00037n-Kh for qemu-devel@nongnu.org; Tue, 15 Oct 2013 10:17:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VW5RK-00056d-CC for qemu-devel@nongnu.org; Tue, 15 Oct 2013 10:17:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:29189) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VW5RK-00056O-4D for qemu-devel@nongnu.org; Tue, 15 Oct 2013 10:17:38 -0400 Date: Tue, 15 Oct 2013 17:20:08 +0300 From: "Michael S. Tsirkin" Message-ID: <20131015142008.GA7613@redhat.com> References: <1381762577-12526-1-git-send-email-mst@redhat.com> <87r4bnlbj6.fsf@codemonkey.ws> <20131015052816.GC16331@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PULL 00/43] pci, pc, acpi fixes, enhancements List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Peter Maydell , Igor Mammedov , marcel.a@redhat.com, qemu-devel , Gerd Hoffmann 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 wrote: > > On Mon, Oct 14, 2013 at 03:42:37PM -0700, Anthony Liguori wrote: > >> "Michael S. Tsirkin" 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