On 05/11/2014 08:48 AM, Roman Bogorodskiy wrote:
Automatically allocate PCI addresses for devices instead
of hardcoding them in the driver code. The current
allocation schema is to dedicate an entire slot for each devices.
Also, allow having arbitrary number of devices.
I think this can be split in a separate patch.
---
po/POTFILES.in | 1 +
src/Makefile.am | 4 +
src/bhyve/bhyve_command.c | 142 ++++++++---------
src/bhyve/bhyve_device.c | 174 +++++++++++++++++++++
src/bhyve/bhyve_device.h | 38 +++++
src/bhyve/bhyve_domain.c | 75 +++++++++
src/bhyve/bhyve_domain.h | 39 +++++
src/bhyve/bhyve_driver.c | 12 +-
.../bhyvexml2argvdata/bhyvexml2argv-acpiapic.args | 2 +-
tests/bhyvexml2argvdata/bhyvexml2argv-acpiapic.xml | 2 +
tests/bhyvexml2argvdata/bhyvexml2argv-base.args | 2 +-
tests/bhyvexml2argvdata/bhyvexml2argv-base.xml | 2 +
tests/bhyvexml2argvdata/bhyvexml2argv-console.args | 4 +-
tests/bhyvexml2argvdata/bhyvexml2argv-console.xml | 2 +
.../bhyvexml2argv-disk-virtio.args | 2 +-
.../bhyvexml2argv-disk-virtio.xml | 2 +
tests/bhyvexml2argvdata/bhyvexml2argv-macaddr.args | 2 +-
tests/bhyvexml2argvdata/bhyvexml2argv-macaddr.xml | 2 +
tests/bhyvexml2argvdata/bhyvexml2argv-serial.args | 4 +-
tests/bhyvexml2argvdata/bhyvexml2argv-serial.xml | 2 +
20 files changed, 429 insertions(+), 84 deletions(-)
create mode 100644 src/bhyve/bhyve_device.c
create mode 100644 src/bhyve/bhyve_device.h
create mode 100644 src/bhyve/bhyve_domain.c
create mode 100644 src/bhyve/bhyve_domain.h
diff --git a/po/POTFILES.in b/po/POTFILES.in
index 6e8d465..ef2f114 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -9,6 +9,7 @@ gnulib/lib/regcomp.c
src/access/viraccessdriverpolkit.c
src/access/viraccessmanager.c
src/bhyve/bhyve_command.c
+src/bhyve/bhyve_device.c
src/bhyve/bhyve_driver.c
src/bhyve/bhyve_process.c
src/conf/capabilities.c
diff --git a/src/Makefile.am b/src/Makefile.am
index cfb7097..dc7f794 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -787,6 +787,10 @@ BHYVE_DRIVER_SOURCES = \
bhyve/bhyve_capabilities.h \
bhyve/bhyve_command.c \
bhyve/bhyve_command.h \
+ bhyve/bhyve_device.c \
+ bhyve/bhyve_device.h \
+ bhyve/bhyve_domain.c \
+ bhyve/bhyve_domain.h \
bhyve/bhyve_driver.h \
bhyve/bhyve_driver.c \
bhyve/bhyve_process.c \
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index 91a8731..bc7ec5c 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -41,21 +41,14 @@ VIR_LOG_INIT("bhyve.bhyve_command");
static int
bhyveBuildNetArgStr(const virDomainDef *def, virCommandPtr cmd, bool dryRun)
{
- virDomainNetDefPtr net = NULL;
- char *brname = NULL;
- char *realifname = NULL;
- int *tapfd = NULL;
char macaddr[VIR_MAC_STRING_BUFLEN];
+ size_t i;
- if (def->nnets != 1) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("domain should have one and only one net defined"));
- return -1;
- }
-
- net = def->nets[0];
-
- if (net) {
+ for (i = 0; i < def->nnets; i++) {
Moving this loop to the caller of the function would reduce the extra
indentation level.
+ char *realifname = NULL;
+ int *tapfd = NULL;
+ char *brname = NULL;
+ virDomainNetDefPtr net = net = def->nets[i];;
No need to assign it twice and one semicolon would be enough. :)
int actualType = virDomainNetGetActualType(net);
if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
@@ -146,7 +141,7 @@ bhyveBuildConsoleArgStr(const virDomainDef *def,
virCommandPtr cmd)
return -1;
}
- virCommandAddArgList(cmd, "-s", "31,lpc", NULL);
+ virCommandAddArgList(cmd, "-s", "1,lpc", NULL);
Are both slot numbers arbitrary? And do we care about backwards compatibility?
virCommandAddArg(cmd, "-l");
virCommandAddArgFormat(cmd, "com%d,%s",
chr->target.port + 1, chr->source.data.file.path);
@@ -157,46 +152,43 @@ bhyveBuildConsoleArgStr(const virDomainDef *def, virCommandPtr
cmd)
static int
bhyveBuildDiskArgStr(const virDomainDef *def, virCommandPtr cmd)
{
- virDomainDiskDefPtr disk;
const char *bus_type;
+ size_t i;
+
+ for (i = 0; i < def->ndisks; i++) {
+ virDomainDiskDefPtr disk = def->disks[i];
+
+ switch (disk->bus) {
+ case VIR_DOMAIN_DISK_BUS_SATA:
+ bus_type = "ahci-hd";
+ break;
+ case VIR_DOMAIN_DISK_BUS_VIRTIO:
+ bus_type = "virtio-blk";
+ break;
+ default:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("unsupported disk bus type"));
+ return -1;
+ }
- if (def->ndisks != 1) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("domain should have one and only one disk
defined"));
- return -1;
- }
-
- disk = def->disks[0];
-
- switch (disk->bus) {
- case VIR_DOMAIN_DISK_BUS_SATA:
- bus_type = "ahci-hd";
- break;
- case VIR_DOMAIN_DISK_BUS_VIRTIO:
- bus_type = "virtio-blk";
- break;
- default:
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("unsupported disk bus type"));
- return -1;
- }
+ if (disk->device != VIR_DOMAIN_DISK_DEVICE_DISK) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("unsupported disk device"));
+ return -1;
+ }
- if (disk->device != VIR_DOMAIN_DISK_DEVICE_DISK) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("unsupported disk device"));
- return -1;
- }
+ if (virDomainDiskGetType(disk) != VIR_STORAGE_TYPE_FILE) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("unsupported disk type"));
+ return -1;
+ }
- if (virDomainDiskGetType(disk) != VIR_STORAGE_TYPE_FILE) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("unsupported disk type"));
- return -1;
+ virCommandAddArg(cmd, "-s");
+ virCommandAddArgFormat(cmd, "%d:0,%s,%s",
+ disk->info.addr.pci.slot, bus_type,
+ virDomainDiskGetSource(disk));
Do SATA disks use a PCI slot too?
}
- virCommandAddArg(cmd, "-s");
- virCommandAddArgFormat(cmd, "2:0,%s,%s", bus_type,
- virDomainDiskGetSource(disk));
-
return 0;
}
@@ -279,9 +271,9 @@ virBhyveProcessBuildLoadCmd(bhyveConnPtr driver ATTRIBUTE_UNUSED,
virCommandPtr cmd;
virDomainDiskDefPtr disk;
- if (def->ndisks != 1) {
+ if (def->ndisks < 1) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("domain should have one and only one disk
defined"));
+ _("domain should have more than one disk defined"));
s/more than/at least/
return NULL;
}
diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c
new file mode 100644
index 0000000..c18665f
--- /dev/null
+++ b/src/bhyve/bhyve_device.c
@@ -0,0 +1,174 @@
+/*
+ * bhyve_device.c: bhyve device management
+ *
+ * Copyright (C) 2014 Roman Bogorodskiy
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <
http://www.gnu.org/licenses/>.
+ *
+ * Author: Roman Bogorodskiy
+ */
+
+#include <config.h>
+
+#include "bhyve_device.h"
+#include "domain_addr.h"
+#include "viralloc.h"
+#include "virlog.h"
+#include "virstring.h"
+
+#define VIR_FROM_THIS VIR_FROM_BHYVE
+
+VIR_LOG_INIT("bhyve.bhyve_device");
+
+static int
+bhyveCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
+ virDomainDeviceDefPtr device ATTRIBUTE_UNUSED,
+ virDomainDeviceInfoPtr info,
+ void *opaque)
+{
+ int ret = -1;
+ virDomainPCIAddressSetPtr addrs = opaque;
+ virDevicePCIAddressPtr addr = &info->addr.pci;
+
+ if (addr->domain == 0 && addr->bus == 0) {
+ if (addr->slot == 0) {
+ return 0;
+ } else if (addr->slot == 1) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("PCI bus 0 slot 1 is reserved for the implicit "
+ "LPC PCI-ISA bridge"));
It's only implied when the console is present. Does it make sense to let
someone use the address if they don't want the ISA bridge?
Maybe we should generate an explicit <controller> element for it too.
+ return -1;
+ }
+ }
+
+ if (virDomainPCIAddressReserveSlot(addrs, addr, VIR_PCI_CONNECT_TYPE_PCI) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ return ret;
+}
+
+bhyveAssignDevicePCISlots(virDomainDefPtr def,
+ virDomainPCIAddressSetPtr addrs)
+{
+ size_t i;
+ virDevicePCIAddress lpc_addr;
+
+ /* explicitly reserve slot 1 for LPC-ISA bridge */
+ memset(&lpc_addr, 0, sizeof(lpc_addr));
+ lpc_addr.slot = 0x1;
+
+ if (virDomainPCIAddressReserveSlot(addrs, &lpc_addr, VIR_PCI_CONNECT_TYPE_PCI)
< 0)
+ goto error;
+
+ for (i = 0; i < def->ndisks; i++) {
+ if (def->disks[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE
&&
+ def->disks[i]->info.addr.pci.slot != 0)
+ continue;
+ if (virDomainPCIAddressReserveNextSlot(addrs,
+ &def->disks[i]->info,
+ VIR_PCI_CONNECT_TYPE_PCI) < 0)
+ goto error;
+ }
+
+ for (i = 0; i < def->nnets; i++) {
+ if (def->nets[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
+ continue;
+ if (virDomainPCIAddressReserveNextSlot(addrs,
+ &def->nets[i]->info,
+ VIR_PCI_CONNECT_TYPE_PCI) < 0)
+ goto error;
+ }
Assigning nets before disks would match the hardcoded slots that were used by
a domain with one net and one disk before.
+
+ for (i = 0; i < def->ncontrollers; i++) {
+ if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
+ if (def->controllers[i]->info.type !=
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
+ continue;
+ if (def->controllers[i]->model ==
VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)
+ continue;
+
+ if (virDomainPCIAddressReserveNextSlot(addrs,
+
&def->controllers[i]->info,
+ VIR_PCI_CONNECT_TYPE_PCI) < 0)
+ goto error;
+ }
I think unsupported controllers don't need PCI slots.
+
+ }
+
+ return 0;
+
+ error:
+ return -1;
+}
+
diff --git a/src/bhyve/bhyve_domain.h b/src/bhyve/bhyve_domain.h
new file mode 100644
index 0000000..d59fe58
--- /dev/null
+++ b/src/bhyve/bhyve_domain.h
@@ -0,0 +1,39 @@
+/*
+ * bhyve_domain.h: bhyve domain private state headers
+ *
+ * Copyright (C) 2014 Roman Bogorodskiy
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <
http://www.gnu.org/licenses/>.
+ *
+ * Author: Roman Bogorodskiy
+ */
+
+#ifndef __BHYVE_DOMAIN_H__
+# define __BHYVE_DOMAIN_H__
+
+# include "domain_addr.h"
+# include "domain_conf.h"
+
+typedef struct _bhyveDomainObjPrivate bhyveDomainObjPrivate;
+typedef bhyveDomainObjPrivate *bhyveDomainObjPrivatePtr;
+struct _bhyveDomainObjPrivate {
+ virDomainPCIAddressSetPtr pciaddrs;
+ int persistentAddrs;
bool would be enough.
+};
+
+extern virDomainXMLPrivateDataCallbacks virBhyveDriverPrivateDataCallbacks;
+extern virDomainDefParserConfig virBhyveDriverDomainDefParserConfig;
+
+#endif /* __BHYVE_DOMAIN_H__ */
Jan