On Tue, May 02, 2023 at 05:19:46PM +0200, Kevin Wolf wrote:
> Am 25.04.2023 um 19:26 hat Stefan Hajnoczi geschrieben:
> > Only report a transport reset event to the guest after the SCSIDevice
> > has been unrealized by qdev_simple_device_unplug_cb().
> >
> > qdev_simple_device_unplug_cb() sets the SCSIDevice's qdev.realized field
> > to false so that scsi_device_find/get() no longer see it.
> >
> > scsi_target_emulate_report_luns() also needs to be updated to filter out
> > SCSIDevices that are unrealized.
> >
> > These changes ensure that the guest driver does not see the SCSIDevice
> > that's being unplugged if it responds very quickly to the transport
> > reset event.
> >
> > Reviewed-by: Paolo Bonzini <pbonz...@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
> > Reviewed-by: Daniil Tatianin <d-tatia...@yandex-team.ru>
> > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
>
> > @@ -1082,6 +1073,15 @@ static void virtio_scsi_hotunplug(HotplugHandler
> > *hotplug_dev, DeviceState *dev,
> > blk_set_aio_context(sd->conf.blk, qemu_get_aio_context(), NULL);
> > virtio_scsi_release(s);
> > }
> > +
> > + if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) {
> > + virtio_scsi_acquire(s);
> > + virtio_scsi_push_event(s, sd,
> > + VIRTIO_SCSI_T_TRANSPORT_RESET,
> > + VIRTIO_SCSI_EVT_RESET_REMOVED);
> > + scsi_bus_set_ua(&s->bus, SENSE_CODE(REPORTED_LUNS_CHANGED));
> > + virtio_scsi_release(s);
> > + }
> > }
>
> s, sd and s->bus are all unrealized at this point, whereas before this
> patch they were still realized. I couldn't find any practical problem
> with it, but it made me nervous enough that I thought I should comment
> on it at least.
>
> Should we maybe have documentation on these functions that says that
> they accept unrealized objects as their parameters?s is the VirtIOSCSI controller, not the SCSIDevice that is being unplugged. The VirtIOSCSI controller is still realized. s->bus is the VirtIOSCSI controller's bus, it is still realized. You are right that the SCSIDevice (sd) has been unrealized at this point: - sd->conf.blk is safe because qdev properties stay alive the Object is deleted, but I'm not sure we should rely on that. - virti_scsi_push_event(.., sd, ...) is questionable because the LUN that's fetched from sd no longer belongs to the unplugged SCSIDevice. How about I change the code to fetch sd->conf.blk and the LUN before unplugging? Stefan
signature.asc
Description: PGP signature
