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

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.