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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



  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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux