Re: [libvirt] [PATCH v1 3/4] bhyve: fix SATA address allocation

As bhyve for a long time didn't have a notion of the explicit SATA controller and created a controller for each drive, the bhyve driver in libvirt acted in a similar way and didn't care about the SATA controllers and assigned PCI addresses to drives directly, as the generated command will look like this anyway: 2:0,ahci-hd,somedisk.img This no longer makes sense because: 1. After commit c07d1c1c4f it's not possible to assign PCI addresses to disks 2. Bhyve now supports multiple disk drives for a controller, so it's going away from 1:1 controller:disk mapping, so the controller object starts to make more sense now So, this patch does the following: - Assign PCI address to SATA controllers (previously we didn't do this) - Assign disk addresses instead of PCI addresses for disks. Now, when building a bhyve command, we take PCI address not from the disk itself but from its controller - Assign addresses at XML parsing time using the assignAddressesCallback. This is done mainly for being able to verify address allocation via xml2xml tests - Adjust existing bhyvexml2{xml,argv} tests to chase the new address allocation This patch is largely based on work of Fabian Freyer. --- po/POTFILES.in | 1 + src/bhyve/bhyve_command.c | 143 ++++++++++++++++----- src/bhyve/bhyve_device.c | 33 ++--- src/bhyve/bhyve_domain.c | 60 ++++++++- .../bhyvexml2argvdata/bhyvexml2argv-acpiapic.args | 4 +- tests/bhyvexml2argvdata/bhyvexml2argv-base.args | 4 +- .../bhyvexml2argv-bhyveload-bootorder.args | 5 +- .../bhyvexml2argv-bhyveload-bootorder1.args | 5 +- .../bhyvexml2argv-bhyveload-bootorder3.args | 5 +- .../bhyvexml2argv-bhyveload-explicitargs.args | 4 +- tests/bhyvexml2argvdata/bhyvexml2argv-console.args | 2 +- .../bhyvexml2argv-custom-loader.args | 4 +- .../bhyvexml2argv-disk-cdrom-grub.args | 4 +- .../bhyvexml2argv-disk-cdrom.args | 4 +- .../bhyvexml2argv-grub-bootorder.args | 6 +- .../bhyvexml2argv-grub-bootorder2.args | 6 +- .../bhyvexml2argv-grub-defaults.args | 4 +- .../bhyvexml2argvdata/bhyvexml2argv-localtime.args | 4 +- tests/bhyvexml2argvdata/bhyvexml2argv-macaddr.args | 4 +- .../bhyvexml2argv-serial-grub-nocons.args | 2 +- .../bhyvexml2argv-serial-grub.args | 2 +- tests/bhyvexml2argvdata/bhyvexml2argv-serial.args | 2 +- tests/bhyvexml2argvtest.c | 2 +- .../bhyvexml2xmlout-acpiapic.xml | 4 +- tests/bhyvexml2xmloutdata/bhyvexml2xmlout-base.xml | 4 +- .../bhyvexml2xmlout-bhyveload-bootorder.xml | 4 +- .../bhyvexml2xmlout-bhyveload-bootorder1.xml | 4 +- .../bhyvexml2xmlout-bhyveload-bootorder2.xml | 4 +- .../bhyvexml2xmlout-bhyveload-bootorder3.xml | 4 +- .../bhyvexml2xmlout-bhyveload-bootorder4.xml | 4 +- .../bhyvexml2xmlout-bhyveload-explicitargs.xml | 4 +- .../bhyvexml2xmlout-console.xml | 4 +- .../bhyvexml2xmlout-custom-loader.xml | 4 +- .../bhyvexml2xmlout-disk-cdrom-grub.xml | 4 +- .../bhyvexml2xmlout-disk-cdrom.xml | 4 +- .../bhyvexml2xmlout-grub-bootorder.xml | 4 +- .../bhyvexml2xmlout-grub-bootorder2.xml | 4 +- .../bhyvexml2xmlout-grub-defaults.xml | 4 +- .../bhyvexml2xmlout-localtime.xml | 4 +- .../bhyvexml2xmlout-macaddr.xml | 4 +- .../bhyvexml2xmlout-metadata.xml | 5 +- .../bhyvexml2xmlout-serial-grub-nocons.xml | 4 +- .../bhyvexml2xmlout-serial-grub.xml | 4 +- .../bhyvexml2xmloutdata/bhyvexml2xmlout-serial.xml | 4 +- tests/bhyvexml2xmltest.c | 2 + 45 files changed, 279 insertions(+), 118 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index e66bb7a3a..632aa7736 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -14,6 +14,7 @@ src/access/viraccessdriverpolkit.c src/access/viraccessmanager.c src/bhyve/bhyve_command.c src/bhyve/bhyve_device.c +src/bhyve/bhyve_domain.c src/bhyve/bhyve_driver.c src/bhyve/bhyve_monitor.c src/bhyve/bhyve_parse_command.c diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c index 8a29977ff..a2fd40378 100644 --- a/src/bhyve/bhyve_command.c +++ b/src/bhyve/bhyve_command.c @@ -148,40 +148,97 @@ bhyveBuildConsoleArgStr(const virDomainDef *def, virCommandPtr cmd) } static int -bhyveBuildDiskArgStr(const virDomainDef *def ATTRIBUTE_UNUSED, - virDomainDiskDefPtr disk, - virCommandPtr cmd) +bhyveBuildAHCIControllerArgStr(const virDomainDef *def, + virDomainControllerDefPtr controller, + virConnectPtr conn, + virCommandPtr cmd) { - const char *bus_type; + virBuffer buf = VIR_BUFFER_INITIALIZER; + virBuffer device = VIR_BUFFER_INITIALIZER; const char *disk_source; + size_t i; + int ret = -1; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = def->disks[i]; + if (disk->bus != VIR_DOMAIN_DISK_BUS_SATA) + continue; + + if (disk->info.addr.drive.controller != controller->idx) + continue;

+ + VIR_DEBUG("disk %zu controller %d", i, controller->idx); + + if ((virDomainDiskGetType(disk) != VIR_STORAGE_TYPE_FILE) && + (virDomainDiskGetType(disk) != VIR_STORAGE_TYPE_VOLUME)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("unsupported disk type")); + goto error; + } + + if (virStorageTranslateDiskSourcePool(conn, disk) < 0) + goto error; + + disk_source = virDomainDiskGetSource(disk); + + if ((disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) && + (disk_source == NULL)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("cdrom device without source path " + "not supported")); + goto error; + } - switch (disk->bus) { - case VIR_DOMAIN_DISK_BUS_SATA: switch (disk->device) { case VIR_DOMAIN_DISK_DEVICE_DISK: - bus_type = "ahci-hd"; + if ((bhyveDriverGetCaps(conn) & BHYVE_CAP_AHCI32SLOT) != 0)

+ virBufferAsprintf(&device, ",hd:%s", disk_source); + else + virBufferAsprintf(&device, "-hd,%s", disk_source); break; case VIR_DOMAIN_DISK_DEVICE_CDROM: - bus_type = "ahci-cd"; + if ((bhyveDriverGetCaps(conn) & BHYVE_CAP_AHCI32SLOT) != 0)

+ virBufferAsprintf(&device, ",cd:%s", disk_source); + else + virBufferAsprintf(&device, "-cd,%s", disk_source);

break; default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("unsupported disk device")); - return -1; - } - break; - case VIR_DOMAIN_DISK_BUS_VIRTIO: - if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { - bus_type = "virtio-blk"; - } else { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("unsupported disk device")); - return -1; + goto error; } - break; - default: + virBufferAddBuffer(&buf, &device); + virBufferFreeAndReset(&device); + } + + if (virBufferCheckError(&buf) <0) + goto error; + + virCommandAddArg(cmd, "-s"); + virCommandAddArgFormat(cmd, "%d:0,ahci%s", + controller->info.addr.pci.slot, + virBufferCurrentContent(&buf)); + + ret = 0; + error: + virBufferFreeAndReset(&buf); + return ret; +} + +static int +bhyveBuildVirtIODiskArgStr(const virDomainDef *def ATTRIBUTE_UNUSED, + virDomainDiskDefPtr disk, + virConnectPtr conn, + virCommandPtr cmd) +{ + const char *disk_source; + + if (virStorageTranslateDiskSourcePool(conn, disk) < 0) + return -1; + + if (disk->device != VIR_DOMAIN_DISK_DEVICE_DISK) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("unsupported disk bus type")); + _("unsupported disk device")); return -1; } @@ -194,17 +251,9 @@ bhyveBuildDiskArgStr(const virDomainDef *def ATTRIBUTE_UNUSED, disk_source = virDomainDiskGetSource(disk); - if ((disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) && - (disk_source == NULL)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("cdrom device without source path " - "not supported")); - return -1; - } - virCommandAddArg(cmd, "-s"); - virCommandAddArgFormat(cmd, "%d:0,%s,%s", - disk->info.addr.pci.slot, bus_type, + virCommandAddArgFormat(cmd, "%d:0,virtio-blk,%s", + disk->info.addr.pci.slot, disk_source); return 0; @@ -278,6 +327,22 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn, virCommandAddArgList(cmd, "-s", "0:0,hostbridge", NULL); /* Devices */ + for (i = 0; i < def->ncontrollers; i++) { + virDomainControllerDefPtr controller = def->controllers[i]; + switch (controller->type) { + case VIR_DOMAIN_CONTROLLER_TYPE_PCI: + if (controller->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("unsupported PCI controller model: only PCI root supported")); + goto error; + } + break; + case VIR_DOMAIN_CONTROLLER_TYPE_SATA: + if (bhyveBuildAHCIControllerArgStr(def, controller, conn, cmd) < 0) + goto error; + break; + } + } for (i = 0; i < def->nnets; i++) { virDomainNetDefPtr net = def->nets[i]; if (bhyveBuildNetArgStr(def, net, cmd, dryRun) < 0) @@ -286,11 +351,19 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn, for (i = 0; i < def->ndisks; i++) { virDomainDiskDefPtr disk = def->disks[i]; - if (virStorageTranslateDiskSourcePool(conn, disk) < 0) - goto error; - - if (bhyveBuildDiskArgStr(def, disk, cmd) < 0) + switch (disk->bus) { + case VIR_DOMAIN_DISK_BUS_SATA: + /* Handled by AHCI controller */ + break;