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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list