Re: [Qemu-devel] [PULL 00/43] pci, pc, acpi fixes, enhancements

[Michael S. Tsirkin]

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 <address@hidden> 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 <address@hidden> 
> >> wrote:
> >> > On Mon, Oct 14, 2013 at 03:42:37PM -0700, Anthony Liguori wrote:
> >> >> "Michael S. Tsirkin" <address@hidden> 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