Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump to QEMU 5.2.0 #1349

Merged
merged 3 commits into from Mar 10, 2021
Merged

Bump to QEMU 5.2.0 #1349

merged 3 commits into from Mar 10, 2021

Conversation

wainersm
Copy link
Contributor

@wainersm wainersm commented Feb 2, 2021

This change the version of QEMU used in the tests and CI.

Not compiling with -O3 to avoid the warning:

Program python3 found: YES (/usr/bin/python3)
../meson.build:104: WARNING: Consider using the built-in optimization level instead of using "-O3".
../meson.build:108: WARNING: Consider using the built-in optimization level instead of using "-O3".

New and uneeded firmware files are removed, except for qboot.rom since it can be used with microvm
machine type.

Needs a patch which allows passing --disable-tools --enable-virtiofsd

Fixes: #1238

Signed-off-by: Wainer dos Santos Moschetta wainersm@redhat.com

@wainersm
Copy link
Contributor Author

wainersm commented Feb 2, 2021

This is just a initial work, I still have to decide some questions as you can see on the issue linked. Let me run the CI to see what happens. :)

@wainersm
Copy link
Contributor Author

wainersm commented Feb 2, 2021

/test

Copy link

@devimc devimc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @wainersm - I've left some comments and questions

tools/packaging/scripts/configure-hypervisor.sh Outdated Show resolved Hide resolved
tools/packaging/static-build/qemu/build-static-qemu.sh Outdated Show resolved Hide resolved
tools/packaging/scripts/configure-hypervisor.sh Outdated Show resolved Hide resolved
wainersm added a commit to wainersm/kata-containers that referenced this pull request Feb 4, 2021
The script split the QEMU and GCC version in major and minor versions
then use those values on conditionals. This is error prone, so instead
this change the script to use the bc program.

Fixes: kata-containers#1349
Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
@wainersm wainersm requested a review from a team as a code owner February 4, 2021 21:11
@wainersm
Copy link
Contributor Author

wainersm commented Feb 4, 2021

@devimc @jodh-intel guys, I just sent an updated version where I solely addressed your review. Thanks!

wainersm added a commit to wainersm/kata-containers that referenced this pull request Feb 4, 2021
The script split the QEMU and GCC version in major and minor versions
then use those values on conditionals. This is error prone, so instead
this change the script to use the bc program.

Fixes: kata-containers#1349
Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
wainersm added a commit to wainersm/kata-containers that referenced this pull request Feb 8, 2021
The script split the QEMU and GCC version in major and minor versions
then use those values on conditionals. This is error prone, so instead
this change the script to use the bc program.

Fixes: kata-containers#1349
Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
@wainersm
Copy link
Contributor Author

wainersm commented Feb 8, 2021

The new version of this PR fixes the problem with use of "x.y.z" with bc in the configure-hypervisor.sh script. Also it is added the patches which contain fixes for virtiofsd, as in #1360

wainersm added a commit to wainersm/kata-containers that referenced this pull request Feb 10, 2021
The script split the QEMU and GCC version in major and minor versions
then use those values on conditionals. This is error prone, so instead
this change the script to use the bc program.

Fixes: kata-containers#1349
Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
Copy link

@devimc devimc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @wainersm - one more nit 😅

tools/packaging/scripts/configure-hypervisor.sh Outdated Show resolved Hide resolved
@skaegi
Copy link
Contributor

skaegi commented Feb 17, 2021

We already have adopted QEMU 5.2.x in our own 1.x tree and this PR would be really helpful to us in terms of letting us move to 2.x. What is still left to do here? We're happy to test with it if this PR is mostly ready.

@wainersm
Copy link
Contributor Author

We already have adopted QEMU 5.2.x in our own 1.x tree and this PR would be really helpful to us in terms of letting us move to 2.x. What is still left to do here? We're happy to test with it if this PR is mostly ready.

Hi @skaegi , today I will send an updated version and I hope it will be the last for this PR. Then I will run the CI jobs. It would be nice if you could test on your use cases and report it here.

Another thing that should be worked out is the metrics CI since we don't want that this update introduce regressions on the metrics. For that I will have to catch up with @jcvenegas @devimc .

wainersm added a commit to wainersm/kata-containers that referenced this pull request Feb 18, 2021
The scripts/configure-hypervisor.sh split the QEMU and GCC version
in major and minor versions then use those values on shell conditionals
to compare versions. This is error prone, so instead this change the script
to use the `sort -V -C ` command for version comparisons.

Fixes: kata-containers#1349
Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
@wainersm
Copy link
Contributor Author

/test-debian

@wainersm
Copy link
Contributor Author

/test

@wainersm
Copy link
Contributor Author

/test-ubuntu

@Jakob-Naucke
Copy link
Member

Hi @wainersm, can we add this patch please? It (re-)enables virtio-9p-ccw, required for 9p on s390x.

@wainersm
Copy link
Contributor Author

Hi @Jakob-Naucke ,

Hi @wainersm, can we add this patch please? It (re-)enables virtio-9p-ccw, required for 9p on s390x.

The patch applies clearly on 5.2 but the build fails because the have_virtfs variable is not defined (see below). I am passing explicitly --enable-virtfs --enable-virtiofsd. So the patch will need backport, or we cherry-pick the changes that introduced the virtfs configuration bits. Could you please work on that?

Program ../scripts/block-coroutine-wrapper.py found: YES

../hw/s390x/meson.build:43:3: ERROR: Unknown variable "have_virtfs".

A full log can be found at /root/qemu/build/meson-logs/meson-log.txt

ERROR: meson setup failed

The scripts/configure-hypervisor.sh split the QEMU and GCC version
in major and minor versions then use those values on shell conditionals
to compare versions. This is error prone, so instead this change the script
to use the `sort -V -C ` command for version comparisons.

Fixes: kata-containers#1349
Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
@wainersm
Copy link
Contributor Author

wainersm commented Mar 1, 2021

/test

This change the version of QEMU used in the tests and CI.

The scripts/configure-hypervisor.sh was changed so that:
  - Passing the `--enable-virtiofsd` flag
  - Do not compiling with -O3 to avoid the warning:

    Program python3 found: YES (/usr/bin/python3)
    ../meson.build:104: WARNING: Consider using the built-in optimization level instead of using "-O3".
    ../meson.build:108: WARNING: Consider using the built-in optimization level instead of using "-O3".

The qemu.blacklist files was changed so that new and uneeded firmware files are removed from the
final tarball. Except for qboot.rom which is new but kept, since it can be used with microvm
machine type (in case we want to enable microvm in the future).

The patches which are applied on QEMU sources:
 - 0001-virtiofsd-Allow-to-build-it-without-the-tools.patch
   (Build fix for Meson - allows passing `--disable-tools --enable-virtiofsd`)
 - 0002-virtiofsd-extract-lo_do_open-from-lo_open.patch
   0003-virtiofsd-optionally-return-inode-pointer-from-lo_do.patch
   0004-virtiofsd-prevent-opening-of-special-files-CVE-2020-.patch
   0005-virtiofsd-Add-_llseek-to-the-seccomp-whitelist.patch
   0006-virtiofsd-Add-restart_syscall-to-the-seccomp-whiteli.patch
   (Security fixes for virtiofsd)
 - 0007-9p-removing-coroutines-of-9p-to-increase-the-I-O-per.patch
   (Performance improvement for 9p driver)
 - 0008-hw-s390x-fix-build-for-virtio-9p-ccw.patch
   (Build fix for virtio-9p-ccw machine type)

Fixes: kata-containers#1238

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
QEMU 5.2.0 needs ninja-build package installed on the build environment.

The default-configs were copied to $QEMU_SRC/default-configs but that does
take any effect, so instead it is now copied to $QEMU_SRC/default-configs/devices
and the configs for i386 were updated.

Also it had to change some arguments being passed to configure as Meson was failing
due inconsistent paths:

  ./meson.build:1:0: ERROR: The value of the 'libdir' option is '/usr/lib/qemu' which must be a subdir of the prefix '/snap/kata-containers/current/usr'.
  Note that if you pass a relative path, it is assumed to be a subdir of prefix.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
@wainersm
Copy link
Contributor Author

wainersm commented Mar 1, 2021

/test

@wainersm
Copy link
Contributor Author

wainersm commented Mar 2, 2021

The jenkins-ci-ARM-ubuntu-18-04 build failed because it could not download muscl for arm. So nothing to so with this change.

@wainersm wainersm dismissed jodh-intel’s stale review March 2, 2021 13:28

I addressed the concerns on latest version of this PR

Copy link
Member

@fidencio fidencio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks @wainersm!

I'd like to ask for @jcvenegas review here before having it merged.

@wainersm
Copy link
Contributor Author

wainersm commented Mar 4, 2021

One important note: This update will take effect for all architectures except for aarch64. According to versions.yaml, aarch64 has used QEMU 5.1 and I didn't change it. The reasons for not changing are:

  • I didn't know if the people interested on arm wanted that update
  • I didn't have hardware to test it and the arm CI job was broken (it seems fixed now)

I propose we merge this PR as is. If someone ask for aarch64 update then I can work on that but I will rely on the CI job for tests (so it should be working properly).

@GabyCT
Copy link
Contributor

GabyCT commented Mar 5, 2021

/test-arm

@egernst
Copy link
Member

egernst commented Mar 5, 2021

Actively reviewing -- will try to leave some feedback by end of day.

I don't love having so many patches to carry. If at all feasible, we can push the upstream folks to create a stable release?

@wainersm
Copy link
Contributor Author

wainersm commented Mar 8, 2021

@egernst ,

I don't love having so many patches to carry. If at all feasible, we can push the upstream folks to create a stable release?

Same here. And today I was inquiring the community on QEMU' irc channel about the process of stable releases. As you can see on the sniped below (note: only removed messages of people join/quit the channel) the process is somewhat ad-hoc. We can for sure ask for a new release with the CVE we care about.

Note that frequently people ask me "why not use the distro's QEMU?". I just sent an email to kata-dev because I don't know the rationale.

<wainersm> hi people! What's the cadence of the stable releases? Alongside the regular releases?
<danpb> wainersm: they're fairly arbitrary timeframe afaik
<wainersm> danpb, being up to the stable-branch maintainers to decide when?
<danpb> i assume they decide based on (a) when alot of stuff has accumulated  (b) when people request one is made for some important bug 
<pm215> as a practical observation, usually I think there is one .1 stable release which comes out broadly at the same time as the next .0 mainline release
<danpb> wainersm: is there a reason why you particularly care about the schedle ?
<wainersm> danpb, kata Containers uses upstream QEMU on its CI/tests (not sure, but some can use on production too). I'm working to update from 5.0 to 5.2 but I will need to carry CVE's patches. If we had a stable 5.2.1 release then I could just consume it. Since this situation can happen frequently, I'm interested on whether it had a regular cadence or not (for the sake of setting a process on kata side)
<pm215> note that "stable release" doesn't imply having patches for known CVE fixes
<danpb> wainersm: you should /not/ assume QEMU stable releases have CVE fixes
<danpb> QEMU essentially leaves this to downstream distros to solve
<pm215> if you want CVE fixes you need to either (a) use a distro QEMU or (b) monitor upstream QEMU/other CVE announcement locations and collate them yourself
<pm215> in particular, "we announced a CVE and there's a patch" does not trigger "we'll do a point release on the stable branch"
<danpb> wainersm: but that said, why does your CI environment need to have CVE fixes
<stsquad> wainersm: I'd use the distro release (for example Debian backports has a last stable release + fixes)
<stsquad> is my understanding correct that our (upstream) stable branches provide a starting point for the distro's final release?
<danpb> CI should be running in  self contained, throw away env generally, so what matters is security of the environment providing the CI env, not security of stuff under test
<wainersm> pm215, got it. and it makes sense
<wainersm> danpb, I don't have an answer for your question because the project had that approach before I joined. But I will ask the community and let you know :)
<danpb> wainersm: so you're saying that they currently run qemu 5.0 and backport CVE fixes for their CI env ?
<wainersm> stsquad, yep. I will bring that topic to the community. Don't know why it was chose to not use distro's QEMU.
<stsquad> the Linaro LKFT (kernel test) team have a similar requirement - they want a recent QEMU to run their tests in so they aren't pegged to very old versions in stable distros... but they don't want to track master (because having two moving targets is bad for CI)
<wainersm> danpb, yes, but just a few (4 or 5 patches) for virtiofsd
<wainersm> stsquad, yeah, track master wouldn't work for kata containers either (IMO)
<danpb> honestly for CI, i would always just track a distro
<danpb> there are bleeding edge distros if you need to be very close to latest, but using a stable distro is better for reliability IME
<wainersm> danpb, I will check if someone uses it on production-like environments 
<wainersm> hey danpb, stsquad, pm215, thanks for sharing those information!
<danpb> we really ought to mention this on the qemu.org download page
<stsquad> danpb: time for a flurry of parches for the website ;-)
<wainersm> danpb, https://qemu.readthedocs.io/en/latest/devel/stable-process.html has good information, maybe just put some complementary info? (IMHO it is not clear the cadence of the stable release- if any - that's why I asked)
<stsquad> the examples are a bit out of date on that so a refresh probably would help

@egernst
Copy link
Member

egernst commented Mar 8, 2021

Thanks @wainersm -- let's wait for more feedback on ML, then post summary here for reference (I agree with Julio's feedback).

wainersm added a commit to wainersm/kata-containers that referenced this pull request Mar 8, 2021
This contain to fixes for the virtiofsd on snap:
 * removed the "-/usr/libexec" so that virtiofsd is copied to prime
 * The configuration.toml expects virtiofsd in /usr/libexec/kata-qemu so it should be passed "kata-qemu"
   to configure_hypervisor.sh script and it wil configure to install the executable onto the right directory.

Fixes kata-containers#1238
Depends-on: github.com/kata-containers#1349
Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
@wainersm wainersm removed the request for review from dgibson March 10, 2021 12:56
@wainersm wainersm merged commit 6e49694 into kata-containers:main Mar 10, 2021
wainersm added a commit to wainersm/kata-containers that referenced this pull request Mar 10, 2021
This contain to fixes for the virtiofsd on snap:
 * removed the "-/usr/libexec" so that virtiofsd is copied to prime
 * The configuration.toml expects virtiofsd in /usr/libexec/kata-qemu so it should be passed "kata-qemu"
   to configure_hypervisor.sh script and it wil configure to install the executable onto the right directory.

Fixes kata-containers#1238
Depends-on: github.com/kata-containers#1349
Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
olagkasn pushed a commit to nubificus/kata-containers that referenced this pull request Mar 16, 2021
This contain to fixes for the virtiofsd on snap:
 * removed the "-/usr/libexec" so that virtiofsd is copied to prime
 * The configuration.toml expects virtiofsd in /usr/libexec/kata-qemu so it should be passed "kata-qemu"
   to configure_hypervisor.sh script and it wil configure to install the executable onto the right directory.

Fixes kata-containers#1238
Depends-on: github.com/kata-containers#1349
Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

qemu: Bump to version 5.2.0
10 participants