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

[Michael S. Tsirkin]

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.

> 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