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

Re: [libvirt] [PATCH v2 10/15] vbox: Process <controller> element in domain XML



On Fri, 2017-11-03 at 09:51 -0400, John Ferlan wrote:
> 
> On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
> > This patch enables the VBOX driver to process the <controller>
> > element
> > in domain XML through which one can now customize the controller
> > model.
> > 
> > Since VirtualBox has two distinct SAS and SCSI they do not "map"
> > directly to libvirt XML, he VBOX driver uses <contoller type="scsi"
> > model="lsisas1068" /> to create SAS controller in VBOX VM.
> > Additionally
> > once can set model on the IDE controller to be one of "piix3",
> > "piix4"
> > or "ich6".
> > ---
> >  src/vbox/vbox_common.c | 214
> > ++++++++++++++++++++++++++++++++++++++-----------
> >  src/vbox/vbox_common.h |   8 ++
> >  2 files changed, 176 insertions(+), 46 deletions(-)
> > 
> 
> So beyond patch 3 which I know you have to repost anyway - starting
> with
> this patch and going forward, I figure you have to repost anyway as
> long
> as I get the question in patch 5 answered of course...
> 
> This patch left me with a concern.  Up to this point vboxAttachDrives
> would add at least one controller for each type supported regardless
> of
> whether the VM used it.
> 
> After this patch only those controllers defined in the VM XML will be
> added. Which would seem to be fine, except for the case of hotplug
> which
> I wasn't clear whether vbox support or not.
> 
> Let's say the VM is defined with and IDE controller, if someone tried
> to
> add a SCSI disk after startup, then there'd be no SCSI controller
> already present.
> 

I've just checked this with VirutalBox GUI and the only thing that can
be changed on live VM is removable media (CD/DVD). All options to add
disk devices to controller are disabled when VM is running. Neither
quick web search returned anything useful regarding hot-add disk
support on VBOX.
Having said that, the original behavior was, IIRC, the reason why I
actually started working on this series. According to our tech support
they have ran into a couple of cases where the OS would BSOD (some
legacy Windows OS e.g. 2000 or XP) when there was a controller (without
a disk) attached to VM that the OS did not handle well. It sounded very
strange to me but I've been told it was not a one-off case though I
personally haven't reproduced it. Unfortunately, I won't be able to
provide more details as we no longer use VBox internally (migrated to
KVM) so those potential test cases are no longer around.

> 
> > diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> > index 2bd891efb..9d45e4a76 100644
> > --- a/src/vbox/vbox_common.c
> > +++ b/src/vbox/vbox_common.c
> > @@ -406,6 +406,160 @@ static char *vboxGenerateMediumName(PRUint32
> > storageBus,
> >      return name;
> >  }
> >  
> > +
> > +static int
> > +vboxSetStorageController(virDomainControllerDefPtr controller,
> > +                         vboxDriverPtr data,
> > +                         IMachine *machine)
> > +{
> > +    PRUnichar *controllerName = NULL;
> > +    PRInt32 vboxModel = StorageControllerType_Null;
> > +    PRInt32 vboxBusType = StorageBus_Null;
> > +    IStorageController *vboxController = NULL;
> > +    nsresult rc = 0;
> > +    char *debugName = NULL;
> > +    int ret = -1;
> > +
> > +    /* libvirt controller type => vbox bus type */
> > +    switch ((virDomainControllerType) controller->type) {
> > +    case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
> > +        VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME,
> > &controllerName);
> > +        vboxBusType = StorageBus_Floppy;
> > +
> > +        break;
> > +    case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
> > +        VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME,
> > &controllerName);
> > +        vboxBusType = StorageBus_IDE;
> > +
> > +        break;
> > +    case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
> > +        VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME,
> > &controllerName);
> > +        vboxBusType = StorageBus_SCSI;
> > +
> > +        break;
> > +    case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
> > +        VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME,
> > &controllerName);
> > +        vboxBusType = StorageBus_SATA;
> 
> 
> I think prior to this patch - there should be a patch that "manages"
> all
> those range check differences, storageBus comparison checks, etc. for
> VIR_DOMAIN_CONTROLLER_TYPE_SATA that end up in future patches.
> 
> John
> 
> > +
> > +        break;
> > +    case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
> > +    case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
> > +    case VIR_DOMAIN_CONTROLLER_TYPE_USB:
> > +    case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
> > +    case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
> > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                       _("The vbox driver does not support %s
> > controller type"),
> > +                       virDomainControllerTypeToString(controller-
> > >type));
> > +        return -1;
> > +    }
> > +
> > +    /* libvirt scsi model => vbox scsi model */
> > +    if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
> > +        switch ((virDomainControllerModelSCSI) controller->model)
> > {
> > +        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
> > +        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO:
> > +            vboxModel = StorageControllerType_LsiLogic;
> > +
> > +            break;
> > +        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
> > +            vboxModel = StorageControllerType_BusLogic;
> > +
> > +            break;
> > +        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068:
> > +            /* in vbox, lsisas has a dedicated SAS bus type with
> > no model */
> > +            VBOX_UTF16_FREE(controllerName);
> > +            VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME,
> > &controllerName);
> > +            vboxBusType = StorageBus_SAS;
> > +
> > +            break;
> > +        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI:
> > +        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI:
> > +        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
> > +        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078:
> > +        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST:
> > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                           _("The vbox driver does not support %s
> > SCSI "
> > +                             "controller model"),
> > +                           virDomainControllerModelSCSITypeToStrin
> > g(controller->model));
> > +            goto cleanup;
> > +        }
> > +    /* libvirt ide model => vbox ide model */
> > +    } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
> > {
> > +        switch ((virDomainControllerModelIDE) controller->model) {
> > +        case VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3:
> > +            vboxModel = StorageControllerType_PIIX3;
> > +
> > +            break;
> > +        case VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4:
> > +            vboxModel = StorageControllerType_PIIX4;
> > +
> > +            break;
> > +        case VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6:
> > +            vboxModel = StorageControllerType_ICH6;
> > +
> > +            break;
> > +        case VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST:
> > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                           _("The vbox driver does not support %s
> > IDE "
> > +                             "controller model"),
> > +                             virDomainControllerModelIDETypeToStri
> > ng(controller->model));
> > +            goto cleanup;
> > +        }
> > +    }
> > +
> > +    VBOX_UTF16_TO_UTF8(controllerName, &debugName);
> > +    VIR_DEBUG("Adding VBOX storage controller (name: %s, busType:
> > %d)",
> > +               debugName, vboxBusType);
> > +
> > +    rc = gVBoxAPI.UIMachine.AddStorageController(machine,
> > controllerName,
> > +                                                 vboxBusType,
> > &vboxController);
> > +
> > +    if (NS_FAILED(rc)) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                       _("Failed to add storage controller "
> > +                         "(name: %s, busType: %d), rc=%08x"),
> > +                       debugName, vboxBusType, rc);
> > +        goto cleanup;
> > +    }
> > +
> > +    /* only IDE or SCSI controller have model choices */
> > +    if (vboxModel != StorageControllerType_Null) {
> > +        rc =
> > gVBoxAPI.UIStorageController.SetControllerType(vboxController,
> > +                                                            vboxMo
> > del);
> > +        if (NS_FAILED(rc)) {
> > +            virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                            _("Failed to change storage controller
> > model, "
> > +                              "rc=%08x"), rc);
> > +            goto cleanup;
> > +        }
> > +    }
> > +
> > +    ret = 0;
> > +
> > + cleanup:
> > +    VBOX_UTF16_FREE(controllerName);
> > +    VBOX_UTF8_FREE(debugName);
> > +    VBOX_RELEASE(vboxController);
> > +
> > +    return ret;
> > +}
> > +
> > +
> > +static int
> > +vboxAttachStorageControllers(virDomainDefPtr def,
> > +                             vboxDriverPtr data,
> > +                             IMachine *machine)
> > +{
> > +    size_t i;
> > +    for (i = 0; i < def->ncontrollers; i++) {
> > +        if (vboxSetStorageController(def->controllers[i], data,
> > machine) < 0)
> > +            return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +
> >  static virDrvOpenStatus
> >  vboxConnectOpen(virConnectPtr conn,
> >                  virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> > @@ -959,7 +1113,7 @@ static int
> >  vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
> > *machine)
> >  {
> >      size_t i;
> > -    int type, ret = 0;
> > +    int type, ret = 0, model = -1;
> >      const char *src = NULL;
> >      nsresult rc = 0;
> >      virDomainDiskDefPtr disk = NULL;
> > @@ -972,46 +1126,6 @@ vboxAttachDrives(virDomainDefPtr def,
> > vboxDriverPtr data, IMachine *machine)
> >  
> >      VBOX_IID_INITIALIZE(&mediumUUID);
> >  
> > -    /* add a storage controller for the mediums to be attached */
> > -    /* this needs to change when multiple controller are supported
> > for
> > -     * ver > 3.1 */
> > -    {
> > -        IStorageController *storageCtl = NULL;
> > -        PRUnichar *sName = NULL;
> > -
> > -        VBOX_UTF8_TO_UTF16("IDE Controller", &sName);
> > -        gVBoxAPI.UIMachine.AddStorageController(machine,
> > -                                                sName,
> > -                                                StorageBus_IDE,
> > -                                                &storageCtl);
> > -        VBOX_UTF16_FREE(sName);
> > -        VBOX_RELEASE(storageCtl);
> > -
> > -        VBOX_UTF8_TO_UTF16("SATA Controller", &sName);
> > -        gVBoxAPI.UIMachine.AddStorageController(machine,
> > -                                                sName,
> > -                                                StorageBus_SATA,
> > -                                                &storageCtl);
> > -        VBOX_UTF16_FREE(sName);
> > -        VBOX_RELEASE(storageCtl);
> > -
> > -        VBOX_UTF8_TO_UTF16("SCSI Controller", &sName);
> > -        gVBoxAPI.UIMachine.AddStorageController(machine,
> > -                                                sName,
> > -                                                StorageBus_SCSI,
> > -                                                &storageCtl);
> > -        VBOX_UTF16_FREE(sName);
> > -        VBOX_RELEASE(storageCtl);
> > -
> > -        VBOX_UTF8_TO_UTF16("Floppy Controller", &sName);
> > -        gVBoxAPI.UIMachine.AddStorageController(machine,
> > -                                                sName,
> > -                                                StorageBus_Floppy,
> > -                                                &storageCtl);
> > -        VBOX_UTF16_FREE(sName);
> > -        VBOX_RELEASE(storageCtl);
> > -    }
> > -
> >      for (i = 0; i < def->ndisks; i++) {
> >          disk = def->disks[i];
> >          src = virDomainDiskGetSource(disk);
> > @@ -1066,21 +1180,28 @@ vboxAttachDrives(virDomainDefPtr def,
> > vboxDriverPtr data, IMachine *machine)
> >  
> >          switch ((virDomainDiskBus) disk->bus) {
> >          case VIR_DOMAIN_DISK_BUS_IDE:
> > -            VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName);
> > +            VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME,
> > &storageCtlName);
> >              devicePort = def->disks[i]->info.addr.drive.bus;
> >              deviceSlot = def->disks[i]->info.addr.drive.unit;
> >  
> >              break;
> >          case VIR_DOMAIN_DISK_BUS_SATA:
> > -            VBOX_UTF8_TO_UTF16("SATA Controller",
> > &storageCtlName);
> > +            VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME,
> > &storageCtlName);
> >  
> >              break;
> >          case VIR_DOMAIN_DISK_BUS_SCSI:
> > -            VBOX_UTF8_TO_UTF16("SCSI Controller",
> > &storageCtlName);
> > +            VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME,
> > &storageCtlName);
> > +
> > +            model = virDomainDeviceFindControllerModel(def, &disk-
> > >info,
> > +                                                       VIR_DOMAIN_
> > CONTROLLER_TYPE_SCSI);
> > +            if (model ==
> > VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) {
> > +                VBOX_UTF16_FREE(storageCtlName);
> > +                VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME,
> > &storageCtlName);
> > +            }
> >  
> >              break;
> >          case VIR_DOMAIN_DISK_BUS_FDC:
> > -            VBOX_UTF8_TO_UTF16("Floppy Controller",
> > &storageCtlName);
> > +            VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME,
> > &storageCtlName);
> >              devicePort = 0;
> >              deviceSlot = disk->info.addr.drive.unit;
> >  
> > @@ -1148,7 +1269,6 @@ vboxAttachDrives(virDomainDefPtr def,
> > vboxDriverPtr data, IMachine *machine)
> >                  gVBoxAPI.UIMedium.SetType(medium,
> > MediumType_Normal);
> >                  VIR_DEBUG("Setting hard disk type to normal");
> >              }
> > -
> >          }
> >  
> >          VBOX_UTF16_TO_UTF8(storageCtlName, &controllerName);
> > @@ -1918,6 +2038,8 @@ vboxDomainDefineXMLFlags(virConnectPtr conn,
> > const char *xml, unsigned int flags
> >      gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine);
> >  
> >      vboxSetBootDeviceOrder(def, data, machine);
> > +    if (vboxAttachStorageControllers(def, data, machine) < 0)
> > +        goto cleanup;
> >      if (vboxAttachDrives(def, data, machine) < 0)
> >          goto cleanup;
> >      vboxAttachSound(def, machine);
> > diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h
> > index b08ad1e3e..3340374c1 100644
> > --- a/src/vbox/vbox_common.h
> > +++ b/src/vbox/vbox_common.h
> > @@ -326,6 +326,14 @@ enum HardDiskVariant
> >  # define VBOX_E_INVALID_SESSION_STATE 0x80BB000B
> >  # define VBOX_E_OBJECT_IN_USE 0x80BB000C
> >  
> > +/* VBOX storage controller name definitions */
> > +
> > +# define VBOX_CONTROLLER_IDE_NAME "IDE Controller"
> > +# define VBOX_CONTROLLER_FLOPPY_NAME "Floppy Controller"
> > +# define VBOX_CONTROLLER_SATA_NAME "SATA Controller"
> > +# define VBOX_CONTROLLER_SCSI_NAME "SCSI Controller"
> > +# define VBOX_CONTROLLER_SAS_NAME "SAS Controller"
> > +
> >  /* Simplied definitions in vbox_CAPI_*.h */
> >  
> >  typedef void const *PCVBOXXPCOM;
> > 


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