On 03/16/2017 10:37 AM, Andrea Bolognani wrote:
> On Wed, 2017-03-15 at 17:49 -0400, Laine Stump wrote:
> [...]
>>> @@ -3,7 +3,7 @@
>>> <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid>
>>> <memory unit='KiB'>2097152</memory>
>>> <currentMemory unit='KiB'>2097152</currentMemory>
>>> - <vcpu placement='static' cpuset='0-1'>2</vcpu>
>>> + <vcpu placement='static'>2</vcpu>
>>
>> You made some other changes to the input XML beyond just the differences
>> in root ports. Mostly they're innocuous and easy to verify, but...
>
> [...]
>>> </controller>
>>> <controller type='pci' index='2' model='pcie-root-port'>
>>> <model name='ioh3420'/>
>>> - <target chassis='40' port='0x1a'/>
>>> + <target chassis='2' port='0x11'/>
>>
>> ...you removed the <target chassis='40' port='0x1a'/> from the input
>> file, but that was there for a reason - it was in the test to assure
>> that non-default values specified for chassis and port would be honored.
>> Please put that back in.
>
> I'll just leave it alone and add a comment about its purpose
> instead. Possibly change the name so it reflects the idea
> behind the test a little bit better.
It was the original test added when we added support for pcie-root-port,
and the idea behind the test was to test automatic and manual setting of
every associated attribute (i.e. it's not a special purpose test
intended just to test manual setting of the attributes in <target>, if
that's what you're implying.
>
>>> + DO_TEST("pcie-root-port-q35",
>>> QEMU_CAPS_DEVICE_IOH3420,
>>
>> If we were going to continue using ioh3420 for Q35, I would suggest that
>> you should add QEMU_CAPS_DEVICE_PCIE_ROOT_PORT here to verify that the
>> output still uses ioh3420.
>
> It's there, exactly for that purpose ;)
Yeah, I don't know how I missed that.
>
>> But as I said earlier I think we should
>> switch Q35 to using the generic root port too, so.... you *still* should
>> add that CAP, and change the expected output (and add a separate
>> "...-q35-old" test that doesn't have the cap for the generic root port)
>
> Yeah, I'll have just two new tests, one where the capability
> for the new controller is present and one where it's not.
>
> --
> Andrea Bolognani / Red Hat / Virtualization
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
