Laine Stump wrote:
> 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.
True.
> > +
> > + 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.
Will fix.
> > + 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.
Will fix it here too.
>
> > + 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".
Good idea.
> 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.
Unfortunately, it's neither forward or backward compatible. I was
thinking about preparing a script that strips addresses for the SATA
devices so user can undefine/define a domain and let libvirt regenerate
the proper addresses.
Thanks,
Roman Bogorodskiy
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
