Skip to content
This repository has been archived by the owner on Apr 3, 2018. It is now read-only.

hypervisor: add hot plugging support for Qemu Q35 #456

Merged
merged 1 commit into from
Nov 14, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,9 @@ func TestStatusPodSuccessfulStateReady(t *testing.T) {
KernelPath: filepath.Join(testDir, testKernel),
ImagePath: filepath.Join(testDir, testImage),
HypervisorPath: filepath.Join(testDir, testHypervisor),
DefaultVCPUs: defaultVCPUs,
DefaultMemSz: defaultMemSzMiB,
DefaultBridges: defaultBridges,
}

expectedStatus := PodStatus{
Expand Down Expand Up @@ -648,6 +651,9 @@ func TestStatusPodSuccessfulStateRunning(t *testing.T) {
KernelPath: filepath.Join(testDir, testKernel),
ImagePath: filepath.Join(testDir, testImage),
HypervisorPath: filepath.Join(testDir, testHypervisor),
DefaultVCPUs: defaultVCPUs,
DefaultMemSz: defaultMemSzMiB,
DefaultBridges: defaultBridges,
}

expectedStatus := PodStatus{
Expand Down
104 changes: 104 additions & 0 deletions bridge.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
//
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please fix errors in your commit message:

hotplug: Implement Bridge structure

Bridge represents a PCI or PCIE bridge and can be used
to hot plug PCI devices for pc and q35 machine types.

Signed-off-by: Julio Montes <julio.montes@intel.com>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed, thanks

Copy link
Collaborator

Choose a reason for hiding this comment

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

The title of the commit message is still wrong.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ahh ok s/Implements/Implement/
done, thanks

// Copyright (c) 2017 Intel Corporation
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

package virtcontainers

import "fmt"

type bridgeType string

const (
pciBridge bridgeType = "pci"
pcieBridge = "pcie"
)

const pciBridgeMaxCapacity = 30
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Where does this limit come from?
  • Is there any way to avoid hard-coding this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Where does this limit come from?

Qemu's documentation is not very good, I found this [1], but I'm not able to hot plug 32 devices,
I guess because of SHPC is enabled.

Is there any way to avoid hard-coding this?

I don't think so

[1] https://github.com/qemu/qemu/blob/master/docs/pcie.txt#L76

Copy link
Contributor

Choose a reason for hiding this comment

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

Having a peek at: https://en.wikipedia.org/wiki/PCI_configuration_space
looks like the per-bus device space is defined by a 5-bit field.
Maybe 0 and 0x1F are reserved - leaving 30 ? Just a guess. We could add a note saying this is part of the PCI spec I guess etc.


// Bridge is a bridge where devices can be hot plugged
type Bridge struct {
// Address contains information about devices plugged and its address in the bridge
Address map[uint32]string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct me if I am wrong but no need for a very specific uint32 type here since that's only an index, right ?
uint should be okay. But on the other hand, if you really think about saving space, then you might want to go with uint8 which is gonna be enough for 30 possible entries.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pcie has more capacity, for this reason I'd like to keep uint32


// Type is the type of the bridge (pci, pcie, etc)
Type bridgeType

//ID is used to identify the bridge in the hypervisor
ID string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we also add the chassis information here.

}

// NewBridges creates n new pci(e) bridges depending of the machine type
func NewBridges(n uint32, machine string) []Bridge {
var bridges []Bridge
var bt bridgeType

switch machine {
case QemuQ35:
// currently only pci bridges are supported
// qemu-2.10 will introduce pcie bridges
fallthrough
case QemuPC:
bt = pciBridge
default:
return nil
}

for i := uint32(0); i < n; i++ {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Once again, if we really want to limit the memory, then let's use uint8.

bridges = append(bridges, Bridge{
Type: bt,
ID: fmt.Sprintf("%s-bridge-%d", bt, i),
Address: make(map[uint32]string),
})
}

return bridges
}

// addDevice on success adds the device ID to the bridge and return the address
// where the device was added, otherwise an error is returned
func (b *Bridge) addDevice(ID string) (uint32, error) {
var addr uint32
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's more a key than an address.


// looking for the first available address
for i := uint32(1); i <= pciBridgeMaxCapacity; i++ {
if _, ok := b.Address[i]; !ok {
addr = i
break
}
}

if addr == 0 {
return 0, fmt.Errorf("Unable to hot plug device on bridge: there are not empty slots")
}

// save address and device
b.Address[addr] = ID
return addr, nil
}

// removeDevice on success removes the device ID from the bridge and return nil,
// otherwise an error is returned
func (b *Bridge) removeDevice(ID string) error {
// check if the device was hot plugged in the bridge
for addr, devID := range b.Address {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's more a key than an address. Let's call it key.

if devID == ID {
// free address to re-use the same slot with other devices
delete(b.Address, addr)
return nil
}
}

return fmt.Errorf("Unable to hot unplug device %s: not present on bridge", ID)
}
65 changes: 65 additions & 0 deletions bridge_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
//
// Copyright (c) 2017 Intel Corporation
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

package virtcontainers

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestNewBridges(t *testing.T) {
assert := assert.New(t)
var countBridges uint32 = 1

bridges := NewBridges(countBridges, "")
assert.Nil(bridges)

bridges = NewBridges(countBridges, QemuQ35)
assert.Len(bridges, int(countBridges))

b := bridges[0]
assert.NotEmpty(b.ID)
assert.NotNil(b.Address)
}

func TestAddRemoveDevice(t *testing.T) {
assert := assert.New(t)
var countBridges uint32 = 1

// create a bridge
bridges := NewBridges(countBridges, "")
assert.Nil(bridges)
bridges = NewBridges(countBridges, QemuQ35)
assert.Len(bridges, int(countBridges))

// add device
devID := "abc123"
b := bridges[0]
addr, err := b.addDevice(devID)
assert.NoError(err)
if addr < 1 {
assert.Fail("address cannot be less then 1")
}

// remove device
err = b.removeDevice("")
assert.Error(err)

err = b.removeDevice(devID)
assert.NoError(err)
}
30 changes: 28 additions & 2 deletions filesystem.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ const (
// networkFileType represents a network file type (pod only)
networkFileType

// hypervisorFileType represents a hypervisor file type (pod only)
hypervisorFileType

// processFileType represents a process file type
processFileType

Expand All @@ -64,6 +67,9 @@ const stateFile = "state.json"
// networkFile is the file name storing a pod network.
const networkFile = "network.json"

// hypervisorFile is the file name storing a hypervisor's state.
const hypervisorFile = "hypervisor.json"

// processFile is the file name storing a container process.
const processFile = "process.json"

Expand Down Expand Up @@ -109,6 +115,10 @@ type resourceStorage interface {
fetchPodNetwork(podID string) (NetworkNamespace, error)
storePodNetwork(podID string, networkNS NetworkNamespace) error

// Hypervisor resources
fetchHypervisorState(podID string, state interface{}) error
storeHypervisorState(podID string, state interface{}) error

// Container resources
storeContainerResource(podID, containerID string, resource podResource, data interface{}) error
deleteContainerResources(podID, containerID string, resources []podResource) error
Expand Down Expand Up @@ -319,7 +329,7 @@ func (fs *filesystem) fetchDeviceFile(fileData []byte, devices *[]Device) error
func resourceNeedsContainerID(podSpecific bool, resource podResource) bool {

switch resource {
case lockFileType, networkFileType:
case lockFileType, networkFileType, hypervisorFileType:
// pod-specific resources
return false
default:
Expand All @@ -342,7 +352,7 @@ func resourceDir(podSpecific bool, podID, containerID string, resource podResour
case configFileType:
path = configStoragePath
break
case stateFileType, networkFileType, processFileType, lockFileType, mountsFileType, devicesFileType:
case stateFileType, networkFileType, processFileType, lockFileType, mountsFileType, devicesFileType, hypervisorFileType:
path = runStoragePath
break
default:
Expand Down Expand Up @@ -378,6 +388,8 @@ func (fs *filesystem) resourceURI(podSpecific bool, podID, containerID string, r
filename = stateFile
case networkFileType:
filename = networkFile
case hypervisorFileType:
filename = hypervisorFile
case processFileType:
filename = processFile
case lockFileType:
Expand Down Expand Up @@ -429,6 +441,7 @@ func (fs *filesystem) commonResourceChecks(podSpecific bool, podID, containerID
case configFileType:
case stateFileType:
case networkFileType:
case hypervisorFileType:
case processFileType:
case mountsFileType:
case devicesFileType:
Expand Down Expand Up @@ -598,10 +611,23 @@ func (fs *filesystem) fetchPodNetwork(podID string) (NetworkNamespace, error) {
return networkNS, nil
}

func (fs *filesystem) fetchHypervisorState(podID string, state interface{}) error {
return fs.fetchResource(true, podID, "", hypervisorFileType, state)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is perfect, but look at commonResourceChecks(), you have to update the list of known file type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

change applied, thanks

}

func (fs *filesystem) storePodNetwork(podID string, networkNS NetworkNamespace) error {
return fs.storePodResource(podID, networkFileType, networkNS)
}

func (fs *filesystem) storeHypervisorState(podID string, state interface{}) error {
hypervisorFile, _, err := fs.resourceURI(true, podID, "", hypervisorFileType)
if err != nil {
return err
}

return fs.storeFile(hypervisorFile, state)
}

func (fs *filesystem) deletePodResources(podID string, resources []podResource) error {
if resources == nil {
resources = []podResource{configFileType, stateFileType}
Expand Down
13 changes: 12 additions & 1 deletion hypervisor.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ const (
defaultVCPUs = 1
// 2 GiB
defaultMemSzMiB = 2048

defaultBridges = 1
)

// deviceType describes a virtualized device type.
Expand Down Expand Up @@ -161,6 +163,10 @@ type HypervisorConfig struct {
// Pod configuration VMConfig.Memory overwrites this.
DefaultMemSz uint32

// DefaultBridges specifies default number of bridges for the VM.
// Bridges can be used to hot plug devices
DefaultBridges uint32

// MemPrealloc specifies if the memory should be pre-allocated
MemPrealloc bool

Expand Down Expand Up @@ -203,6 +209,10 @@ func (conf *HypervisorConfig) valid() (bool, error) {
conf.DefaultMemSz = defaultMemSzMiB
}

if conf.DefaultBridges == 0 {
conf.DefaultBridges = defaultBridges
}

return true, nil
}

Expand Down Expand Up @@ -434,7 +444,7 @@ func RunningOnVMM(cpuInfoPath string) (bool, error) {
// hypervisor is the virtcontainers hypervisor interface.
// The default hypervisor implementation is Qemu.
type hypervisor interface {
init(config HypervisorConfig) error
init(pod *Pod) error
createPod(podConfig PodConfig) error
startPod(startCh, stopCh chan struct{}) error
stopPod() error
Expand All @@ -445,4 +455,5 @@ type hypervisor interface {
hotplugRemoveDevice(devInfo interface{}, devType deviceType) error
getPodConsole(podID string) string
capabilities() capabilities
getState() interface{}
}
1 change: 1 addition & 0 deletions hypervisor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ func TestHypervisorConfigDefaults(t *testing.T) {
HypervisorPath: "",
DefaultVCPUs: defaultVCPUs,
DefaultMemSz: defaultMemSzMiB,
DefaultBridges: defaultBridges,
}
if reflect.DeepEqual(hypervisorConfig, hypervisorConfigDefaultsExpected) == false {
t.Fatal()
Expand Down
8 changes: 6 additions & 2 deletions mock_hypervisor.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ package virtcontainers
type mockHypervisor struct {
}

func (m *mockHypervisor) init(config HypervisorConfig) error {
valid, err := config.valid()
func (m *mockHypervisor) init(pod *Pod) error {
valid, err := pod.config.HypervisorConfig.valid()
if valid == false || err != nil {
return err
}
Expand Down Expand Up @@ -69,3 +69,7 @@ func (m *mockHypervisor) hotplugRemoveDevice(devInfo interface{}, devType device
func (m *mockHypervisor) getPodConsole(podID string) string {
return ""
}

func (m *mockHypervisor) getState() interface{} {
return nil
}
20 changes: 13 additions & 7 deletions mock_hypervisor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,24 +25,30 @@ import (
func TestMockHypervisorInit(t *testing.T) {
var m *mockHypervisor

wrongConfig := HypervisorConfig{
KernelPath: "",
ImagePath: "",
HypervisorPath: "",
pod := &Pod{
config: &PodConfig{
HypervisorConfig: HypervisorConfig{
KernelPath: "",
ImagePath: "",
HypervisorPath: "",
},
},
}

err := m.init(wrongConfig)
// wrong config
err := m.init(pod)
if err == nil {
t.Fatal()
}

rightConfig := HypervisorConfig{
pod.config.HypervisorConfig = HypervisorConfig{
KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel),
ImagePath: fmt.Sprintf("%s/%s", testDir, testImage),
HypervisorPath: fmt.Sprintf("%s/%s", testDir, testHypervisor),
}

err = m.init(rightConfig)
// right config
err = m.init(pod)
if err != nil {
t.Fatal(err)
}
Expand Down