[libvirt] [PATCH 2/3] qemu: Update x86_64 QEMU 3.0.0 capabilities

Wed Jan 23 16:08:02 UTC 2019

On Wed, Jan 23, 2019 at 08:59:52AM -0500, John Ferlan wrote:
> 
> 
> 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.

Obviously I know that you didn't downgrade your CPU that you used to
generate replies and it depends on the current CPU that is used while
generating new replies.

Yes we need to improve/document the whole process, in the mean time we
can manually avoid constantly changing the CPU model and adding extra
noise into the commits.  Usually if I need to update replies I replace
the files in repository with the generated ones and the I use
"git add -p path/to/file" to manually select only relevant changes in
order to skip the CPU changes.

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

I agree that we should (re)generate capabilities as soon as there is new
version of QEMU, nobody was questioning that :).

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

This is an interesting idea that we might consider doing, but I'm not
sure how it can affect our tests.  The replies are not directly used by
any other tests then qemucapabilitiestest but the XML files that we use
in qemucapabilitiestest are also used by domaincapstest, 
qemuargv2xmltest, qemucaps2xmltest, qemuxml2argvtest and it might affect
all of them.

Pavel

> >>
> >> [...]
> >>> @@ -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 at redhat.com
> >> https://www.redhat.com/mailman/listinfo/libvir-list
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190123/2d28de05/attachment-0001.sig>