On 01/05/2017 09:46 AM, Roman Bogorodskiy wrote:
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;
Took a minute for me to see why you were doing this - so all the disks
attached to a controller are put directly into the controller's
commandline arg rather than them being separate args with a ref back to
the controller.
+
+ 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)
The "!= 0" is superfluous.
+ 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)
Same here.
+ virBufferAsprintf(&device, ",cd:%s", disk_source);
+ else
+ virBufferAsprintf(&device, "-cd,%s", disk_source);
It seems like there could be some consolidation of the multiple
nearly-identical printf's here. Possibly not worth the trouble though.
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;
This is a bit odd, but I see why you're doing it. Maybe you should put
the full name of the function (bhyveBuildAHCIControllerArgStr) in the
comment rather than just "AHCI controller".
Aside from these minor nits, my only concern is that of compatibility
when migrating forward and backward, but since I don't have a system
with bhyve I can't test that, so I'll assume that you've done your due
diligence in that regard. (Also, I'm assuming that you've successfully
run make syntax-check in addition to make check). Based on those
assumptions, ACK.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
