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

virtcontainers: Set ppc64le maxmem depending on qemu version #417

Merged
merged 1 commit into from
Jul 9, 2018
Merged

virtcontainers: Set ppc64le maxmem depending on qemu version #417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions virtcontainers/qemu.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ const qmpCapErrMsg = "Failed to negoatiate QMP capabilities"

const defaultConsole = "console.sock"

var qemuMajorVersion int
var qemuMinorVersion int
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we want to store these in one of the types, even though they are only being used for PPC64le atm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jodh-intel : Should we? I am not sure if its gonna get consumed by any other architectures atm.

Copy link

Choose a reason for hiding this comment

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

I would definitely include those as part of the qemu structure since this is an info about the hypervisor. And I agree with @jodh-intel that even if it's only used by ppc64, this is not a reason to define some global variable there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sboeuf : Okay, I shall add qemuMajorVersion and qemuMinorVersion into the qemu structure. However, I am unsure of what would be the most efficient way of exposing it in func (q *qemuPPC64le) memoryTopology. One way is to get qemu version info in func (q *qemu) init(sandbox *Sandbox) error and pass via newQemuArch resulting in change of signature of newQemuArch everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit fiddly, but we could add them to qemuArchBase like this:

Copy link

Choose a reason for hiding this comment

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

@nitkon oh yeah my bad, I forgot qemu structure was not the interface related to qemu_ppc64le. As @jodh-intel suggested, qemuArchBase should be the one.

@jodh-intel, I have taken a look at your suggestion but I thought we could simplify this by simply having those fields being called from qemu_ppc64le structure (no need for getters and setters). Isn't it possible since this structure inherits from qemuArchBase ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not quite sure what you're saying tbh. I'm hoping there is a simpler way though as I'm not particularly happy with what I came up with :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jodh-intel : So I was going through the implementation but noticed the following.
Printing q.qemuMajorVersion, q.qemuMinorVersion in "func (q qemuArchBase) getQEMUVersion() (major, minor int) {" shows me the following: level=info msg="DEBUG: versions: major: 0, minor: 0" arch=amd64 name=kata-runtime pid=6215 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jodh-intel : I am a bit confused by the reference implementation. We are setting qemuMajorVersion and qemuMinorVersion using one qemu instance and getting it with another qemu instance. (resulting it in 0,0 values).

IIUC, we need to set/get using the same qemu instance


// agnostic list of kernel parameters
var defaultKernelParameters = []Param{
{"panic", "1"},
Expand Down Expand Up @@ -509,6 +512,8 @@ func (q *qemu) waitSandbox(timeout int) error {
}

q.qmpMonitorCh.qmp = qmp
qemuMajorVersion = ver.Major
qemuMinorVersion = ver.Minor

q.Logger().WithFields(logrus.Fields{
"qmp-major-version": ver.Major,
Expand Down
16 changes: 12 additions & 4 deletions virtcontainers/qemu_ppc64le.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
govmmQemu "github.com/intel/govmm/qemu"
"github.com/kata-containers/runtime/virtcontainers/device/drivers"
"github.com/kata-containers/runtime/virtcontainers/utils"
"github.com/sirupsen/logrus"
)

type qemuPPC64le struct {
Expand Down Expand Up @@ -54,6 +55,11 @@ var supportedQemuMachines = []govmmQemu.Machine{
},
}

// Logger returns a logrus logger appropriate for logging qemu messages
func (q *qemuPPC64le) Logger() *logrus.Entry {
return virtLog.WithField("subsystem", "qemu")
}

// MaxQemuVCPUs returns the maximum number of vCPUs supported
func MaxQemuVCPUs() uint32 {
return uint32(128)
Expand Down Expand Up @@ -105,12 +111,14 @@ func (q *qemuPPC64le) cpuModel() string {

func (q *qemuPPC64le) memoryTopology(memoryMb, hostMemoryMb uint64) govmmQemu.Memory {

if hostMemoryMb > defaultMemMaxPPC64le {
hostMemoryMb = defaultMemMaxPPC64le
} else {
// align hostMemoryMb to 256MB multiples
if qemuMajorVersion >= 2 && qemuMinorVersion >= 10 {
q.Logger().Debug("Aligning maxmem to multiples of 256MB. Assumption: Kernel Version >= 4.11")
hostMemoryMb -= (hostMemoryMb % 256)
} else {
q.Logger().Debug("Restricting maxmem to 32GB as Qemu Version < 2.10, Assumption: Kernel Version >= 4.11")
hostMemoryMb = defaultMemMaxPPC64le
}

return genericMemoryTopology(memoryMb, hostMemoryMb)
}

Expand Down