diff mbox series

[02/10] scsi: switch to bus->check_address

Message ID 20200925172604.2142227-3-pbonzini@redhat.com
State New
Headers show
Series Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread | expand

Commit Message

Paolo Bonzini Sept. 25, 2020, 5:25 p.m. UTC 
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-bus.c | 122 ++++++++++++++++++++++++++++-----------------
 1 file changed, 75 insertions(+), 47 deletions(-)

Comments

Stefan Hajnoczi Sept. 28, 2020, 9:43 a.m. UTC | #1 
On Fri, Sep 25, 2020 at 01:25:56PM -0400, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> ---

>  hw/scsi/scsi-bus.c | 122 ++++++++++++++++++++++++++++-----------------

>  1 file changed, 75 insertions(+), 47 deletions(-)


Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Maxim Levitsky Sept. 30, 2020, 2:28 p.m. UTC | #2 
On Fri, 2020-09-25 at 13:25 -0400, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> ---

>  hw/scsi/scsi-bus.c | 122 ++++++++++++++++++++++++++++-----------------

>  1 file changed, 75 insertions(+), 47 deletions(-)

> 

> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c

> index 3284a5d1fb..94921c04b1 100644

> --- a/hw/scsi/scsi-bus.c

> +++ b/hw/scsi/scsi-bus.c

> @@ -22,33 +22,6 @@ static void scsi_req_dequeue(SCSIRequest *req);

>  static uint8_t *scsi_target_alloc_buf(SCSIRequest *req, size_t len);

>  static void scsi_target_free_buf(SCSIRequest *req);

>  

> -static Property scsi_props[] = {

> -    DEFINE_PROP_UINT32("channel", SCSIDevice, channel, 0),

> -    DEFINE_PROP_UINT32("scsi-id", SCSIDevice, id, -1),

> -    DEFINE_PROP_UINT32("lun", SCSIDevice, lun, -1),

> -    DEFINE_PROP_END_OF_LIST(),

> -};

> -

> -static void scsi_bus_class_init(ObjectClass *klass, void *data)

> -{

> -    BusClass *k = BUS_CLASS(klass);

> -    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);

> -

> -    k->get_dev_path = scsibus_get_dev_path;

> -    k->get_fw_dev_path = scsibus_get_fw_dev_path;

> -    hc->unplug = qdev_simple_device_unplug_cb;

> -}

> -

> -static const TypeInfo scsi_bus_info = {

> -    .name = TYPE_SCSI_BUS,

> -    .parent = TYPE_BUS,

> -    .instance_size = sizeof(SCSIBus),

> -    .class_init = scsi_bus_class_init,

> -    .interfaces = (InterfaceInfo[]) {

> -        { TYPE_HOTPLUG_HANDLER },

> -        { }

> -    }

> -};

Very very minor nitpick: I'll would do the move of these,
in a separate patch.

>  static int next_scsi_bus;

>  

>  static void scsi_device_realize(SCSIDevice *s, Error **errp)

> @@ -160,35 +133,68 @@ static void scsi_dma_restart_cb(void *opaque, int running, RunState state)

>      }

>  }

>  

> -static void scsi_qdev_realize(DeviceState *qdev, Error **errp)

> +static bool scsi_bus_is_address_free(SCSIBus *bus,

> +				     int channel, int target, int lun,

> +				     SCSIDevice **p_dev)

Alignment wrong on that '(' - we really need to make checkpatch complain
about it.

> +{

> +    SCSIDevice *d = scsi_device_find(bus, channel, target, lun);

> +    if (d && d->lun == lun) {

> +        if (p_dev) {

> +            *p_dev = d;

> +        }

> +        return false;

> +    }

> +    if (p_dev) {

> +        *p_dev = NULL;

> +    }

> +    return true;

> +}

> +

> +static bool scsi_bus_check_address(BusState *qbus, DeviceState *qdev, Error **errp)

>  {

>      SCSIDevice *dev = SCSI_DEVICE(qdev);

> -    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);

> -    SCSIDevice *d;

> -    Error *local_err = NULL;

> +    SCSIBus *bus = SCSI_BUS(qbus);

>  

>      if (dev->channel > bus->info->max_channel) {

>          error_setg(errp, "bad scsi channel id: %d", dev->channel);

> -        return;

> +        return false;

>      }

>      if (dev->id != -1 && dev->id > bus->info->max_target) {

>          error_setg(errp, "bad scsi device id: %d", dev->id);

> -        return;

> +        return false;

>      }

>      if (dev->lun != -1 && dev->lun > bus->info->max_lun) {

>          error_setg(errp, "bad scsi device lun: %d", dev->lun);

> -        return;

> +        return false;

> +    }

> +

> +    if (dev->id != -1 && dev->lun != -1) {

> +        SCSIDevice *d;

> +        if (!scsi_bus_is_address_free(bus, dev->channel, dev->id, dev->lun, &d)) {

> +            error_setg(errp, "lun already used by '%s'", d->qdev.id);

> +            return false;

> +        }

>      }

>  

> +    return true;

> +}

> +

> +static void scsi_qdev_realize(DeviceState *qdev, Error **errp)

> +{

> +    SCSIDevice *dev = SCSI_DEVICE(qdev);

> +    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);

> +    bool is_free;

> +    Error *local_err = NULL;

> +

>      if (dev->id == -1) {

>          int id = -1;

>          if (dev->lun == -1) {

>              dev->lun = 0;

>          }

>          do {

> -            d = scsi_device_find(bus, dev->channel, ++id, dev->lun);

> -        } while (d && d->lun == dev->lun && id < bus->info->max_target);

> -        if (d && d->lun == dev->lun) {

> +            is_free = scsi_bus_is_address_free(bus, dev->channel, ++id, dev->lun, NULL);

> +        } while (!is_free && id < bus->info->max_target);

> +        if (!is_free) {

>              error_setg(errp, "no free target");

>              return;

>          }

> @@ -196,20 +202,13 @@ static void scsi_qdev_realize(DeviceState *qdev, Error **errp)

>      } else if (dev->lun == -1) {

>          int lun = -1;

>          do {

> -            d = scsi_device_find(bus, dev->channel, dev->id, ++lun);

> -        } while (d && d->lun == lun && lun < bus->info->max_lun);

> -        if (d && d->lun == lun) {

> +            is_free = scsi_bus_is_address_free(bus, dev->channel, dev->id, ++lun, NULL);

> +        } while (!is_free && lun < bus->info->max_lun);

> +        if (!is_free) {

>              error_setg(errp, "no free lun");

>              return;

>          }

>          dev->lun = lun;

> -    } else {

> -        d = scsi_device_find(bus, dev->channel, dev->id, dev->lun);

> -        assert(d);

> -        if (d->lun == dev->lun && dev != d) {

> -            error_setg(errp, "lun already used by '%s'", d->qdev.id);

> -            return;

> -        }

>      }

>  

>      QTAILQ_INIT(&dev->requests);

> @@ -1709,6 +1708,13 @@ const VMStateDescription vmstate_scsi_device = {

>      }

>  };

>  

> +static Property scsi_props[] = {

> +    DEFINE_PROP_UINT32("channel", SCSIDevice, channel, 0),

> +    DEFINE_PROP_UINT32("scsi-id", SCSIDevice, id, -1),

> +    DEFINE_PROP_UINT32("lun", SCSIDevice, lun, -1),

> +    DEFINE_PROP_END_OF_LIST(),

> +};

> +

>  static void scsi_device_class_init(ObjectClass *klass, void *data)

>  {

>      DeviceClass *k = DEVICE_CLASS(klass);

> @@ -1739,6 +1745,28 @@ static const TypeInfo scsi_device_type_info = {

>      .instance_init = scsi_dev_instance_init,

>  };

>  

> +static void scsi_bus_class_init(ObjectClass *klass, void *data)

> +{

> +    BusClass *k = BUS_CLASS(klass);

> +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);

> +

> +    k->get_dev_path = scsibus_get_dev_path;

> +    k->get_fw_dev_path = scsibus_get_fw_dev_path;

> +    k->check_address = scsi_bus_check_address;

> +    hc->unplug = qdev_simple_device_unplug_cb;

> +}

> +

> +static const TypeInfo scsi_bus_info = {

> +    .name = TYPE_SCSI_BUS,

> +    .parent = TYPE_BUS,

> +    .instance_size = sizeof(SCSIBus),

> +    .class_init = scsi_bus_class_init,

> +    .interfaces = (InterfaceInfo[]) {

> +        { TYPE_HOTPLUG_HANDLER },

> +        { }

> +    }

> +};

> +

>  static void scsi_register_types(void)

>  {

>      type_register_static(&scsi_bus_info);



With very minor nitpicks, which don't really have to be fixed,
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>


Best regards,
	Maxim Levitsky
diff mbox series

Patch

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 3284a5d1fb..94921c04b1 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -22,33 +22,6 @@  static void scsi_req_dequeue(SCSIRequest *req);
 static uint8_t *scsi_target_alloc_buf(SCSIRequest *req, size_t len);
 static void scsi_target_free_buf(SCSIRequest *req);
 
-static Property scsi_props[] = {
-    DEFINE_PROP_UINT32("channel", SCSIDevice, channel, 0),
-    DEFINE_PROP_UINT32("scsi-id", SCSIDevice, id, -1),
-    DEFINE_PROP_UINT32("lun", SCSIDevice, lun, -1),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void scsi_bus_class_init(ObjectClass *klass, void *data)
-{
-    BusClass *k = BUS_CLASS(klass);
-    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
-
-    k->get_dev_path = scsibus_get_dev_path;
-    k->get_fw_dev_path = scsibus_get_fw_dev_path;
-    hc->unplug = qdev_simple_device_unplug_cb;
-}
-
-static const TypeInfo scsi_bus_info = {
-    .name = TYPE_SCSI_BUS,
-    .parent = TYPE_BUS,
-    .instance_size = sizeof(SCSIBus),
-    .class_init = scsi_bus_class_init,
-    .interfaces = (InterfaceInfo[]) {
-        { TYPE_HOTPLUG_HANDLER },
-        { }
-    }
-};
 static int next_scsi_bus;
 
 static void scsi_device_realize(SCSIDevice *s, Error **errp)
@@ -160,35 +133,68 @@  static void scsi_dma_restart_cb(void *opaque, int running, RunState state)
     }
 }
 
-static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
+static bool scsi_bus_is_address_free(SCSIBus *bus,
+				     int channel, int target, int lun,
+				     SCSIDevice **p_dev)
+{
+    SCSIDevice *d = scsi_device_find(bus, channel, target, lun);
+    if (d && d->lun == lun) {
+        if (p_dev) {
+            *p_dev = d;
+        }
+        return false;
+    }
+    if (p_dev) {
+        *p_dev = NULL;
+    }
+    return true;
+}
+
+static bool scsi_bus_check_address(BusState *qbus, DeviceState *qdev, Error **errp)
 {
     SCSIDevice *dev = SCSI_DEVICE(qdev);
-    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
-    SCSIDevice *d;
-    Error *local_err = NULL;
+    SCSIBus *bus = SCSI_BUS(qbus);
 
     if (dev->channel > bus->info->max_channel) {
         error_setg(errp, "bad scsi channel id: %d", dev->channel);
-        return;
+        return false;
     }
     if (dev->id != -1 && dev->id > bus->info->max_target) {
         error_setg(errp, "bad scsi device id: %d", dev->id);
-        return;
+        return false;
     }
     if (dev->lun != -1 && dev->lun > bus->info->max_lun) {
         error_setg(errp, "bad scsi device lun: %d", dev->lun);
-        return;
+        return false;
+    }
+
+    if (dev->id != -1 && dev->lun != -1) {
+        SCSIDevice *d;
+        if (!scsi_bus_is_address_free(bus, dev->channel, dev->id, dev->lun, &d)) {
+            error_setg(errp, "lun already used by '%s'", d->qdev.id);
+            return false;
+        }
     }
 
+    return true;
+}
+
+static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
+{
+    SCSIDevice *dev = SCSI_DEVICE(qdev);
+    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
+    bool is_free;
+    Error *local_err = NULL;
+
     if (dev->id == -1) {
         int id = -1;
         if (dev->lun == -1) {
             dev->lun = 0;
         }
         do {
-            d = scsi_device_find(bus, dev->channel, ++id, dev->lun);
-        } while (d && d->lun == dev->lun && id < bus->info->max_target);
-        if (d && d->lun == dev->lun) {
+            is_free = scsi_bus_is_address_free(bus, dev->channel, ++id, dev->lun, NULL);
+        } while (!is_free && id < bus->info->max_target);
+        if (!is_free) {
             error_setg(errp, "no free target");
             return;
         }
@@ -196,20 +202,13 @@  static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
     } else if (dev->lun == -1) {
         int lun = -1;
         do {
-            d = scsi_device_find(bus, dev->channel, dev->id, ++lun);
-        } while (d && d->lun == lun && lun < bus->info->max_lun);
-        if (d && d->lun == lun) {
+            is_free = scsi_bus_is_address_free(bus, dev->channel, dev->id, ++lun, NULL);
+        } while (!is_free && lun < bus->info->max_lun);
+        if (!is_free) {
             error_setg(errp, "no free lun");
             return;
         }
         dev->lun = lun;
-    } else {
-        d = scsi_device_find(bus, dev->channel, dev->id, dev->lun);
-        assert(d);
-        if (d->lun == dev->lun && dev != d) {
-            error_setg(errp, "lun already used by '%s'", d->qdev.id);
-            return;
-        }
     }
 
     QTAILQ_INIT(&dev->requests);
@@ -1709,6 +1708,13 @@  const VMStateDescription vmstate_scsi_device = {
     }
 };
 
+static Property scsi_props[] = {
+    DEFINE_PROP_UINT32("channel", SCSIDevice, channel, 0),
+    DEFINE_PROP_UINT32("scsi-id", SCSIDevice, id, -1),
+    DEFINE_PROP_UINT32("lun", SCSIDevice, lun, -1),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void scsi_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *k = DEVICE_CLASS(klass);
@@ -1739,6 +1745,28 @@  static const TypeInfo scsi_device_type_info = {
     .instance_init = scsi_dev_instance_init,
 };
 
+static void scsi_bus_class_init(ObjectClass *klass, void *data)
+{
+    BusClass *k = BUS_CLASS(klass);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
+
+    k->get_dev_path = scsibus_get_dev_path;
+    k->get_fw_dev_path = scsibus_get_fw_dev_path;
+    k->check_address = scsi_bus_check_address;
+    hc->unplug = qdev_simple_device_unplug_cb;
+}
+
+static const TypeInfo scsi_bus_info = {
+    .name = TYPE_SCSI_BUS,
+    .parent = TYPE_BUS,
+    .instance_size = sizeof(SCSIBus),
+    .class_init = scsi_bus_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
+};
+
 static void scsi_register_types(void)
 {
     type_register_static(&scsi_bus_info);