Re: [libvirt] [PATCH v4 3/3] bhyve: implement PCI address allocation

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.

> 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?

> +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.

> +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.