8:17 a.m.
Technically, this is a v2 of:
https://www.redhat.com/archives/libvir-list/2019-August/msg01273.html
but a different approach is implemented here. The limitation for
lsilogic, buslogic, spapr-vscsi and vmpvscsi comes from this document:
https://docs.vmware.com/en/VMware-vSphere/6.7/com.vmware.vsphere.vm_admin...
Michal Prívozník (2):
domain_conf: Make virDomainDeviceFindSCSIController accept
virDomainDeviceDriveAddress struct
domain_conf: Relax SCSI addr used check
src/conf/domain_conf.c | 59 +++++++++++++++++++++++++++++-----
src/conf/domain_conf.h | 2 +-
src/qemu/qemu_domain_address.c | 2 +-
src/vbox/vbox_common.c | 2 +-
4 files changed, 54 insertions(+), 11 deletions(-)
--
2.21.0
8:17 a.m.
New subject: [libvirt] [PATCH v2 1/2] domain_conf: Make virDomainDeviceFindSCSIController accept virDomainDeviceDriveAddress struct
So far, the virDomainDeviceFindSCSIController() takes
virDomainDeviceInfo structure which is an overkill. It assumes
that the passed structure is type of
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE which is not obvious.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 6 +++---
src/conf/domain_conf.h | 2 +-
src/qemu/qemu_domain_address.c | 2 +-
src/vbox/vbox_common.c | 2 +-
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3dc638f0de..69c486f382 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8542,13 +8542,13 @@ virDomainHostdevDefParseXMLCaps(xmlNodePtr node ATTRIBUTE_UNUSED,
virDomainControllerDefPtr
virDomainDeviceFindSCSIController(const virDomainDef *def,
- virDomainDeviceInfoPtr info)
+ const virDomainDeviceDriveAddress *addr)
{
size_t i;
for (i = 0; i < def->ncontrollers; i++) {
if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI
&&
- def->controllers[i]->idx == info->addr.drive.controller)
+ def->controllers[i]->idx == addr->controller)
return def->controllers[i];
}
@@ -18527,7 +18527,7 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
* So let's grab the model from it and update the model we're
* going to add as long as this one isn't undefined. The premise
* being keeping the same controller model for all SCSI hostdevs. */
- cont = virDomainDeviceFindSCSIController(def, hostdev->info);
+ cont = virDomainDeviceFindSCSIController(def,
&hostdev->info->addr.drive);
if (cont && cont->model != -1)
newModel = cont->model;
}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 82631ecb07..93de663cc1 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2832,7 +2832,7 @@ int virDomainDiskGetFormat(virDomainDiskDefPtr def);
void virDomainDiskSetFormat(virDomainDiskDefPtr def, int format);
virDomainControllerDefPtr
virDomainDeviceFindSCSIController(const virDomainDef *def,
- virDomainDeviceInfoPtr info);
+ const virDomainDeviceDriveAddress *addr);
virDomainDiskDefPtr virDomainDiskFindByBusAndDst(virDomainDefPtr def,
int bus,
char *dst);
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 216ba6235e..ce117e1669 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -111,7 +111,7 @@ qemuDomainFindSCSIControllerModel(const virDomainDef *def,
{
virDomainControllerDefPtr cont;
- if (!(cont = virDomainDeviceFindSCSIController(def, info))) {
+ if (!(cont = virDomainDeviceFindSCSIController(def, &info->addr.drive))) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unable to find a SCSI controller for idx=%d"),
info->addr.drive.controller);
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index ddabcb80ca..749c1b807d 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -1121,7 +1121,7 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
case VIR_DOMAIN_DISK_BUS_SCSI:
VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME, &storageCtlName);
- cont = virDomainDeviceFindSCSIController(def, &disk->info);
+ cont = virDomainDeviceFindSCSIController(def,
&disk->info.addr.drive);
if (cont && cont->model ==
VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) {
VBOX_UTF16_FREE(storageCtlName);
VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME, &storageCtlName);
--
2.21.0
10:51 a.m.
New subject: [libvirt] [PATCH v2 1/2] domain_conf: Make virDomainDeviceFindSCSIController accept virDomainDeviceDriveAddress struct
On Wed, Sep 11, 2019 at 03:17:41PM +0200, Michal Privoznik wrote:
So far, the virDomainDeviceFindSCSIController() takes
virDomainDeviceInfo structure which is an overkill. It assumes
that the passed structure is type of
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE which is not obvious.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 6 +++---
src/conf/domain_conf.h | 2 +-
src/qemu/qemu_domain_address.c | 2 +-
src/vbox/vbox_common.c | 2 +-
4 files changed, 6 insertions(+), 6 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
8:17 a.m.
New subject: [libvirt] [PATCH v2 2/2] domain_conf: Relax SCSI addr used check
In domain_conf.c we have virDomainSCSIDriveAddressIsUsed()
function which returns true or false if given drive address is
already in use for given domain config or not. However, it also
takes a shortcut and returns true (meaning address in use) if the
unit number equals 7. This is because for some controllers this
is reserved address. The limitation comes mostly from vmware and
applies to lsilogic, buslogic, spapr-vscsi and vmpvscsi models.
On the other hand, we were not checking for the maximum unit
number (aka LUN number) which is also relevant and differs from
model to model.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 53 ++++++++++++++++++++++++++++++++++++++----
1 file changed, 48 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 69c486f382..3e7603891f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4813,11 +4813,54 @@ bool
virDomainSCSIDriveAddressIsUsed(const virDomainDef *def,
const virDomainDeviceDriveAddress *addr)
{
- /* In current implementation, the maximum unit number of a controller
- * is either 16 or 7 (narrow SCSI bus), and if the maximum unit number
- * is 16, the controller itself is on unit 7 */
- if (addr->unit == 7)
- return true;
+ const virDomainControllerDef *cont;
+
+ cont = virDomainDeviceFindSCSIController(def, addr);
+ if (cont) {
+ int max = -1;
+ int reserved = -1;
+
+ /* Different controllers have different limits. These limits here are
+ * taken from QEMU source code, but nevertheless they should apply to
+ * other hypervisors too. */
+ switch ((virDomainControllerModelSCSI) cont->model) {
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL:
+ max = 16383;
+ break;
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI:
+ max = 31;
+ reserved = 7;
+ break;
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068:
+ max = 1;
+ break;
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078:
+ max = 255;
+ break;
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
+ max = 0;
+ reserved = 7;
+ break;
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI:
+ max = 0;
+ reserved = 7;
+ break;
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
+ reserved = 7;
+ break;
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST:
+ break;
+ }
+
+ if (max != -1 && addr->unit >= max)
+ return true;
+ if (reserved != -1 && addr->unit == reserved)
+ return true;
+ }
if (virDomainDriveAddressIsUsedByDisk(def, VIR_DOMAIN_DISK_BUS_SCSI,
addr) ||
--
2.21.0
8:24 a.m.
New subject: [libvirt] [PATCH v2 2/2] domain_conf: Relax SCSI addr used check
On Wed, Sep 11, 2019 at 03:17:42PM +0200, Michal Privoznik wrote:
In domain_conf.c we have virDomainSCSIDriveAddressIsUsed()
function which returns true or false if given drive address is
already in use for given domain config or not. However, it also
takes a shortcut and returns true (meaning address in use) if the
unit number equals 7. This is because for some controllers this
is reserved address. The limitation comes mostly from vmware and
applies to lsilogic, buslogic, spapr-vscsi and vmpvscsi models.
On the other hand, we were not checking for the maximum unit
number (aka LUN number) which is also relevant and differs from
model to model.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 53 ++++++++++++++++++++++++++++++++++++++----
1 file changed, 48 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 69c486f382..3e7603891f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4813,11 +4813,54 @@ bool
virDomainSCSIDriveAddressIsUsed(const virDomainDef *def,
const virDomainDeviceDriveAddress *addr)
{
- /* In current implementation, the maximum unit number of a controller
- * is either 16 or 7 (narrow SCSI bus), and if the maximum unit number
- * is 16, the controller itself is on unit 7 */
- if (addr->unit == 7)
- return true;
+ const virDomainControllerDef *cont;
+
+ cont = virDomainDeviceFindSCSIController(def, addr);
+ if (cont) {
+ int max = -1;
+ int reserved = -1;
+
+ /* Different controllers have different limits. These limits here are
+ * taken from QEMU source code, but nevertheless they should apply to
+ * other hypervisors too. */
+ switch ((virDomainControllerModelSCSI) cont->model) {
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL:
+ max = 16383;
+ break;
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI:
+ max = 31;
+ reserved = 7;
+ break;
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068:
+ max = 1;
+ break;
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078:
+ max = 255;
+ break;
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
+ max = 0;
Surely this ^^^ means....
+ if (max != -1 && addr->unit >= max)
+ return true;
...this is always true and so we'll be unable to attach
a drive to any LUN at all.
+ if (reserved != -1 && addr->unit == reserved)
+ return true;
+ }
if (virDomainDriveAddressIsUsedByDisk(def, VIR_DOMAIN_DISK_BUS_SCSI,
addr) ||
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
10:16 a.m.
New subject: [libvirt] [PATCH v2 2/2] domain_conf: Relax SCSI addr used check
On 9/11/19 3:24 PM, Daniel P. Berrangé wrote:
On Wed, Sep 11, 2019 at 03:17:42PM +0200, Michal Privoznik wrote:
> In domain_conf.c we have virDomainSCSIDriveAddressIsUsed()
> function which returns true or false if given drive address is
> already in use for given domain config or not. However, it also
> takes a shortcut and returns true (meaning address in use) if the
> unit number equals 7. This is because for some controllers this
> is reserved address. The limitation comes mostly from vmware and
> applies to lsilogic, buslogic, spapr-vscsi and vmpvscsi models.
> On the other hand, we were not checking for the maximum unit
> number (aka LUN number) which is also relevant and differs from
> model to model.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/conf/domain_conf.c | 53 ++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 48 insertions(+), 5 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 69c486f382..3e7603891f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -4813,11 +4813,54 @@ bool
> virDomainSCSIDriveAddressIsUsed(const virDomainDef *def,
> const virDomainDeviceDriveAddress *addr)
> {
> - /* In current implementation, the maximum unit number of a controller
> - * is either 16 or 7 (narrow SCSI bus), and if the maximum unit number
> - * is 16, the controller itself is on unit 7 */
> - if (addr->unit == 7)
> - return true;
> + const virDomainControllerDef *cont;
> +
> + cont = virDomainDeviceFindSCSIController(def, addr);
> + if (cont) {
> + int max = -1;
> + int reserved = -1;
> +
> + /* Different controllers have different limits. These limits here are
> + * taken from QEMU source code, but nevertheless they should apply to
> + * other hypervisors too. */
> + switch ((virDomainControllerModelSCSI) cont->model) {
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL:
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL:
> + max = 16383;
> + break;
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI:
> + max = 31;
> + reserved = 7;
> + break;
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068:
> + max = 1;
> + break;
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078:
> + max = 255;
> + break;
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
> + max = 0;
Surely this ^^^ means....
> + if (max != -1 && addr->unit >= max)
> + return true;
...this is always true and so we'll be unable to attach
a drive to any LUN at all.
Ah, good catch. Looks like I've misread qemu's sorce code. Conside this squashed
in:
diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
index 3e7603891f..31cff38ae3 100644
--- i/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -4840,11 +4840,9 @@ virDomainSCSIDriveAddressIsUsed(const virDomainDef *def,
max = 255;
break;
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
- max = 0;
reserved = 7;
break;
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI:
- max = 0;
reserved = 7;
break;
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
Michal
10:53 a.m.
New subject: [libvirt] [PATCH v2 2/2] domain_conf: Relax SCSI addr used check
On Wed, Sep 11, 2019 at 05:16:31PM +0200, Michal Privoznik wrote:
On 9/11/19 3:24 PM, Daniel P. Berrangé wrote:
> On Wed, Sep 11, 2019 at 03:17:42PM +0200, Michal Privoznik wrote:
>> In domain_conf.c we have virDomainSCSIDriveAddressIsUsed()
>> function which returns true or false if given drive address is
>> already in use for given domain config or not. However, it also
>> takes a shortcut and returns true (meaning address in use) if the
>> unit number equals 7. This is because for some controllers this
>> is reserved address. The limitation comes mostly from vmware and
>> applies to lsilogic, buslogic, spapr-vscsi and vmpvscsi models.
>> On the other hand, we were not checking for the maximum unit
>> number (aka LUN number) which is also relevant and differs from
>> model to model.
>>
>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>> ---
>> src/conf/domain_conf.c | 53 ++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 48 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 69c486f382..3e7603891f 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -4813,11 +4813,54 @@ bool
>> virDomainSCSIDriveAddressIsUsed(const virDomainDef *def,
>> const virDomainDeviceDriveAddress *addr)
>> {
>> - /* In current implementation, the maximum unit number of a controller
>> - * is either 16 or 7 (narrow SCSI bus), and if the maximum unit number
>> - * is 16, the controller itself is on unit 7 */
>> - if (addr->unit == 7)
>> - return true;
>> + const virDomainControllerDef *cont;
>> +
>> + cont = virDomainDeviceFindSCSIController(def, addr);
>> + if (cont) {
>> + int max = -1;
>> + int reserved = -1;
>> +
>> + /* Different controllers have different limits. These limits here are
>> + * taken from QEMU source code, but nevertheless they should apply to
>> + * other hypervisors too. */
>> + switch ((virDomainControllerModelSCSI) cont->model) {
>> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
>> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL:
>> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL:
>> + max = 16383;
>> + break;
>> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI:
>> + max = 31;
>> + reserved = 7;
>> + break;
>> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068:
>> + max = 1;
>> + break;
>> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078:
>> + max = 255;
>> + break;
>> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
>> + max = 0;
>
> Surely this ^^^ means....
>
>> + if (max != -1 && addr->unit >= max)
>> + return true;
>
> ...this is always true and so we'll be unable to attach
> a drive to any LUN at all.
Ah, good catch. Looks like I've misread qemu's sorce code. Conside this squashed
in:
diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
index 3e7603891f..31cff38ae3 100644
--- i/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -4840,11 +4840,9 @@ virDomainSCSIDriveAddressIsUsed(const virDomainDef *def,
max = 255;
break;
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
- max = 0;
reserved = 7;
break;
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI:
- max = 0;
reserved = 7;
break;
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
With that
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
1860
days inactive
1901
days old
6 comments
2 participants
participants (2)
-
Daniel P. Berrangé -
Michal Privoznik