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

[Daniel P. Berrangé]

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

With that

Reviewed-by: Daniel P. Berrangé <berrange at 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 :|