[libvirt] [PATCH v2 2/2] domain_conf: Relax SCSI addr used check

[Michal Privoznik]

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 at 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