| Message ID | 20120123171525.GM32632@akamai.com |
|---|---|
| State | New |
| Headers | show |
On 01/23/2012 06:15 PM, Thomas Higdon wrote: > This prevents the emulated SCSI device from trying to DMA more bytes to the > initiator than are expected. Without this, the SCRIPTS code in the emulated LSI > device eventually raises a DMA interrupt for a data overrun when an INQUIRY > command whose buflen exceeds req->cmd.xfer is processed. It's the > responsibility of the client to provide a request buffer and allocation > length that are large enough for the result of the command. > > I'm no expert on SCSI (or qemu), but this appears to be more correct behavior > than before, and makes my SCSI INQUIRY commands more successful. Yes, this is correct. SCSI says "The device server shall terminate transfers to the Data-In Buffer when the number of bytes or blocks specified by the ALLOCATION LENGTH field have been transferred or when all available data have been transferred, whichever is less". The same is already done for example for MODE SENSE commands. Can you please also do the same REPORT LUNS and INQUIRY in hw/scsi-bus.c? Thanks, Paolo
On Mon, Jan 23, 2012 at 12:47:54PM -0500, Paolo Bonzini wrote: > On 01/23/2012 06:15 PM, Thomas Higdon wrote: > > This prevents the emulated SCSI device from trying to DMA more bytes to the > > initiator than are expected. Without this, the SCRIPTS code in the emulated LSI > > device eventually raises a DMA interrupt for a data overrun when an INQUIRY > > command whose buflen exceeds req->cmd.xfer is processed. It's the > > responsibility of the client to provide a request buffer and allocation > > length that are large enough for the result of the command. > > > > I'm no expert on SCSI (or qemu), but this appears to be more correct behavior > > than before, and makes my SCSI INQUIRY commands more successful. > > Yes, this is correct. SCSI says "The device server shall terminate > transfers to the Data-In Buffer when the number of bytes or blocks > specified by the ALLOCATION LENGTH field have been transferred or when > all available data have been transferred, whichever is less". > > The same is already done for example for MODE SENSE commands. > > Can you please also do the same REPORT LUNS and INQUIRY in hw/scsi-bus.c? You're talking about the scsi_target_emulate_report_luns() and scsi_target_emulate_inquiry() functions in hw/scsi-bus.c? By my read of the code, these appear safe. In both functions, I see len getting set via calls to MIN with r->req->cmd.xfer as one of the arguments. If you're referring to something else, can you be more specific?
On 01/23/2012 07:14 PM, Thomas Higdon wrote: > > Can you please also do the same REPORT LUNS and INQUIRY in hw/scsi-bus.c? > > You're talking about the scsi_target_emulate_report_luns() and > scsi_target_emulate_inquiry() functions in hw/scsi-bus.c? By my read of > the code, these appear safe. In both functions, I see len getting set > via calls to MIN with r->req->cmd.xfer as one of the arguments. If > you're referring to something else, can you be more specific? Ugh, you're right, sorry. I just looked for if.*xfer. Paolo
Am 23.01.2012 18:15, schrieb Thomas Higdon: > This prevents the emulated SCSI device from trying to DMA more bytes to the > initiator than are expected. Without this, the SCRIPTS code in the emulated LSI > device eventually raises a DMA interrupt for a data overrun when an INQUIRY > command whose buflen exceeds req->cmd.xfer is processed. It's the > responsibility of the client to provide a request buffer and allocation > length that are large enough for the result of the command. > > I'm no expert on SCSI (or qemu), but this appears to be more correct behavior > than before, and makes my SCSI INQUIRY commands more successful. > > Before patch: > > # echo setverbose 2 > /proc/scsi/sym53c8xx/0 > # sginfo -s -TT /dev/sda > cdb: 12 00 00 00 24 00 > inquiry response: > 00 00 05 02 1f 00 00 12 51 45 4d 55 20 20 20 20 > 51 45 4d 55 20 48 41 52 44 44 49 53 4b 20 20 20 > 31 2e 30 2e > cdb: 12 01 00 00 04 00 > [ 237.014083] sd 0:0:0:0: extraneous data discarded. > [ 237.014437] sd 0:0:0:0: COMMAND FAILED (87 0 1). > do_scsi_io: opcode=0x12: Host_status=0x07 [DID_ERROR] > No serial number (error doing INQUIRY, supported VPDs) > > After: > > # echo setverbose 2 > /proc/scsi/sym53c8xx/0 > # sginfo -s -TT /dev/sda > cdb: 12 00 00 00 24 00 > inquiry response: > 00 00 05 02 1f 00 00 12 51 45 4d 55 20 20 20 20 > 51 45 4d 55 20 48 41 52 44 44 49 53 4b 20 20 20 > 31 2e 30 2e > cdb: 12 01 00 00 04 00 > cdb: 12 01 80 00 04 00 > cdb: 12 01 80 00 06 00 > inquiry (evpd page (0x80) response: > 00 80 00 02 32 37 > Serial Number '27' > > Command line: > /usr/bin/qemu-system-x86_64 -S -M pc -enable-kvm -m 1024 -smp 1,sockets=1,cores=1,threads=1 -nodefconfig -nodefaults -chardev socket,id=charmonitor,path=/var/lib/libvirt/qemu/monitor,server,nowait -mon chardev=charmonitor,id=monitor,mode=readline -rtc base=utc -boot d -device lsi,id=scsi0,bus=pci.0,addr=0x5 -drive file=disk.0,if=none,id=drive-scsi0-0-0,format=qcow2 -device scsi-disk,bus=scsi0.0,scsi-id=0,drive=drive-scsi0-0-0,id=scsi0-0-0,serial=27 -drive file=disk.2,if=none,media=cdrom,id=drive-ide0-0-0,readonly=on,format=raw -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 -usb -vnc 0.0.0.0:80 -vga cirrus > > Signed-off-by: Thomas Higdon <thigdon@akamai.com> > --- > hw/scsi-disk.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c > index 5d8bf53..71fe2a3 100644 > --- a/hw/scsi-disk.c > +++ b/hw/scsi-disk.c > @@ -477,6 +477,9 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf) > "buffer size %zd\n", page_code, req->cmd.xfer); > return -1; > } > + if (buflen > req->cmd.xfer) { > + buflen = req->cmd.xfer; > + } > /* done with EVPD */ > return buflen; > } I wonder if it would make sense to make this check in a more central place like scsi_send_command(). This way we would avoid similar bugs in other commands. Kevin
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index 5d8bf53..71fe2a3 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -477,6 +477,9 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf) "buffer size %zd\n", page_code, req->cmd.xfer); return -1; } + if (buflen > req->cmd.xfer) { + buflen = req->cmd.xfer; + } /* done with EVPD */ return buflen; }
This prevents the emulated SCSI device from trying to DMA more bytes to the initiator than are expected. Without this, the SCRIPTS code in the emulated LSI device eventually raises a DMA interrupt for a data overrun when an INQUIRY command whose buflen exceeds req->cmd.xfer is processed. It's the responsibility of the client to provide a request buffer and allocation length that are large enough for the result of the command. I'm no expert on SCSI (or qemu), but this appears to be more correct behavior than before, and makes my SCSI INQUIRY commands more successful. Before patch: # echo setverbose 2 > /proc/scsi/sym53c8xx/0 # sginfo -s -TT /dev/sda cdb: 12 00 00 00 24 00 inquiry response: 00 00 05 02 1f 00 00 12 51 45 4d 55 20 20 20 20 51 45 4d 55 20 48 41 52 44 44 49 53 4b 20 20 20 31 2e 30 2e cdb: 12 01 00 00 04 00 [ 237.014083] sd 0:0:0:0: extraneous data discarded. [ 237.014437] sd 0:0:0:0: COMMAND FAILED (87 0 1). do_scsi_io: opcode=0x12: Host_status=0x07 [DID_ERROR] No serial number (error doing INQUIRY, supported VPDs) After: # echo setverbose 2 > /proc/scsi/sym53c8xx/0 # sginfo -s -TT /dev/sda cdb: 12 00 00 00 24 00 inquiry response: 00 00 05 02 1f 00 00 12 51 45 4d 55 20 20 20 20 51 45 4d 55 20 48 41 52 44 44 49 53 4b 20 20 20 31 2e 30 2e cdb: 12 01 00 00 04 00 cdb: 12 01 80 00 04 00 cdb: 12 01 80 00 06 00 inquiry (evpd page (0x80) response: 00 80 00 02 32 37 Serial Number '27' Command line: /usr/bin/qemu-system-x86_64 -S -M pc -enable-kvm -m 1024 -smp 1,sockets=1,cores=1,threads=1 -nodefconfig -nodefaults -chardev socket,id=charmonitor,path=/var/lib/libvirt/qemu/monitor,server,nowait -mon chardev=charmonitor,id=monitor,mode=readline -rtc base=utc -boot d -device lsi,id=scsi0,bus=pci.0,addr=0x5 -drive file=disk.0,if=none,id=drive-scsi0-0-0,format=qcow2 -device scsi-disk,bus=scsi0.0,scsi-id=0,drive=drive-scsi0-0-0,id=scsi0-0-0,serial=27 -drive file=disk.2,if=none,media=cdrom,id=drive-ide0-0-0,readonly=on,format=raw -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 -usb -vnc 0.0.0.0:80 -vga cirrus Signed-off-by: Thomas Higdon <thigdon@akamai.com> --- hw/scsi-disk.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)