On 1/23/19 6:42 AM, Pavel Hrdina wrote:
> On Wed, Jan 23, 2019 at 10:08:31AM +0100, Andrea Bolognani wrote:
>> On Tue, 2019-01-22 at 12:46 -0500, John Ferlan wrote:
>>> Regenerate the output from the QEMU v3.0.0 tag using
>>>
>>> ./configure --target-list=x86_64-softmmu \
>>> --disable-strip \
>>> --disable-fdt \
>>> --disable-xen \
>>> --disable-werror \
>>> --enable-debug \
>>> --enable-system \
>>> --enable-user \
>>> --enable-linux-user \
>>> --with-pkgversion=v3.0.0
>>>
>>> NB: I had to fudge in the qemu-sev-capabilities output from
>>> commit d4005609f3 (not sure if there's a specific package
>>> to allow it just from build).
>>
>> While I very much appreciate the effort and agree it's something
>> that we should do, you can't just mix and match replies like that,
>> otherwise you'll end up with nonsensical results such as...
>
> In addition do we really need to change the CPU part of replies every
> time someone wants to update capabilities? It creates unnecessary
> noise and in some cases it might remove some advanced features because
> the capabilities were generated on Server CPU.
True. Of course results are dependent upon who runs them and on what
hardware they run them on and of course how the target QEMU was built.
There's nothing documented in qemucapabilitiestest.c that says, "build
qemu using" - just how to create the replies file using a built qemu. As
I found, before running ./configure a "yum builddep qemu" should also be
run to ensure whatever the checked out tag needs is installed.
So are the results a fault of a bad process or a bad test? Or just
something we have to accept that in order to have "recent results"? Are
there qemuxml2argvtest's that care about the CPU model? Obviously not -
it's just a string. What does care about the model is the SEV output.
Are there rules to what is found acceptable? Seems we've accepted odd
results in the past. Two examples, commit d4005609f3 which hand edited
in the SEV output and commit 6c50cef8 which tied the testing of
"launch-security-sev" to the "hand edited" caps for "2.12.0".
It seems that test is the only one where we've allowed a
DO_TEST_CAPS_VER without also a DO_TEST_CAPS_LATEST for a test. In
thinking about that now, it may be a wrong decision. IIRC the _VER test
was meant for when there were differences in output between QEMU caps
versions, but that the _LATEST should be provided for completeness.
Maybe the solution should be that qemucapsprobe lies and fills in the
query-sev-capabilities results via some voodoo and black magic. That
depends on whether we truly care about what qemuxml2argvtest is testing.
Considering that commit d4005609f3 was allowed to proceed even though:
"model-id": "Intel(R) Xeon(R) CPU E5-2630 v3 @ 2.40GHz",
was true, then what difference does it make if this series also hand
edits things? It was only after posting did I realize the extent of the
SEV testing alterations. So we only test that we create a proper sev
command line for qemu 2.12 capabilities. If something ever changed in
the short term future which necessitated more/different options, then
we'd be in the same situation.
>
> That's the case of the first patch in this series, you basically
> downgrade the CPU used to generate replies. I know, it requires more
> effort when preparing patches.
>
> Pavel
>
As noted above - it has nothing to do with downgrading a CPU - it has
everything to do with where one runs tests/qemucapsprobe. Generating the
patches shouldn't be that much effort, but the methodology used to
generate the *.replies files should be consistent and documented.
We may have to just accept there will be some output differences. As
long as those differences don't require alterations to some existing
qemuxml2argvtest output, then does it really matter? I'm more than
happy to let someone else generate the "more recent" .replies files. I
think it's something we should do each time QEMU reaches it's final
version build. As the 2.12 results show the difference between -rc0 and
the final tag were perhaps more extensive. At least nothing required
qemuxml2argvtest .args output changes.
FWIW: The 4.0 caps just pushed used the same model-id that I have:
"model-id": "Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz",
However, they were built w/o the "--disable-xen" which seems to be how
other caps files were built (based on the results I got). If I remove
the --disable-xen from my ./configure command above and regenerate
results based on the commit id Cole noted for 4.0, then I got very
similar, but not exactly similar results. The difference happened to be
that "vmx" was false for me, which reminded me I needed to enable nested
virt for my new laptop... With that adjustment, I got the same results.
Of course that also changes the results for what I've posted... But it
also shows perhaps another "difference" that can occur although we don't
care about that difference in our tests. Still, should that be mocked to
always return true? (probably not, unless we care that much about
masking certain differences). Should we mock the 'model-id' output to
be something else too?
>>
>> [...]
>>> @@ -19181,7 +19180,7 @@
>>> "kvm-pv-tlb-flush": true,
>>> "tbm": false,
>>> "wdt": false,
>>> - "model-id": "Intel(R) Xeon(R) CPU E3-1245 v5 @ 3.50GHz",
>>> + "model-id": "Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz",
>>
>> ... an Intel CPU...
>>
>> [...]
>>> @@ -20321,11 +20320,13 @@
>>> }
>>>
>>> {
>>> - "id": "libvirt-50",
>>> - "error": {
>>> - "class": "GenericError",
>>> - "desc": "SEV feature is not available"
>>> - }
>>> + "return": {
>>> + "reduced-phys-bits": 1,
>>> + "cbitpos": 47,
>>> + "cert-chain": "AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA",
>>> + "pdh": "AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA"
>>> + },
>>> + "id": "libvirt-50"
>>> }
>>
>> ... suddenly supporting SEV, which is an AMD-specific feature.
>>
>> The only sane way to deal with the situation is picking a few
>> QEMU versions and generate the corresponding capabilities on an
>> SEV-capable AMD host; then it's only a matter of making sure SEV
>> testing is performed against those specific QEMU versions rather
>> than random ones.
>>
Well, we could also go with the voodoo or black magic logic and mock the
results. Of course what we do depends more on what we care more about?
Absolutely valid results based on the CPU used to run qemucapsprobe or
generating a caps_VER.ARCH.replies file that allows us to test without
needing to have "x86_64.intel.replies" and "x86_64.amd.replies" files.
Since using DO_TEST*_LATEST has been the most recent push/request when
altering qemuxml2argvtest, then I think it seems we care more that the
replies files will have results that return valid data for everything
and not :
"error": {
"class": "GenericError",
"desc": "SEV feature is not available"
}
John
>> --
>> Andrea Bolognani / Red Hat / Virtualization
>>
>> --
>> libvir-list mailing list
>> libvir-list@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
