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

[Anthony Liguori]

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?  A full run
or just a restricted run?

>> 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.  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.

Regards,

Anthony Liguori

>
> --
> MST