This repository has been archived by the owner on May 12, 2021. It is now read-only.

ci: refine qemu-options for arm64 #94

Merged

jodh-intel merged 1 commit into kata-containers:master from Pennyzct:qemu

Jul 26, 2018

Contributor

Pennyzct commented Jul 10, 2018

a few qemu options generated by configure-hypervisor.sh weren't suitable for arm64, leading compilation err. lib-configure-hypervisor-aarch64.sh will create specific qemu options for arm64.

Fixes: #92

Signed-off-by: Penny Zheng penny.zheng@arm.com
Signed-off-by: Wei Chen Wei.Chen@arm.com

Contributor Author

Pennyzct commented Jul 10, 2018

Pennyzct mentioned this pull request

ci: support install_qemu in aarch64 kata-containers/tests#487

Merged

Member

jcvenegas commented Jul 11, 2018

Hey @Pennyzct the changes looks good, I wonder do you have the list of options that make conflict with aarch64 I see some options duplicated with the new config file I wonder if we could have a function to call the minimal set of options needed for kata. And then in the architecture specific function add the only ones that are different.

Contributor Author

Pennyzct commented Jul 11, 2018

Hi~ @jcvenegas the majority of the qemu options are same with x86_64, only a little would be different. here are the difference:

1. --disable-fdt 
2. --disable-tcg --disable-debug-tcg --disable-tcg-interpreter
3. --disable-zen
4. --target-list=aarch64-softmmu
5. not -fPIE, but -fPIC
6. remove -pie

At first, I was considering the minimal set of qemu options for all arch, but I really couldn't decide what's the common in ppc64le😭.

Member

jcvenegas commented Jul 11, 2018

@Pennyzct thanks for explain what is different IMO because it is an small about of differences I would prefer to only add a few checks and enabled and disable options needed by the achitecture. If we see the list of difference is bigger lets change it. @grahamwhaley @sboeuf @egernst @bergwolf @jodh-intel any comment about it ?

devimc commented Jul 17, 2018 •

edited

Loading

would be nice to see this PR landing, since I need it to ship snap images for ARM64. see #99
@Pennyzct any update on this?

Contributor Author

Pennyzct commented Jul 18, 2018

sorry for the delay @devimc. since getting no further comments, i will update asap following @jcvenegas's suggestion.😊

Pennyzct force-pushed the qemu branch from 0338462 to 1f16565 Compare

July 20, 2018 03:05

Contributor Author

Pennyzct commented Jul 20, 2018

@jcvenegas sorry for the delay, ptal.😊

devimc reviewed

View reviewed changes

scripts/configure-hypervisor.sh Outdated

    
            @@ -424,14 +405,69 @@ main()
          
              	unset _qemu_ldflags

              	# Where to install qemu libraries

              	[ "$arch" = x86_64 ] && qemu_options+=(arch:--libdir=/usr/lib64/${hypervisor})

              	[ "$arch" == "x86_64" ] || [ "$arch" == "aarch64" ] || [ "$arch" == "ppc64le" ] && qemu_options+=(arch:--libdir=/usr/lib64/${hypervisor})

devimc Jul 20, 2018

qemu_options+=(arch:--libdir=/usr/lib64/${hypervisor}) is the same for all architectures, please remove [ "$arch" == "x86_64" ] || [ "$arch" == "aarch64" ] || [ "$arch" == "ppc64le" ] &&

Contributor Author

Pennyzct Jul 23, 2018

ok. thanks.

devimc reviewed

View reviewed changes

scripts/configure-hypervisor.sh Outdated

    
            @@ -370,7 +343,7 @@ main()
          
              	# Other options

              	# 64-bit only

              	[ "$arch" = x86_64 ] && qemu_options+=(arch:"--target-list=${arch}-softmmu")

              	[ "$arch" == "x86_64" ] || [ "$arch" == "aarch64" ] || [ "$arch" == "ppc64le" ] && qemu_options+=(arch:"--target-list=${arch}-softmmu")

devimc Jul 20, 2018

please remove [ "$arch" == "x86_64" ] || [ "$arch" == "aarch64" ] || [ "$arch" == "ppc64le" ] &&

Contributor Author

Pennyzct Jul 23, 2018

ok. thanks

devimc mentioned this pull request

packaging: Get packaging for ppc64le #102

Merged

devimc commented Jul 20, 2018

@Pennyzct your PR adds support for arm64 and ppc64, please update the tittle and the commit message

devimc reviewed

View reviewed changes

scripts/configure-hypervisor.sh Outdated

@@ @@ -277,7 +229,11 @@ main() @@
               	qemu_options+=(size:--disable-vnc-sasl)
               	# Disable unused filesystem support
-              	qemu_options+=(size:--disable-fdt)
+              	case "$arch" in
+              		aarch64) ;;

devimc Jul 20, 2018

from #102 (comment)

please use following:

[ "$arch" == x86_64 ] && qemu_options+=(size:--disable-fdt)

devimc reviewed

View reviewed changes

scripts/configure-hypervisor.sh

+              			;;
+                              ppc64le)
+              			qemu_options+=(size:--disable-debug-tcg)
+              			qemu_options+=(size:--disable-tcg-interpreter)

devimc Jul 20, 2018

@nitkon please take a look

devimc suggested changes

View reviewed changes

scripts/configure-hypervisor.sh Outdated

@@ @@ -15,6 +15,23 @@ @@
               #---------------------------------------------------------------------
               script_name=${0##*/}
+              hypervisor_dir=${0%/*}

devimc Jul 20, 2018

useless variable

Contributor Author

Pennyzct Jul 23, 2018

removed, thanks.

Pennyzct force-pushed the qemu branch from 1f16565 to 96512e0 Compare

July 23, 2018 02:31

Contributor Author

Pennyzct commented Jul 23, 2018

ptal, thanks. 😊
@devimc @nitkon

Pennyzct changed the title ~~ci: refine qemu-options for arm64~~ ci: refine qemu-options for arm64 and ppc64le

devimc approved these changes

View reviewed changes

devimc left a comment

lgtm

Contributor

jodh-intel commented Jul 24, 2018 •

edited by amshinde

Loading

lgtm but let's wait for feedback from @nitkon.

jodh-intel added the do-not-merge label

Contributor

jodh-intel commented Jul 24, 2018

Added dnm label until we have a ppc64le review.

Contributor

nitkon commented Jul 24, 2018

Okay. So applying this patch and building on ppc64le results in the following:

Step 11/17 : RUN git clone  https://github.com/qemu/keycodemapdb.git ui/keycodemapdb
 ---> Using cache
 ---> 6551ec46b234
Step 12/17 : ADD configure-hypervisor.sh /root/configure-hypervisor.sh
 ---> Using cache
 ---> 81e13306a980
Step 13/17 : RUN if [ "$goarch" = "amd64" ]; then /root/configure-hypervisor.sh -s kata-qemu | xargs ./configure --prefix=/opt/kata --with-pkgversion=kata-static; else ./configure --prefix=/opt/kata --with-pkgversion=kata-static ; fi
 ---> Running in a356cd7cb9fa
OCI runtime create failed: rpc error: code = Unavailable desc = transport is closing: unknown
.../src/github.com/nitkon/packaging/static-build/qemu/Makefile:5: recipe for target 'build' failed
make[2]: *** [build] Error 1
make[2]: Leaving directory '.../src/github.com/nitkon/packaging'
...../src/github.com/nitkon/packaging/.ci/../Makefile:30: recipe for target 'test-static-build' failed
make[1]: *** [test-static-build] Error 2
make[1]: Leaving directory '..../src/github.com/nitkon/packaging'
Makefile:23: recipe for target 'test' failed
make: *** [test] Error 2

devimc commented Jul 24, 2018

@nitkon I think this error is because of in your test you are not using configure-hypervisor.sh

Step 13/17

if [ "$goarch" = "amd64" ]; then 
    /root/configure-hypervisor.sh -s kata-qemu | xargs ./configure --prefix=/opt/kata --with-pkgversion=kata-static;
else
    ./configure --prefix=/opt/kata --with-pkgversion=kata-static ;
fi

and seems like you have some problems in your environment

OCI runtime create failed: rpc error: code = Unavailable desc = transport is closing: unknown

Contributor

nitkon commented Jul 24, 2018 •

edited

Loading

@devimc : Oops u were right. I had git cloned by mistake my local git branch instead of kata containers packaging master branch. I see the following error:
`Step 10/16 : RUN git clone https://github.com/qemu/keycodemapdb.git ui/keycodemapdb
---> Using cache
---> 1516741ae214
Step 11/16 : ADD configure-hypervisor.sh /root/configure-hypervisor.sh
---> 4be4347928ce
Step 12/16 : RUN /root/configure-hypervisor.sh -s kata-qemu | xargs ./configure --prefix=/opt/kata --with-pkgversion=kata-static
---> Running in 230f5fbc92f1
./configure: --disable-uuid is obsolete, UUID support is always built

ERROR: Unknown target name 'ppc64le-softmmu'

The command '/bin/sh -c /root/configure-hypervisor.sh -s kata-qemu | xargs ./configure --prefix=/opt/kata --with-pkgversion=kata-static' returned a non-zero code: 123
`
@Pennyzct

U need to use --target-list=ppc64-softmmu and --disable-uuid is not needed for ppc64le. Check my commnets here: #102

However post those 2 fixes, i still see a string of errors (which I need to look into more): https://paste.fedoraproject.org/paste/-bAEtTzzkVebmrOqe~66Hw

Contributor Author

Pennyzct commented Jul 25, 2018 •

edited

Loading

for now, when encountering difference in arm, I left ppc64le the same options as with amd64.
I'm really not familiar with ppc64le, then maybe I should reroll the commit title back to refine qemu-options for arm64, and maybe @nitkon can fix it later .
Is that okay with you? @jodh-intel @devimc 😊

Contributor

jodh-intel commented Jul 25, 2018

Sounds reasonable to me - any thoughts @nitkon?

Contributor

nitkon commented Jul 25, 2018

@jodh-intel: I am ok with it.


          ci: refine qemu-options for arm64

f275127

a few qemu options generated by configure-hypervisor.sh were only
suitable for amd64, leading compilation err in aarch64.

Fixes: kata-containers#92

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Wei Chen <Wei.Chen@arm.com>

Pennyzct force-pushed the qemu branch from 96512e0 to f275127 Compare

July 25, 2018 09:49

Pennyzct changed the title ~~ci: refine qemu-options for arm64 and ppc64le~~ ci: refine qemu-options for arm64

Contributor

nitkon commented Jul 25, 2018

@jodh-intel @Pennyzct : I figured out why qemu static build was failing on ppc64le. But since @Pennyzct has already roll backed the changes , I can send a PR for it.
Else if @Pennyzct wants to update his PR then he can
use --target-list=ppc64-softmmu and --disable-uuid, --disable-fdt and --disable-tcg is not needed for ppc64le

Contributor

nitkon commented Jul 26, 2018

@jodh-intel @devimc : Since this has 2 lgtms already, I am ok to send a new PR. You may go ahead and merge. Thnx!

jodh-intel removed the do-not-merge label

Contributor

jodh-intel commented Jul 26, 2018

Thanks @nitkon.

jodh-intel merged commit 5d2a95b into kata-containers:master

jcvenegas pushed a commit that referenced this pull request


          rootfs: copy kernel modules to correct location

fd8d9bd

Commit b8f1a68 ("rootfs: Simplify
code") introduced a variable called destdir but accidentally used
dest_dir with cp(1) instead.  This causes kernel modules to be copied to
the wrong location.

Rename the variable to dest_dir to be consistent with module_dir and
rootfs_dir variables used in this function.

Fixes: #94
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet