Re: [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&gt; >> --- >> 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: