| Message ID | 20230712134352.118655-2-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | scsi: fix issue with Linux guest and unit attention | expand |
On 7/12/23 15:43, Stefano Garzarella wrote: > Commit 1880ad4f4e ("virtio-scsi: Batched prepare for cmd reqs") split > calls to scsi_req_new() and scsi_req_enqueue() in the virtio-scsi device. > This had no drawback, until commit 8cc5583abe ("virtio-scsi: Send More precisely, it was pretty hard to trigger it; it might be possible using a CD-ROM, as it can report a MEDIUM_CHANGED unit attention. I will change "This had no drawback" to "No ill effect was reported" > "REPORTED LUNS CHANGED" sense data upon disk hotplug events") added a > bus unit attention. ... that was fairly easy to trigger via SCSI device hotplug/hot-unplug. Queued the series, thanks for the tests and for applying the cleanups on top. > Co-developed-by: Paolo Bonzini <pbonzini@redhat.com> Heh, I basically only wrote the "if (req->init_req)" statement so that's pretty generous, but I'll keep it anyway. :) Paolo > Having the two calls separated, all requests in the batch were prepared > calling scsi_req_new() to report a sense. > Then only the first one submitted calling scsi_req_enqueue() reported the > right sense and reset it to NO_SENSE. > The others reported NO_SENSE, causing SCSI errors in Linux.
On Wed, Jul 12, 2023 at 06:38:14PM +0200, Paolo Bonzini wrote: >On 7/12/23 15:43, Stefano Garzarella wrote: >>Commit 1880ad4f4e ("virtio-scsi: Batched prepare for cmd reqs") split >>calls to scsi_req_new() and scsi_req_enqueue() in the virtio-scsi device. >>This had no drawback, until commit 8cc5583abe ("virtio-scsi: Send > >More precisely, it was pretty hard to trigger it; it might be possible >using a CD-ROM, as it can report a MEDIUM_CHANGED unit attention. I >will change "This had no drawback" to "No ill effect was reported" Yep, agree! > >>"REPORTED LUNS CHANGED" sense data upon disk hotplug events") added a >>bus unit attention. > >... that was fairly easy to trigger via SCSI device hotplug/hot-unplug. > >Queued the series, thanks for the tests and for applying the cleanups >on top. Thanks for queueing! > >>Co-developed-by: Paolo Bonzini <pbonzini@redhat.com> > >Heh, I basically only wrote the "if (req->init_req)" statement so >that's pretty generous, but I'll keep it anyway. :) Your help was invaluable ;-) Thanks, Stefano
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h index e2bb1a2fbf..3692ca82f3 100644 --- a/include/hw/scsi/scsi.h +++ b/include/hw/scsi/scsi.h @@ -108,6 +108,7 @@ int cdrom_read_toc_raw(int nb_sectors, uint8_t *buf, int msf, int session_num); /* scsi-bus.c */ struct SCSIReqOps { size_t size; + void (*init_req)(SCSIRequest *req); void (*free_req)(SCSIRequest *req); int32_t (*send_command)(SCSIRequest *req, uint8_t *buf); void (*read_data)(SCSIRequest *req); diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index f80f4cb4fc..f083373021 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -412,19 +412,35 @@ static const struct SCSIReqOps reqops_invalid_opcode = { /* SCSIReqOps implementation for unit attention conditions. */ -static int32_t scsi_unit_attention(SCSIRequest *req, uint8_t *buf) +static void scsi_fetch_unit_attention_sense(SCSIRequest *req) { + SCSISense *ua = NULL; + if (req->dev->unit_attention.key == UNIT_ATTENTION) { - scsi_req_build_sense(req, req->dev->unit_attention); + ua = &req->dev->unit_attention; } else if (req->bus->unit_attention.key == UNIT_ATTENTION) { - scsi_req_build_sense(req, req->bus->unit_attention); + ua = &req->bus->unit_attention; } + + /* + * Fetch the unit attention sense immediately so that another + * scsi_req_new does not use reqops_unit_attention. + */ + if (ua) { + scsi_req_build_sense(req, *ua); + *ua = SENSE_CODE(NO_SENSE); + } +} + +static int32_t scsi_unit_attention(SCSIRequest *req, uint8_t *buf) +{ scsi_req_complete(req, CHECK_CONDITION); return 0; } static const struct SCSIReqOps reqops_unit_attention = { .size = sizeof(SCSIRequest), + .init_req = scsi_fetch_unit_attention_sense, .send_command = scsi_unit_attention }; @@ -699,6 +715,11 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d, object_ref(OBJECT(d)); object_ref(OBJECT(qbus->parent)); notifier_list_init(&req->cancel_notifiers); + + if (reqops->init_req) { + reqops->init_req(req); + } + trace_scsi_req_alloc(req->dev->id, req->lun, req->tag); return req; } @@ -798,6 +819,15 @@ uint8_t *scsi_req_get_buf(SCSIRequest *req) static void scsi_clear_unit_attention(SCSIRequest *req) { SCSISense *ua; + + /* + * scsi_fetch_unit_attention_sense() already cleaned the unit attention + * in this case. + */ + if (req->ops == &reqops_unit_attention) { + return; + } + if (req->dev->unit_attention.key != UNIT_ATTENTION && req->bus->unit_attention.key != UNIT_ATTENTION) { return;
Commit 1880ad4f4e ("virtio-scsi: Batched prepare for cmd reqs") split calls to scsi_req_new() and scsi_req_enqueue() in the virtio-scsi device. This had no drawback, until commit 8cc5583abe ("virtio-scsi: Send "REPORTED LUNS CHANGED" sense data upon disk hotplug events") added a bus unit attention. Having the two calls separated, all requests in the batch were prepared calling scsi_req_new() to report a sense. Then only the first one submitted calling scsi_req_enqueue() reported the right sense and reset it to NO_SENSE. The others reported NO_SENSE, causing SCSI errors in Linux. To solve this issue, let's fetch the unit attention as early as possible when we prepare the request, that way only the first request in the batch will carry the right sense. Fixes: 1880ad4f4e ("virtio-scsi: Batched prepare for cmd reqs") Fixes: 8cc5583abe ("virtio-scsi: Send "REPORTED LUNS CHANGED" sense data upon disk hotplug events") Reported-by: Thomas Huth <thuth@redhat.com> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2176702 Co-developed-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- include/hw/scsi/scsi.h | 1 + hw/scsi/scsi-bus.c | 36 +++++++++++++++++++++++++++++++++--- 2 files changed, 34 insertions(+), 3 deletions(-)