On Fri, 2020-09-25 at 13:25 -0400, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonz...@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 <mlevi...@redhat.com>
Best regards,
Maxim Levitsky
