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

s390x: add support for s390x #990

Merged
merged 1 commit into from Dec 11, 2018

Conversation

Projects
None yet
7 participants
@alicefr
Copy link
Contributor

alicefr commented Dec 7, 2018

The PR adds the support for s390x.

In the case of CCW devices, the vhost-user devices are not supported.
See #659. An error message is thrown if they tried to be used.

Memory hotplug is not supported on s390 yet and an error message is thrown.

The VirtioNetPCI has been changed to VirtioNet. The generalization
allows to set the VirtioNet to the correct CCW device for s390x.

A lot of the elements in this PR have been taken from #667

Fixes: #666

Co-authored-by: Yash D Jain ydjainopensource@gmail.com
Signed-off-by: Alice Frosi <afrosi@de.ibm.>

@alicefr alicefr force-pushed the alicefr:s390x branch 2 times, most recently from 4592e59 to 0458cb5 Dec 7, 2018

@jodh-intel
Copy link
Contributor

jodh-intel left a comment

Very nice! Thanks @alicefr!

const testCPUInfoTemplate = `
vendor_id : IBM/S390
# processors : 4
bogomips per cpu: 20325.00

This comment has been minimized.

@jodh-intel

jodh-intel Dec 7, 2018

Contributor

Wow :)

Show resolved Hide resolved cli/kata-check_s390x_test.go Outdated
Show resolved Hide resolved cli/utils_s390x.go Outdated
func MaxQemuVCPUs() uint32 {
// Max number of virtual Cpu defined in qemu. See
// https://github.com/qemu/qemu/blob/80422b00196a7af4c6efb628fae0ad8b644e98af/target/s390x/cpu.h#L55
// #define S390_MAX_CPUS 248

This comment has been minimized.

@jodh-intel

jodh-intel Dec 7, 2018

Contributor

It's a shame this can't be queried by calling qemu (via govmm) directly. But clearly not a blocker as other architectures also hard-code a value.

Aside: For the future, I do wonder if we could push the hard-coded arch-specific values down into govmm rather than having them in the runtime though - wdyt @markdryan?

This comment has been minimized.

@alicefr

alicefr Dec 10, 2018

Author Contributor

I didn't find a way. For Linux you can find it in /sys/devices/system/cpu/kernel_max

This comment has been minimized.

@jodh-intel

jodh-intel Dec 10, 2018

Contributor

Sure - this was more a question for @markdryan ;)

@alicefr alicefr force-pushed the alicefr:s390x branch 2 times, most recently from d5f8484 to 5800002 Dec 10, 2018

@jodh-intel

This comment has been minimized.

Copy link
Contributor

jodh-intel commented Dec 10, 2018

@grahamwhaley

This comment has been minimized.

Copy link
Member

grahamwhaley commented Dec 10, 2018

/test

@jodh-intel

This comment has been minimized.

Copy link
Contributor

jodh-intel commented Dec 10, 2018

Travis is complaining due to a static analysis issue:

virtcontainers/qemu.go:907:9:warning: if block ends with a return statement, so drop this else and outdent its block (golint)
Show resolved Hide resolved virtcontainers/qemu.go Outdated

@alicefr alicefr force-pushed the alicefr:s390x branch from 5800002 to fdae96e Dec 10, 2018

@alicefr

This comment has been minimized.

Copy link
Contributor Author

alicefr commented Dec 10, 2018

@jodh-intel now the changes in virtcontainers/qemu.go are a little bit more, but it simplifies the code structure. I moved the switch out of the if statement. It was the same for both if and else statement and removed the else because the if statement has a return err

@jodh-intel

This comment has been minimized.

Copy link
Contributor

jodh-intel commented Dec 10, 2018

Thanks @alicefr.

@jodh-intel

This comment has been minimized.

Copy link
Contributor

jodh-intel commented Dec 10, 2018

/retest

Show resolved Hide resolved virtcontainers/qemu.go Outdated
Show resolved Hide resolved virtcontainers/qemu_s390x.go Outdated

@alicefr alicefr force-pushed the alicefr:s390x branch from fdae96e to 7ee4c1a Dec 10, 2018

@devimc

This comment has been minimized.

Copy link
Contributor

devimc commented Dec 10, 2018

thanks @alicefr

@devimc

This comment has been minimized.

Copy link
Contributor

devimc commented Dec 10, 2018

/test

@jcvenegas
Copy link
Contributor

jcvenegas left a comment

LGTM

Show resolved Hide resolved virtcontainers/qemu.go Outdated
@sboeuf

This comment has been minimized.

Copy link
Contributor

sboeuf commented Dec 11, 2018

Looks good!

@jodh-intel

This comment has been minimized.

Copy link
Contributor

jodh-intel commented Dec 11, 2018

@alicefr alicefr force-pushed the alicefr:s390x branch from 7ee4c1a to 411c2eb Dec 11, 2018

@caoruidong

This comment has been minimized.

Copy link
Contributor

caoruidong commented Dec 11, 2018

/test

@caoruidong
Copy link
Contributor

caoruidong left a comment

lgtm

Show resolved Hide resolved cli/kata-check_s390x.go Outdated
return err
}
if err := q.qmpMonitorCh.qmp.ExecuteNetdevDel(q.qmpMonitorCh.ctx, tap.Name); err != nil {
return err

This comment has been minimized.

@caoruidong

caoruidong Dec 11, 2018

Contributor

Thanks for refactoring!

s390x: add support for s390x
The PR adds the support for s390x.

In the case of CCW devices, the vhost-user devices are not supported.
See #659. An error message is thrown if they tried to be used.

Memory hotplug is not supported on s390 yet and an error message is thrown.

The VirtioNetPCI has been changed to VirtioNet. The generalization
allows to set the VirtioNet to the correct CCW device for s390x.

Fixes: #666

Co-authored-by: Yash D Jain ydjainopensource@gmail.com
Signed-off-by: Alice Frosi <afrosi@de.ibm.com>

@alicefr alicefr force-pushed the alicefr:s390x branch from 411c2eb to 6f83061 Dec 11, 2018

@alicefr

This comment has been minimized.

Copy link
Contributor Author

alicefr commented Dec 11, 2018

I introduced the error check for the getQemuMachine() and moved the devID := "virtio-" + tap.ID with the switch outside the if statement. I removed the model optional comment

@devimc

devimc approved these changes Dec 11, 2018

Copy link
Contributor

devimc left a comment

lgtm

@devimc

This comment has been minimized.

Copy link
Contributor

devimc commented Dec 11, 2018

/test

@devimc devimc merged commit 976f5b2 into kata-containers:master Dec 11, 2018

8 checks passed

code-review/pullapprove Approved by caoruidong, devimc, jodh-intel
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-centos-7-4 Build finished.
Details
jenkins-ci-cri-containerd Build finished.
Details
jenkins-ci-fedora Build finished.
Details
jenkins-ci-ubuntu-16-04 Build finished.
Details
jenkins-ci-ubuntu-16-04-initrd Build finished.
Details
jenkins-metrics-ubuntu-16-04 Build finished.
Details
@jodh-intel

This comment has been minimized.

Copy link
Contributor

jodh-intel commented Dec 11, 2018

\o/ Thanks again @alicefr! 😄

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