Hi all,
this are the preparatory bits for virtio-scsi support in SeaBIOS. They mostly aim at making code more generic in SeaBIOS for both SCSI and virtio, but (on the SCSI side) they also fix some bugs, so I'm sending them early to get more exposure.
New features are mostly visible on the USB side since that's currently the main consumer of the SCSI implementation. They include more robust LUN detection, support for hard disk geometry, and USB mass storage write support.
I also fixed for real the race that the RFC series was working around.
For virtio, the series adds a few extra interface bits that are common to both virtio-scsi and virtio-blk.
The two parts are logically separate but they have a small conflict, so I'm sending them together.
Paolo Bonzini (16): cdrom: use TEST UNIT READY to detect ready medium usb-uhci: reorganize wait_qh into wait_pipe usb-uhci: fix race against host controller usb-msc: support commands without payload usb-msc: add usb_msc_send usb-msc: support WRITE commands usb-msc: move READ CAPACITY to usb_msc_init, fix off-by-one usb-msc: pass drive to setup_drive_* usb-msc: rename INQUIRY types usb-msc: go through TEST UNIT READY for hard disks. usb-msc: move common scsi code to blockcmd.c usb-msc: move cdb dispatch to block.c scsi: get physical chs geometry from mode page 0x04 always specify virtio-blk rather than virtio virtio-pci: introduce vp_init_simple virtio-pci: allocate vq in vp_find_vq
v1->v2: do not send commands to CDs until they are actually selected for boot; fix usb-uhci race for real; virtio-scsi driver not yet included.
src/Kconfig | 4 +- src/block.c | 34 ++++++++++-- src/blockcmd.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/blockcmd.h | 42 ++++++++++++++- src/cdrom.c | 58 +------------------- src/disk.c | 4 +- src/disk.h | 18 +++--- src/usb-msc.c | 132 ++++++++++++++------------------------------- src/usb-msc.h | 3 - src/usb-uhci.c | 91 +++++++++++++++---------------- src/virtio-blk.c | 25 +++------ src/virtio-blk.h | 2 +- src/virtio-pci.c | 33 ++++++++++-- src/virtio-pci.h | 3 +- 14 files changed, 364 insertions(+), 242 deletions(-)
The READ CAPACITY output is not used except for some debugging messages. In the future, we will use this code for USB sticks too, but those already send READ CAPACITY. To avoid code duplication, switch to TEST UNIT READY for this task.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/blockcmd.c | 12 ++++++++++++ src/blockcmd.h | 6 ++++-- src/cdrom.c | 16 +++------------- 3 files changed, 19 insertions(+), 15 deletions(-)
diff --git a/src/blockcmd.c b/src/blockcmd.c index c9c6845..c127729 100644 --- a/src/blockcmd.c +++ b/src/blockcmd.c @@ -56,6 +56,18 @@ cdb_get_sense(struct disk_op_s *op, struct cdbres_request_sense *data) return cdb_cmd_data(op, &cmd, sizeof(*data)); }
+// Test unit ready +int +cdb_test_unit_ready(struct disk_op_s *op) +{ + struct cdb_request_sense cmd; + memset(&cmd, 0, sizeof(cmd)); + cmd.command = CDB_CMD_TEST_UNIT_READY; + op->count = 0; + op->buf_fl = NULL; + return cdb_cmd_data(op, &cmd, 0); +} + // Request capacity int cdb_read_capacity(struct disk_op_s *op, struct cdbres_read_capacity *data) diff --git a/src/blockcmd.h b/src/blockcmd.h index 903c435..49921b0 100644 --- a/src/blockcmd.h +++ b/src/blockcmd.h @@ -32,8 +32,9 @@ struct cdbres_read_capacity { u32 blksize; } PACKED;
-#define CDB_CMD_INQUIRY 0x12 -#define CDB_CMD_REQUEST_SENSE 0x03 +#define CDB_CMD_TEST_UNIT_READY 0x00 +#define CDB_CMD_INQUIRY 0x12 +#define CDB_CMD_REQUEST_SENSE 0x03
struct cdb_request_sense { u8 command; @@ -70,6 +71,7 @@ struct cdbres_inquiry { // blockcmd.c int cdb_get_inquiry(struct disk_op_s *op, struct cdbres_inquiry *data); int cdb_get_sense(struct disk_op_s *op, struct cdbres_request_sense *data); +int cdb_test_unit_ready(struct disk_op_s *op); int cdb_read_capacity(struct disk_op_s *op, struct cdbres_read_capacity *data); int cdb_inquiry(struct disk_op_s *op, struct cdbres_inquiry *data); int cdb_read(struct disk_op_s *op); diff --git a/src/cdrom.c b/src/cdrom.c index 6351fec..b817999 100644 --- a/src/cdrom.c +++ b/src/cdrom.c @@ -189,19 +189,18 @@ atapi_is_ready(struct disk_op_s *op) { dprintf(6, "atapi_is_ready (drive=%p)\n", op->drive_g);
- /* Retry READ CAPACITY for 5 seconds unless MEDIUM NOT PRESENT is + /* Retry TEST UNIT READY for 5 seconds unless MEDIUM NOT PRESENT is * reported by the device. If the device reports "IN PROGRESS", * 30 seconds is added. */ - struct cdbres_read_capacity info; int in_progress = 0; u64 end = calc_future_tsc(5000); for (;;) { if (check_tsc(end)) { - dprintf(1, "read capacity failed\n"); + dprintf(1, "test unit ready failed\n"); return -1; }
- int ret = cdb_read_capacity(op, &info); + int ret = cdb_test_unit_ready(op); if (!ret) // Success break; @@ -226,15 +225,6 @@ atapi_is_ready(struct disk_op_s *op) in_progress = 1; } } - - u32 blksize = ntohl(info.blksize), sectors = ntohl(info.sectors); - if (blksize != GET_GLOBAL(op->drive_g->blksize)) { - printf("Unsupported sector size %u\n", blksize); - return -1; - } - - dprintf(6, "sectors=%u\n", sectors); - printf("%dMB medium detected\n", sectors>>(20-11)); return 0; }
Three changes:
1) Add explicit GET_FLATPTR/SET_FLATPTR.
2) Pass the whole pipe to wait_qh so that we can get the iobase from there.
3) Clean up the pipe upon timeout, since that is the only sensible thing to do: tds are on the stack, and leaving pointers to them in the pipe is not a good idea.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/usb-uhci.c | 65 ++++++++++++++++++++++++++----------------------------- 1 files changed, 31 insertions(+), 34 deletions(-)
diff --git a/src/usb-uhci.c b/src/usb-uhci.c index f3680d3..5957567 100644 --- a/src/usb-uhci.c +++ b/src/usb-uhci.c @@ -212,26 +212,13 @@ uhci_init(struct pci_device *pci, int busid) * End point communication ****************************************************************/
-static int -wait_qh(struct usb_uhci_s *cntl, struct uhci_qh *qh) -{ - // XXX - 500ms just a guess - u64 end = calc_future_tsc(500); - for (;;) { - if (qh->element & UHCI_PTR_TERM) - return 0; - if (check_tsc(end)) { - warn_timeout(); - struct uhci_td *td = (void*)(qh->element & ~UHCI_PTR_BITS); - dprintf(1, "Timeout on wait_qh %p (td=%p s=%x c=%x/%x)\n" - , qh, td, td->status - , inw(cntl->iobase + USBCMD) - , inw(cntl->iobase + USBSTS)); - return -1; - } - yield(); - } -} +struct uhci_pipe { + struct uhci_qh qh; + struct uhci_td *next_td; + struct usb_pipe pipe; + u16 iobase; + u8 toggle; +};
// Wait for next USB frame to start - for ensuring safe memory release. static void @@ -251,13 +238,29 @@ uhci_waittick(u16 iobase) } }
-struct uhci_pipe { - struct uhci_qh qh; - struct uhci_td *next_td; - struct usb_pipe pipe; - u16 iobase; - u8 toggle; -}; +static int +wait_pipe(struct uhci_pipe *pipe) +{ + // XXX - 500ms just a guess + u64 end = calc_future_tsc(500); + for (;;) { + u32 el_link = GET_FLATPTR(pipe->qh.element); + if (el_link & UHCI_PTR_TERM) + return 0; + if (check_tsc(end)) { + warn_timeout(); + struct uhci_td *td = (void*)(el_link & ~UHCI_PTR_BITS); + dprintf(1, "Timeout on wait_pipe %p (td=%p s=%x c=%x/%x)\n" + , pipe, (void*)el_link, GET_FLATPTR(td->status) + , inw(pipe->iobase + USBCMD) + , inw(pipe->iobase + USBSTS)); + SET_FLATPTR(pipe->qh.element, UHCI_PTR_TERM); + uhci_waittick(pipe->iobase); + return -1; + } + yield(); + } +}
void uhci_free_pipe(struct usb_pipe *p) @@ -331,8 +334,6 @@ uhci_control(struct usb_pipe *p, int dir, const void *cmd, int cmdsize return -1; dprintf(5, "uhci_control %p\n", p); struct uhci_pipe *pipe = container_of(p, struct uhci_pipe, pipe); - struct usb_uhci_s *cntl = container_of( - pipe->pipe.cntl, struct usb_uhci_s, usb);
int maxpacket = pipe->pipe.maxpacket; int lowspeed = pipe->pipe.speed; @@ -376,11 +377,7 @@ uhci_control(struct usb_pipe *p, int dir, const void *cmd, int cmdsize // Transfer data barrier(); pipe->qh.element = (u32)&tds[0]; - int ret = wait_qh(cntl, &pipe->qh); - if (ret) { - pipe->qh.element = UHCI_PTR_TERM; - uhci_waittick(pipe->iobase); - } + int ret = wait_pipe(pipe); free(tds); return ret; }
Four changes:
1) Add explicit GET_FLATPTR/SET_FLATPTR.
2) Pass the whole pipe to wait_qh so that we can get the iobase from there.
3) Clean up the pipe upon timeout, since that is the only sensible thing to do: tds are on the stack, and leaving pointers to them in the pipe is not a good idea.
4) Add a variable timeout argument, since bulk transfers might take more than 500 ms.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- v1->v2: prepare for passing a bigger timeout
src/usb-uhci.c | 64 ++++++++++++++++++++++++++----------------------------- 1 files changed, 30 insertions(+), 34 deletions(-)
diff --git a/src/usb-uhci.c b/src/usb-uhci.c index f3680d3..4702695 100644 --- a/src/usb-uhci.c +++ b/src/usb-uhci.c @@ -212,26 +212,13 @@ uhci_init(struct pci_device *pci, int busid) * End point communication ****************************************************************/
-static int -wait_qh(struct usb_uhci_s *cntl, struct uhci_qh *qh) -{ - // XXX - 500ms just a guess - u64 end = calc_future_tsc(500); - for (;;) { - if (qh->element & UHCI_PTR_TERM) - return 0; - if (check_tsc(end)) { - warn_timeout(); - struct uhci_td *td = (void*)(qh->element & ~UHCI_PTR_BITS); - dprintf(1, "Timeout on wait_qh %p (td=%p s=%x c=%x/%x)\n" - , qh, td, td->status - , inw(cntl->iobase + USBCMD) - , inw(cntl->iobase + USBSTS)); - return -1; - } - yield(); - } -} +struct uhci_pipe { + struct uhci_qh qh; + struct uhci_td *next_td; + struct usb_pipe pipe; + u16 iobase; + u8 toggle; +};
// Wait for next USB frame to start - for ensuring safe memory release. static void @@ -251,13 +238,28 @@ uhci_waittick(u16 iobase) } }
-struct uhci_pipe { - struct uhci_qh qh; - struct uhci_td *next_td; - struct usb_pipe pipe; - u16 iobase; - u8 toggle; -}; +static int +wait_pipe(struct uhci_pipe *pipe, int timeout) +{ + u64 end = calc_future_tsc(timeout); + for (;;) { + u32 el_link = GET_FLATPTR(pipe->qh.element); + if (el_link & UHCI_PTR_TERM) + return 0; + if (check_tsc(end)) { + warn_timeout(); + struct uhci_td *td = (void*)(el_link & ~UHCI_PTR_BITS); + dprintf(1, "Timeout on wait_pipe %p (td=%p s=%x c=%x/%x)\n" + , pipe, (void*)el_link, GET_FLATPTR(td->status) + , inw(pipe->iobase + USBCMD) + , inw(pipe->iobase + USBSTS)); + SET_FLATPTR(pipe->qh.element, UHCI_PTR_TERM); + uhci_waittick(pipe->iobase); + return -1; + } + yield(); + } +}
void uhci_free_pipe(struct usb_pipe *p) @@ -331,8 +333,6 @@ uhci_control(struct usb_pipe *p, int dir, const void *cmd, int cmdsize return -1; dprintf(5, "uhci_control %p\n", p); struct uhci_pipe *pipe = container_of(p, struct uhci_pipe, pipe); - struct usb_uhci_s *cntl = container_of( - pipe->pipe.cntl, struct usb_uhci_s, usb);
int maxpacket = pipe->pipe.maxpacket; int lowspeed = pipe->pipe.speed; @@ -376,11 +376,7 @@ uhci_control(struct usb_pipe *p, int dir, const void *cmd, int cmdsize // Transfer data barrier(); pipe->qh.element = (u32)&tds[0]; - int ret = wait_qh(cntl, &pipe->qh); - if (ret) { - pipe->qh.element = UHCI_PTR_TERM; - uhci_waittick(pipe->iobase); - } + int ret = wait_pipe(pipe, 500); free(tds); return ret; }
While processing a frame, the host controller will write to the queue head's element link field. The following sequence could then happen when two consecutive sends occur to the same pipe.
controller SeaBIOS --------------------------------------------------------------------- td->link = UHCI_PTR_TERM; td->ctrl |= TD_CTRL_ACTIVE; read TD from memory wait_td(td); td->ctrl &= ~TD_CTRL_ACTIVE; write back td->ctrl exit usb_send_bulk restart usb_send_bulk pipe->qh.element = &tds; pipe->qh.element = td->link; ... go on and set up the first td ... write back pipe->qh.element td->ctrl |= TD_CTRL_ACTIVE;
Once the host controller has written UHCI_PTR_TERM to the element link, subsequent tds would never be processed. This is surprisingly frequent when the two consecutive sends are in the OUT direction (and just as surprisingly, it seems like it never happens in the IN direction).
To fix this, re-enable TDs whenever the host controller completes the pending ones. Also, at the end of the processing do not wait for each single TD to become inactive, but simply wait for the host controller to invalidate the element link.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/usb-uhci.c | 25 ++++++++++++------------- 1 files changed, 12 insertions(+), 13 deletions(-)
diff --git a/src/usb-uhci.c b/src/usb-uhci.c index 5957567..84153a7 100644 --- a/src/usb-uhci.c +++ b/src/usb-uhci.c @@ -455,11 +455,8 @@ uhci_send_bulk(struct usb_pipe *p, int dir, void *data, int datasize) struct uhci_td *tds = (void*)ALIGN((u32)tdsbuf, TDALIGN); memset(tds, 0, sizeof(*tds) * STACKTDS);
- // Enable tds - barrier(); - SET_FLATPTR(pipe->qh.element, (u32)MAKE_FLATPTR(GET_SEG(SS), tds)); - int tdpos = 0; + struct uhci_td *start_td = tds; while (datasize) { struct uhci_td *td = &tds[tdpos++ % STACKTDS]; int ret = wait_td(td); @@ -479,21 +476,23 @@ uhci_send_bulk(struct usb_pipe *p, int dir, void *data, int datasize) barrier(); td->status = (uhci_maxerr(3) | (lowspeed ? TD_CTRL_LS : 0) | TD_CTRL_ACTIVE); + if (GET_FLATPTR(pipe->qh.element) & UHCI_PTR_TERM) { + // Enable tds. The previous td might be the first one, since + // there is still a race window between reading and writing + // pipe->qh.element (we do not use CMPXCHG). start_td ensures + // that what we write will always be linked to the first active td. + barrier(); + SET_FLATPTR(pipe->qh.element, (u32)MAKE_FLATPTR(GET_SEG(SS) + , start_td)); + } toggle ^= TD_TOKEN_TOGGLE;
data += transfer; datasize -= transfer; + start_td = td; } - int i; - for (i=0; i<STACKTDS; i++) { - struct uhci_td *td = &tds[tdpos++ % STACKTDS]; - int ret = wait_td(td); - if (ret) - goto fail; - } - SET_FLATPTR(pipe->toggle, !!toggle); - return 0; + return wait_pipe(pipe); fail: dprintf(1, "uhci_send_bulk failed\n"); SET_FLATPTR(pipe->qh.element, UHCI_PTR_TERM);
On Wed, Nov 16, 2011 at 01:02:44PM +0100, Paolo Bonzini wrote:
While processing a frame, the host controller will write to the queue head's element link field. The following sequence could then happen when two consecutive sends occur to the same pipe.
controller SeaBIOS --------------------------------------------------------------------- td->link = UHCI_PTR_TERM; td->ctrl |= TD_CTRL_ACTIVE; read TD from memory wait_td(td); td->ctrl &= ~TD_CTRL_ACTIVE; write back td->ctrl exit usb_send_bulk restart usb_send_bulk pipe->qh.element = &tds; pipe->qh.element = td->link; ... go on and set up the first td ... write back pipe->qh.element td->ctrl |= TD_CTRL_ACTIVE;Once the host controller has written UHCI_PTR_TERM to the element link, subsequent tds would never be processed. This is surprisingly frequent when the two consecutive sends are in the OUT direction (and just as surprisingly, it seems like it never happens in the IN direction).
Interesting. Good catch! If I understand this correctly, the issue occurs because all TDs being complete doesn't guarentee that the QH is complete, and thus the next transfer races with the controller on the QH update.
To fix this, re-enable TDs whenever the host controller completes the pending ones. Also, at the end of the processing do not wait for each single TD to become inactive, but simply wait for the host controller to invalidate the element link.
[...]
- // Enable tds
- barrier();
- SET_FLATPTR(pipe->qh.element, (u32)MAKE_FLATPTR(GET_SEG(SS), tds));
- int tdpos = 0;
- struct uhci_td *start_td = tds; while (datasize) { struct uhci_td *td = &tds[tdpos++ % STACKTDS]; int ret = wait_td(td);
@@ -479,21 +476,23 @@ uhci_send_bulk(struct usb_pipe *p, int dir, void *data, int datasize) barrier(); td->status = (uhci_maxerr(3) | (lowspeed ? TD_CTRL_LS : 0) | TD_CTRL_ACTIVE);
if (GET_FLATPTR(pipe->qh.element) & UHCI_PTR_TERM) {// Enable tds. The previous td might be the first one, since// there is still a race window between reading and writing// pipe->qh.element (we do not use CMPXCHG). start_td ensures// that what we write will always be linked to the first active td.barrier();SET_FLATPTR(pipe->qh.element, (u32)MAKE_FLATPTR(GET_SEG(SS), start_td));}
Can't the code just use wait_pipe() at the end to eliminate the race? Why does the code need to recheck for qh.element==UHCI_PTR_TERM in the loop? The host should never modify qh.element, and the controller can only assign qh.element=td->link. So, the only way qh.element can be UHCI_PTR_TERM is if the transfer is complete (in which case we don't want to start it again) or the transfer is just beginning (which can be setup at the start of the transfer instead of in the loop). Am I missing something?
[...]
- int i;
- for (i=0; i<STACKTDS; i++) {
struct uhci_td *td = &tds[tdpos++ % STACKTDS];int ret = wait_td(td);if (ret)goto fail;- }
- SET_FLATPTR(pipe->toggle, !!toggle);
- return 0;
- return wait_pipe(pipe);
I agree this is better, but the timeouts have changed. Bulk transfers need more time than 500ms to complete.
Thanks for working on this. -Kevin
PS - the patch series looks good to me.
On 11/17/2011 05:53 AM, Kevin O'Connor wrote:
Interesting. Good catch! If I understand this correctly, the issue occurs because all TDs being complete doesn't guarentee that the QH is complete, and thus the next transfer races with the controller on the QH update.
Yep.
Can't the code just use wait_pipe() at the end to eliminate the race?
Yes, that's correct. The previous call to usb_send_bulk guarantees that the QH is quiescent. It still works (tested with 15 consecutive boots, usually it fails in 2-3).
I'll reply to each modified individual patch with the v2, there's just a handful of them.
Paolo
While processing a frame, the host controller will write to the queue head's element link field. The following sequence could then happen when two consecutive sends occur to the same pipe.
controller SeaBIOS --------------------------------------------------------------------- td->link = UHCI_PTR_TERM; td->ctrl |= TD_CTRL_ACTIVE; read TD from memory wait_td(td); td->ctrl &= ~TD_CTRL_ACTIVE; write back td->ctrl exit usb_send_bulk restart usb_send_bulk pipe->qh.element = &tds; pipe->qh.element = td->link; ... go on and set up the first td ... write back pipe->qh.element td->ctrl |= TD_CTRL_ACTIVE;
Once the host controller has written UHCI_PTR_TERM to the element link, subsequent tds would never be processed. This is surprisingly frequent when the two consecutive sends are in the OUT direction (and just as surprisingly, it seems like it never happens in the IN direction).
To fix this, at the end of the processing do not wait for each single TD to become inactive, but for the host controller to invalidate the element link (which implies it's done with all TDs).
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- Thanks Kevin for suggesting a simpler fix!
By the way, I think EHCI is also vulnerable to the same race, it just does not happen in practice. I will look into it.
src/usb-uhci.c | 10 +--------- 1 files changed, 1 insertions(+), 9 deletions(-)
diff --git a/src/usb-uhci.c b/src/usb-uhci.c index 4702695..242f3ba 100644 --- a/src/usb-uhci.c +++ b/src/usb-uhci.c @@ -483,16 +483,8 @@ uhci_send_bulk(struct usb_pipe *p, int dir, void *data, int datasize) data += transfer; datasize -= transfer; } - int i; - for (i=0; i<STACKTDS; i++) { - struct uhci_td *td = &tds[tdpos++ % STACKTDS]; - int ret = wait_td(td); - if (ret) - goto fail; - } - SET_FLATPTR(pipe->toggle, !!toggle); - return 0; + return wait_pipe(pipe, 5000); fail: dprintf(1, "uhci_send_bulk failed\n"); SET_FLATPTR(pipe->qh.element, UHCI_PTR_TERM);
This lets the usb-msc driver send TEST UNIT READY commands.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/usb-msc.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/usb-msc.c b/src/usb-msc.c index 13ef93e..f74b8ec 100644 --- a/src/usb-msc.c +++ b/src/usb-msc.c @@ -79,9 +79,11 @@ usb_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize) goto fail;
// Transfer data from device. - ret = usb_send_bulk(bulkin, USB_DIR_IN, op->buf_fl, bytes); - if (ret) - goto fail; + if (bytes) { + ret = usb_send_bulk(bulkin, USB_DIR_IN, op->buf_fl, bytes); + if (ret) + goto fail; + }
// Transfer csw info. struct csw_s csw;
On Wed, Nov 16, 2011 at 01:02:45PM +0100, Paolo Bonzini wrote:
This lets the usb-msc driver send TEST UNIT READY commands.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com
src/usb-msc.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/usb-msc.c b/src/usb-msc.c index 13ef93e..f74b8ec 100644 --- a/src/usb-msc.c +++ b/src/usb-msc.c @@ -79,9 +79,11 @@ usb_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize) goto fail;
// Transfer data from device.
- ret = usb_send_bulk(bulkin, USB_DIR_IN, op->buf_fl, bytes);
- if (ret)
goto fail;
- if (bytes) {
ret = usb_send_bulk(bulkin, USB_DIR_IN, op->buf_fl, bytes);if (ret)goto fail;- }
Later on in this function there's:
op->count -= csw.dCSWDataResidue / blocksize;
Doesn't it also need to check for blocksize != 0 in order to support TEST UNIT READY?
-Kevin
This lets the usb-msc driver send TEST UNIT READY commands.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- The new hunk was present in the previous submission but was in patch 06/16. It didn't break bisectability, but was not nice.
src/usb-msc.c | 11 +++++++---- 1 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/usb-msc.c b/src/usb-msc.c index 13ef93e..357e638 100644 --- a/src/usb-msc.c +++ b/src/usb-msc.c @@ -79,9 +79,11 @@ usb_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize) goto fail;
// Transfer data from device. - ret = usb_send_bulk(bulkin, USB_DIR_IN, op->buf_fl, bytes); - if (ret) - goto fail; + if (bytes) { + ret = usb_send_bulk(bulkin, USB_DIR_IN, op->buf_fl, bytes); + if (ret) + goto fail; + }
// Transfer csw info. struct csw_s csw; @@ -95,7 +97,8 @@ usb_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize) if (csw.bCSWStatus == 2) goto fail;
- op->count -= csw.dCSWDataResidue / blocksize; + if (csw.dCSWDataResidue) + op->count -= csw.dCSWDataResidue / blocksize; return DISK_RET_EBADTRACK;
fail:
This makes it a bit nicer to later introduce writes.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/usb-msc.c | 19 ++++++++++++++----- 1 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/src/usb-msc.c b/src/usb-msc.c index f74b8ec..5e38b01 100644 --- a/src/usb-msc.c +++ b/src/usb-msc.c @@ -46,6 +46,17 @@ struct csw_s { u8 bCSWStatus; } PACKED;
+static int +usb_msc_send(struct usbdrive_s *udrive_g, int dir, void *buf, u32 bytes) +{ + struct usb_pipe *pipe; + if (dir == USB_DIR_OUT) + pipe = GET_GLOBAL(udrive_g->bulkout); + else + pipe = GET_GLOBAL(udrive_g->bulkin); + return usb_send_bulk(pipe, dir, buf, bytes); +} + // Low-level usb command transmit function. int usb_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize) @@ -57,8 +68,6 @@ usb_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize) , op->drive_g, 0, op->count, blocksize, op->buf_fl); struct usbdrive_s *udrive_g = container_of( op->drive_g, struct usbdrive_s, drive); - struct usb_pipe *bulkin = GET_GLOBAL(udrive_g->bulkin); - struct usb_pipe *bulkout = GET_GLOBAL(udrive_g->bulkout);
// Setup command block wrapper. u32 bytes = blocksize * op->count; @@ -73,21 +82,21 @@ usb_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize) memcpy(cbw.CBWCB, cdbcmd, USB_CDB_SIZE);
// Transfer cbw to device. - int ret = usb_send_bulk(bulkout, USB_DIR_OUT + int ret = usb_msc_send(udrive_g, USB_DIR_OUT , MAKE_FLATPTR(GET_SEG(SS), &cbw), sizeof(cbw)); if (ret) goto fail;
// Transfer data from device. if (bytes) { - ret = usb_send_bulk(bulkin, USB_DIR_IN, op->buf_fl, bytes); + ret = usb_msc_send(udrive_g, USB_DIR_IN, op->buf_fl, bytes); if (ret) goto fail; }
// Transfer csw info. struct csw_s csw; - ret = usb_send_bulk(bulkin, USB_DIR_IN + ret = usb_msc_send(udrive_g, USB_DIR_IN , MAKE_FLATPTR(GET_SEG(SS), &csw), sizeof(csw)); if (ret) goto fail;
Writes only require building the CDB and some care with the direction in the USB packet.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/blockcmd.c | 12 ++++++++++++ src/blockcmd.h | 1 + src/usb-msc.c | 15 ++++++++------- 3 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/src/blockcmd.c b/src/blockcmd.c index c127729..f5e2ce3 100644 --- a/src/blockcmd.c +++ b/src/blockcmd.c @@ -91,3 +91,15 @@ cdb_read(struct disk_op_s *op) cmd.count = htons(op->count); return cdb_cmd_data(op, &cmd, GET_GLOBAL(op->drive_g->blksize)); } + +// Write sectors. +int +cdb_write(struct disk_op_s *op) +{ + struct cdb_rwdata_10 cmd; + memset(&cmd, 0, sizeof(cmd)); + cmd.command = CDB_CMD_WRITE_10; + cmd.lba = htonl(op->lba); + cmd.count = htons(op->count); + return cdb_cmd_data(op, &cmd, GET_GLOBAL(op->drive_g->blksize)); +} diff --git a/src/blockcmd.h b/src/blockcmd.h index 49921b0..84ab659 100644 --- a/src/blockcmd.h +++ b/src/blockcmd.h @@ -75,5 +75,6 @@ int cdb_test_unit_ready(struct disk_op_s *op); int cdb_read_capacity(struct disk_op_s *op, struct cdbres_read_capacity *data); int cdb_inquiry(struct disk_op_s *op, struct cdbres_inquiry *data); int cdb_read(struct disk_op_s *op); +int cdb_write(struct disk_op_s *op);
#endif // blockcmd.h diff --git a/src/usb-msc.c b/src/usb-msc.c index 5e38b01..65b33a2 100644 --- a/src/usb-msc.c +++ b/src/usb-msc.c @@ -73,13 +73,13 @@ usb_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize) u32 bytes = blocksize * op->count; struct cbw_s cbw; memset(&cbw, 0, sizeof(cbw)); + memcpy(cbw.CBWCB, cdbcmd, USB_CDB_SIZE); cbw.dCBWSignature = CBW_SIGNATURE; cbw.dCBWTag = 999; // XXX cbw.dCBWDataTransferLength = bytes; - cbw.bmCBWFlags = USB_DIR_IN; // XXX + cbw.bmCBWFlags = (cbw.CBWCB[0] == CDB_CMD_WRITE_10) ? USB_DIR_OUT : USB_DIR_IN; cbw.bCBWLUN = 0; // XXX cbw.bCBWCBLength = USB_CDB_SIZE; - memcpy(cbw.CBWCB, cdbcmd, USB_CDB_SIZE);
// Transfer cbw to device. int ret = usb_msc_send(udrive_g, USB_DIR_OUT @@ -87,9 +87,9 @@ usb_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize) if (ret) goto fail;
- // Transfer data from device. + // Transfer data to/from device. if (bytes) { - ret = usb_msc_send(udrive_g, USB_DIR_IN, op->buf_fl, bytes); + ret = usb_msc_send(udrive_g, cbw.bmCBWFlags, op->buf_fl, bytes); if (ret) goto fail; } @@ -106,7 +106,8 @@ usb_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize) if (csw.bCSWStatus == 2) goto fail;
- op->count -= csw.dCSWDataResidue / blocksize; + if (csw.dCSWDataResidue) + op->count -= csw.dCSWDataResidue / blocksize; return DISK_RET_EBADTRACK;
fail: @@ -130,9 +131,9 @@ process_usb_op(struct disk_op_s *op) switch (op->command) { case CMD_READ: return cdb_read(op); - case CMD_FORMAT: case CMD_WRITE: - return DISK_RET_EWRITEPROTECT; + return cdb_write(op); + case CMD_FORMAT: case CMD_RESET: case CMD_ISREADY: case CMD_VERIFY:
Only leave the bootprio code in setup_drive_hd, like in setup_drive_cdrom. This is a preparatory step; later, the SCSI code in usb_msc_init will become entirely generic.
Also, the returned number of sectors is off by one. This will become more important when CHS translation is added later.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/usb-msc.c | 30 ++++++++++++++---------------- 1 files changed, 14 insertions(+), 16 deletions(-)
diff --git a/src/usb-msc.c b/src/usb-msc.c index 65b33a2..818710c 100644 --- a/src/usb-msc.c +++ b/src/usb-msc.c @@ -153,7 +153,6 @@ process_usb_op(struct disk_op_s *op) static int setup_drive_cdrom(struct disk_op_s *op, char *desc) { - op->drive_g->blksize = CDROM_SECTOR_SIZE; op->drive_g->sectors = (u64)-1; struct usb_pipe *pipe = container_of( op->drive_g, struct usbdrive_s, drive)->bulkout; @@ -165,29 +164,16 @@ setup_drive_cdrom(struct disk_op_s *op, char *desc) static int setup_drive_hd(struct disk_op_s *op, char *desc) { - struct cdbres_read_capacity info; - int ret = cdb_read_capacity(op, &info); - if (ret) - return ret; - // XXX - retry for some timeout? - - u32 blksize = ntohl(info.blksize), sectors = ntohl(info.sectors); - if (blksize != DISK_SECTOR_SIZE) { - if (blksize == CDROM_SECTOR_SIZE) - return setup_drive_cdrom(op, desc); - dprintf(1, "Unsupported USB MSC block size %d\n", blksize); + if (op->drive_g->blksize != DISK_SECTOR_SIZE) { + dprintf(1, "Unsupported USB MSC block size %d\n", op->drive_g->blksize); return -1; } - op->drive_g->blksize = blksize; - op->drive_g->sectors = sectors; - dprintf(1, "USB MSC blksize=%d sectors=%d\n", blksize, sectors);
// Register with bcv system. struct usb_pipe *pipe = container_of( op->drive_g, struct usbdrive_s, drive)->bulkout; int prio = bootprio_find_usb(pipe->cntl->pci, pipe->path); boot_add_hd(op->drive_g, desc, prio); - return 0; }
@@ -257,6 +243,18 @@ usb_msc_init(struct usb_pipe *pipe , vendor, product, rev); ret = setup_drive_cdrom(&dop, desc); } else { + struct cdbres_read_capacity capdata; + ret = cdb_read_capacity(&dop, &capdata); + if (ret) + return ret; + // XXX - retry for some timeout? + + // READ CAPACITY returns the address of the last block + udrive_g->drive.blksize = ntohl(capdata.blksize); + udrive_g->drive.sectors = ntohl(capdata.sectors) + 1; + dprintf(1, "USB MSC blksize=%d sectors=%d\n", + udrive_g->drive.blksize, (int)udrive_g->drive.sectors); + char *desc = znprintf(MAXDESCSIZE, "USB Drive %s %s %s" , vendor, product, rev); ret = setup_drive_hd(&dop, desc);
The two functions do not need anymore a disk_op_s.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/usb-msc.c | 22 +++++++++++----------- 1 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/usb-msc.c b/src/usb-msc.c index 818710c..5539f6a 100644 --- a/src/usb-msc.c +++ b/src/usb-msc.c @@ -151,29 +151,29 @@ process_usb_op(struct disk_op_s *op) ****************************************************************/
static int -setup_drive_cdrom(struct disk_op_s *op, char *desc) +setup_drive_cdrom(struct drive_s *drive, char *desc) { - op->drive_g->sectors = (u64)-1; + drive->sectors = (u64)-1; struct usb_pipe *pipe = container_of( - op->drive_g, struct usbdrive_s, drive)->bulkout; + drive, struct usbdrive_s, drive)->bulkout; int prio = bootprio_find_usb(pipe->cntl->pci, pipe->path); - boot_add_cd(op->drive_g, desc, prio); + boot_add_cd(drive, desc, prio); return 0; }
static int -setup_drive_hd(struct disk_op_s *op, char *desc) +setup_drive_hd(struct drive_s *drive, char *desc) { - if (op->drive_g->blksize != DISK_SECTOR_SIZE) { - dprintf(1, "Unsupported USB MSC block size %d\n", op->drive_g->blksize); + if (drive->blksize != DISK_SECTOR_SIZE) { + dprintf(1, "Unsupported USB MSC block size %d\n", drive->blksize); return -1; }
// Register with bcv system. struct usb_pipe *pipe = container_of( - op->drive_g, struct usbdrive_s, drive)->bulkout; + drive, struct usbdrive_s, drive)->bulkout; int prio = bootprio_find_usb(pipe->cntl->pci, pipe->path); - boot_add_hd(op->drive_g, desc, prio); + boot_add_hd(drive, desc, prio); return 0; }
@@ -241,7 +241,7 @@ usb_msc_init(struct usb_pipe *pipe if (pdt == USB_MSC_TYPE_CDROM) { char *desc = znprintf(MAXDESCSIZE, "DVD/CD [USB Drive %s %s %s]" , vendor, product, rev); - ret = setup_drive_cdrom(&dop, desc); + ret = setup_drive_cdrom(&udrive_g->drive, desc); } else { struct cdbres_read_capacity capdata; ret = cdb_read_capacity(&dop, &capdata); @@ -257,7 +257,7 @@ usb_msc_init(struct usb_pipe *pipe
char *desc = znprintf(MAXDESCSIZE, "USB Drive %s %s %s" , vendor, product, rev); - ret = setup_drive_hd(&dop, desc); + ret = setup_drive_hd(&udrive_g->drive, desc); } if (ret) goto fail;
They are generic SCSI constants, rename them as such.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/blockcmd.h | 3 +++ src/usb-msc.c | 2 +- src/usb-msc.h | 3 --- 3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/blockcmd.h b/src/blockcmd.h index 84ab659..6923e4c 100644 --- a/src/blockcmd.h +++ b/src/blockcmd.h @@ -57,6 +57,9 @@ struct cdbres_request_sense { u32 reserved_0e; } PACKED;
+#define SCSI_TYPE_DISK 0x00 +#define SCSI_TYPE_CDROM 0x05 + struct cdbres_inquiry { u8 pdt; u8 removable; diff --git a/src/usb-msc.c b/src/usb-msc.c index 5539f6a..115aaef 100644 --- a/src/usb-msc.c +++ b/src/usb-msc.c @@ -238,7 +238,7 @@ usb_msc_init(struct usb_pipe *pipe , vendor, product, rev, pdt, removable); udrive_g->drive.removable = removable;
- if (pdt == USB_MSC_TYPE_CDROM) { + if (pdt == SCSI_TYPE_CDROM) { char *desc = znprintf(MAXDESCSIZE, "DVD/CD [USB Drive %s %s %s]" , vendor, product, rev); ret = setup_drive_cdrom(&udrive_g->drive, desc); diff --git a/src/usb-msc.h b/src/usb-msc.h index 71adb20..a8686a3 100644 --- a/src/usb-msc.h +++ b/src/usb-msc.h @@ -21,7 +21,4 @@ int process_usb_op(struct disk_op_s *op);
#define US_PR_BULK 0x50
-#define USB_MSC_TYPE_DISK 0x00 -#define USB_MSC_TYPE_CDROM 0x05 - #endif // ush-msc.h
Add the wait loop that CDs are already using to usb-msc in the HD case, to cope with a NOT READY or UNIT ATTENTION condition.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/blockcmd.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ src/blockcmd.h | 2 ++ src/cdrom.c | 48 ++---------------------------------------------- src/usb-msc.c | 7 ++++++- 4 files changed, 54 insertions(+), 47 deletions(-)
diff --git a/src/blockcmd.c b/src/blockcmd.c index f5e2ce3..e86fe29 100644 --- a/src/blockcmd.c +++ b/src/blockcmd.c @@ -32,6 +32,50 @@ cdb_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize) }
int +scsi_is_ready(struct disk_op_s *op) +{ + dprintf(6, "scsi_is_ready (drive=%p)\n", op->drive_g); + + /* Retry TEST UNIT READY for 5 seconds unless MEDIUM NOT PRESENT is + * reported by the device. If the device reports "IN PROGRESS", + * 30 seconds is added. */ + int in_progress = 0; + u64 end = calc_future_tsc(5000); + for (;;) { + if (check_tsc(end)) { + dprintf(1, "test unit ready failed\n"); + return -1; + } + + int ret = cdb_test_unit_ready(op); + if (!ret) + // Success + break; + + struct cdbres_request_sense sense; + ret = cdb_get_sense(op, &sense); + if (ret) + // Error - retry. + continue; + + // Sense succeeded. + if (sense.asc == 0x3a) { /* MEDIUM NOT PRESENT */ + dprintf(1, "Device reports MEDIUM NOT PRESENT\n"); + return -1; + } + + if (sense.asc == 0x04 && sense.ascq == 0x01 && !in_progress) { + /* IN PROGRESS OF BECOMING READY */ + printf("Waiting for device to detect medium... "); + /* Allow 30 seconds more */ + end = calc_future_tsc(30000); + in_progress = 1; + } + } + return 0; +} + +int cdb_get_inquiry(struct disk_op_s *op, struct cdbres_inquiry *data) { struct cdb_request_sense cmd; diff --git a/src/blockcmd.h b/src/blockcmd.h index 6923e4c..106a529 100644 --- a/src/blockcmd.h +++ b/src/blockcmd.h @@ -80,4 +80,6 @@ int cdb_inquiry(struct disk_op_s *op, struct cdbres_inquiry *data); int cdb_read(struct disk_op_s *op); int cdb_write(struct disk_op_s *op);
+int scsi_is_ready(struct disk_op_s *op); + #endif // blockcmd.h diff --git a/src/cdrom.c b/src/cdrom.c index b817999..170ffc4 100644 --- a/src/cdrom.c +++ b/src/cdrom.c @@ -184,50 +184,6 @@ cdemu_134b(struct bregs *regs) * CD booting ****************************************************************/
-static int -atapi_is_ready(struct disk_op_s *op) -{ - dprintf(6, "atapi_is_ready (drive=%p)\n", op->drive_g); - - /* Retry TEST UNIT READY for 5 seconds unless MEDIUM NOT PRESENT is - * reported by the device. If the device reports "IN PROGRESS", - * 30 seconds is added. */ - int in_progress = 0; - u64 end = calc_future_tsc(5000); - for (;;) { - if (check_tsc(end)) { - dprintf(1, "test unit ready failed\n"); - return -1; - } - - int ret = cdb_test_unit_ready(op); - if (!ret) - // Success - break; - - struct cdbres_request_sense sense; - ret = cdb_get_sense(op, &sense); - if (ret) - // Error - retry. - continue; - - // Sense succeeded. - if (sense.asc == 0x3a) { /* MEDIUM NOT PRESENT */ - dprintf(1, "Device reports MEDIUM NOT PRESENT\n"); - return -1; - } - - if (sense.asc == 0x04 && sense.ascq == 0x01 && !in_progress) { - /* IN PROGRESS OF BECOMING READY */ - printf("Waiting for device to detect medium... "); - /* Allow 30 seconds more */ - end = calc_future_tsc(30000); - in_progress = 1; - } - } - return 0; -} - int cdrom_boot(struct drive_s *drive_g) { @@ -238,9 +194,9 @@ cdrom_boot(struct drive_s *drive_g) if (!dop.drive_g || cdid < 0) return 1;
- int ret = atapi_is_ready(&dop); + int ret = scsi_is_ready(&dop); if (ret) - dprintf(1, "atapi_is_ready returned %d\n", ret); + dprintf(1, "scsi_is_ready returned %d\n", ret);
// Read the Boot Record Volume Descriptor u8 buffer[2048]; diff --git a/src/usb-msc.c b/src/usb-msc.c index 115aaef..6617a9f 100644 --- a/src/usb-msc.c +++ b/src/usb-msc.c @@ -243,11 +243,16 @@ usb_msc_init(struct usb_pipe *pipe , vendor, product, rev); ret = setup_drive_cdrom(&udrive_g->drive, desc); } else { + ret = scsi_is_ready(&dop); + if (ret) { + dprintf(1, "scsi_is_ready returned %d\n", ret); + return ret; + } + struct cdbres_read_capacity capdata; ret = cdb_read_capacity(&dop, &capdata); if (ret) return ret; - // XXX - retry for some timeout?
// READ CAPACITY returns the address of the last block udrive_g->drive.blksize = ntohl(capdata.blksize);
Finally move the INQUIRY/TEST UNIT READY/READ CAPACITY sequence to generic code, so that virtio-scsi will be able to use it.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/blockcmd.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/blockcmd.h | 1 + src/usb-msc.c | 51 ++++++------------------------------------------- 3 files changed, 65 insertions(+), 44 deletions(-)
diff --git a/src/blockcmd.c b/src/blockcmd.c index e86fe29..cbc31b2 100644 --- a/src/blockcmd.c +++ b/src/blockcmd.c @@ -75,6 +75,63 @@ scsi_is_ready(struct disk_op_s *op) return 0; }
+// Validate drive and find block size and sector count. +int +scsi_init_drive(struct drive_s *drive, const char *s, int *pdt, char **desc) +{ + if (!CONFIG_USB_MSC) + return 0; + + struct disk_op_s dop; + memset(&dop, 0, sizeof(dop)); + dop.drive_g = drive; + struct cdbres_inquiry data; + int ret = cdb_get_inquiry(&dop, &data); + if (ret) + return ret; + char vendor[sizeof(data.vendor)+1], product[sizeof(data.product)+1]; + char rev[sizeof(data.rev)+1]; + strtcpy(vendor, data.vendor, sizeof(vendor)); + nullTrailingSpace(vendor); + strtcpy(product, data.product, sizeof(product)); + nullTrailingSpace(product); + strtcpy(rev, data.rev, sizeof(rev)); + nullTrailingSpace(rev); + *pdt = data.pdt & 0x1f; + int removable = !!(data.removable & 0x80); + dprintf(1, "%s vendor='%s' product='%s' rev='%s' type=%d removable=%d\n" + , s, vendor, product, rev, *pdt, removable); + drive->removable = removable; + + if (*pdt == SCSI_TYPE_CDROM) { + drive->blksize = CDROM_SECTOR_SIZE; + drive->sectors = (u64)-1; + + *desc = znprintf(MAXDESCSIZE, "DVD/CD [%s Drive %s %s %s]" + , s, vendor, product, rev); + return 0; + } + + if (scsi_is_ready(&dop)) + return ret; + + struct cdbres_read_capacity capdata; + ret = cdb_read_capacity(&dop, &capdata); + if (ret) + return ret; + + // READ CAPACITY returns the address of the last block. + // TODO: send READ CAPACITY(16) if capdata.sectors is 0xFFFFFFFF + drive->blksize = ntohl(capdata.blksize); + drive->sectors = (u64)ntohl(capdata.sectors) + 1; + dprintf(1, "%s blksize=%d sectors=%d\n" + , s, drive->blksize, (unsigned)drive->sectors); + + *desc = znprintf(MAXDESCSIZE, "%s Drive %s %s %s" + , s, vendor, product, rev); + return 0; +} + int cdb_get_inquiry(struct disk_op_s *op, struct cdbres_inquiry *data) { diff --git a/src/blockcmd.h b/src/blockcmd.h index 106a529..13ae991 100644 --- a/src/blockcmd.h +++ b/src/blockcmd.h @@ -81,5 +81,6 @@ int cdb_read(struct disk_op_s *op); int cdb_write(struct disk_op_s *op);
int scsi_is_ready(struct disk_op_s *op); +int scsi_init_drive(struct drive_s *drive, const char *s, int *pdt, char **desc);
#endif // blockcmd.h diff --git a/src/usb-msc.c b/src/usb-msc.c index 6617a9f..2ec5356 100644 --- a/src/usb-msc.c +++ b/src/usb-msc.c @@ -216,54 +216,17 @@ usb_msc_init(struct usb_pipe *pipe if (!udrive_g->bulkin || !udrive_g->bulkout) goto fail;
- // Validate drive and find block size and sector count. - struct disk_op_s dop; - memset(&dop, 0, sizeof(dop)); - dop.drive_g = &udrive_g->drive; - struct cdbres_inquiry data; - int ret = cdb_get_inquiry(&dop, &data); + int ret, pdt; + char *desc = NULL; + ret = scsi_init_drive(&udrive_g->drive, "USB MSC", &pdt, &desc); if (ret) goto fail; - char vendor[sizeof(data.vendor)+1], product[sizeof(data.product)+1]; - char rev[sizeof(data.rev)+1]; - strtcpy(vendor, data.vendor, sizeof(vendor)); - nullTrailingSpace(vendor); - strtcpy(product, data.product, sizeof(product)); - nullTrailingSpace(product); - strtcpy(rev, data.rev, sizeof(rev)); - nullTrailingSpace(rev); - int pdt = data.pdt & 0x1f; - int removable = !!(data.removable & 0x80); - dprintf(1, "USB MSC vendor='%s' product='%s' rev='%s' type=%d removable=%d\n" - , vendor, product, rev, pdt, removable); - udrive_g->drive.removable = removable; - - if (pdt == SCSI_TYPE_CDROM) { - char *desc = znprintf(MAXDESCSIZE, "DVD/CD [USB Drive %s %s %s]" - , vendor, product, rev); - ret = setup_drive_cdrom(&udrive_g->drive, desc); - } else { - ret = scsi_is_ready(&dop); - if (ret) { - dprintf(1, "scsi_is_ready returned %d\n", ret); - return ret; - } - - struct cdbres_read_capacity capdata; - ret = cdb_read_capacity(&dop, &capdata); - if (ret) - return ret;
- // READ CAPACITY returns the address of the last block - udrive_g->drive.blksize = ntohl(capdata.blksize); - udrive_g->drive.sectors = ntohl(capdata.sectors) + 1; - dprintf(1, "USB MSC blksize=%d sectors=%d\n", - udrive_g->drive.blksize, (int)udrive_g->drive.sectors); - - char *desc = znprintf(MAXDESCSIZE, "USB Drive %s %s %s" - , vendor, product, rev); + if (pdt == SCSI_TYPE_CDROM) + ret = setup_drive_cdrom(&udrive_g->drive, desc); + else ret = setup_drive_hd(&udrive_g->drive, desc); - } + if (ret) goto fail;
On Wed, Nov 16, 2011 at 01:02:52PM +0100, Paolo Bonzini wrote: [...]
- if (scsi_is_ready(&dop))
return ret;
Typo? (Should be return -1?) -Kevin
Finally move the INQUIRY/TEST UNIT READY/READ CAPACITY sequence to generic code, so that virtio-scsi will be able to use it.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- v1->v2: move scsi_is_ready call correctly from usb-msc to blockcmd
src/blockcmd.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/blockcmd.h | 1 + src/usb-msc.c | 51 ++++++---------------------------------------- 3 files changed, 69 insertions(+), 44 deletions(-)
diff --git a/src/blockcmd.c b/src/blockcmd.c index e86fe29..312dfb6 100644 --- a/src/blockcmd.c +++ b/src/blockcmd.c @@ -75,6 +75,67 @@ scsi_is_ready(struct disk_op_s *op) return 0; }
+// Validate drive and find block size and sector count. +int +scsi_init_drive(struct drive_s *drive, const char *s, int *pdt, char **desc) +{ + if (!CONFIG_USB_MSC) + return 0; + + struct disk_op_s dop; + memset(&dop, 0, sizeof(dop)); + dop.drive_g = drive; + struct cdbres_inquiry data; + int ret = cdb_get_inquiry(&dop, &data); + if (ret) + return ret; + char vendor[sizeof(data.vendor)+1], product[sizeof(data.product)+1]; + char rev[sizeof(data.rev)+1]; + strtcpy(vendor, data.vendor, sizeof(vendor)); + nullTrailingSpace(vendor); + strtcpy(product, data.product, sizeof(product)); + nullTrailingSpace(product); + strtcpy(rev, data.rev, sizeof(rev)); + nullTrailingSpace(rev); + *pdt = data.pdt & 0x1f; + int removable = !!(data.removable & 0x80); + dprintf(1, "%s vendor='%s' product='%s' rev='%s' type=%d removable=%d\n" + , s, vendor, product, rev, *pdt, removable); + drive->removable = removable; + + if (*pdt == SCSI_TYPE_CDROM) { + drive->blksize = CDROM_SECTOR_SIZE; + drive->sectors = (u64)-1; + + *desc = znprintf(MAXDESCSIZE, "DVD/CD [%s Drive %s %s %s]" + , s, vendor, product, rev); + return 0; + } + + ret = scsi_is_ready(&dop); + if (ret) { + dprintf(1, "scsi_is_ready returned %d\n", ret); + return ret; + } + + struct cdbres_read_capacity capdata; + ret = cdb_read_capacity(&dop, &capdata); + if (ret) + return ret; + + // READ CAPACITY returns the address of the last block. + // We do not bother with READ CAPACITY(16) because BIOS does not support + // 64-bit LBA anyway. + drive->blksize = ntohl(capdata.blksize); + drive->sectors = (u64)ntohl(capdata.sectors) + 1; + dprintf(1, "%s blksize=%d sectors=%d\n" + , s, drive->blksize, (unsigned)drive->sectors); + + *desc = znprintf(MAXDESCSIZE, "%s Drive %s %s %s" + , s, vendor, product, rev); + return 0; +} + int cdb_get_inquiry(struct disk_op_s *op, struct cdbres_inquiry *data) { diff --git a/src/blockcmd.h b/src/blockcmd.h index 106a529..13ae991 100644 --- a/src/blockcmd.h +++ b/src/blockcmd.h @@ -81,5 +81,6 @@ int cdb_read(struct disk_op_s *op); int cdb_write(struct disk_op_s *op);
int scsi_is_ready(struct disk_op_s *op); +int scsi_init_drive(struct drive_s *drive, const char *s, int *pdt, char **desc);
#endif // blockcmd.h diff --git a/src/usb-msc.c b/src/usb-msc.c index 6617a9f..2ec5356 100644 --- a/src/usb-msc.c +++ b/src/usb-msc.c @@ -216,54 +216,17 @@ usb_msc_init(struct usb_pipe *pipe if (!udrive_g->bulkin || !udrive_g->bulkout) goto fail;
- // Validate drive and find block size and sector count. - struct disk_op_s dop; - memset(&dop, 0, sizeof(dop)); - dop.drive_g = &udrive_g->drive; - struct cdbres_inquiry data; - int ret = cdb_get_inquiry(&dop, &data); + int ret, pdt; + char *desc = NULL; + ret = scsi_init_drive(&udrive_g->drive, "USB MSC", &pdt, &desc); if (ret) goto fail; - char vendor[sizeof(data.vendor)+1], product[sizeof(data.product)+1]; - char rev[sizeof(data.rev)+1]; - strtcpy(vendor, data.vendor, sizeof(vendor)); - nullTrailingSpace(vendor); - strtcpy(product, data.product, sizeof(product)); - nullTrailingSpace(product); - strtcpy(rev, data.rev, sizeof(rev)); - nullTrailingSpace(rev); - int pdt = data.pdt & 0x1f; - int removable = !!(data.removable & 0x80); - dprintf(1, "USB MSC vendor='%s' product='%s' rev='%s' type=%d removable=%d\n" - , vendor, product, rev, pdt, removable); - udrive_g->drive.removable = removable; - - if (pdt == SCSI_TYPE_CDROM) { - char *desc = znprintf(MAXDESCSIZE, "DVD/CD [USB Drive %s %s %s]" - , vendor, product, rev); - ret = setup_drive_cdrom(&udrive_g->drive, desc); - } else { - ret = scsi_is_ready(&dop); - if (ret) { - dprintf(1, "scsi_is_ready returned %d\n", ret); - return ret; - } - - struct cdbres_read_capacity capdata; - ret = cdb_read_capacity(&dop, &capdata); - if (ret) - return ret;
- // READ CAPACITY returns the address of the last block - udrive_g->drive.blksize = ntohl(capdata.blksize); - udrive_g->drive.sectors = ntohl(capdata.sectors) + 1; - dprintf(1, "USB MSC blksize=%d sectors=%d\n", - udrive_g->drive.blksize, (int)udrive_g->drive.sectors); - - char *desc = znprintf(MAXDESCSIZE, "USB Drive %s %s %s" - , vendor, product, rev); + if (pdt == SCSI_TYPE_CDROM) + ret = setup_drive_cdrom(&udrive_g->drive, desc); + else ret = setup_drive_hd(&udrive_g->drive, desc); - } + if (ret) goto fail;
virtio-scsi's low-level dispatch code is exactly the same as USB's, since in the end both are actually SCSI HBAs. Move it to common code.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/block.c | 28 +++++++++++++++++++++++++--- src/usb-msc.c | 28 ---------------------------- 2 files changed, 25 insertions(+), 31 deletions(-)
diff --git a/src/block.c b/src/block.c index f7e7851..ac9eb26 100644 --- a/src/block.c +++ b/src/block.c @@ -11,8 +11,8 @@ #include "util.h" // dprintf #include "ata.h" // process_ata_op #include "ahci.h" // process_ahci_op -#include "usb-msc.h" // process_usb_op #include "virtio-blk.h" // process_virtio_op +#include "blockcmd.h" // cdb_*
u8 FloppyCount VAR16VISIBLE; u8 CDCount; @@ -276,6 +276,28 @@ map_floppy_drive(struct drive_s *drive_g) * 16bit calling interface ****************************************************************/
+int +process_scsi_op(struct disk_op_s *op) +{ + if (!CONFIG_USB_MSC) + return 0; + switch (op->command) { + case CMD_READ: + return cdb_read(op); + case CMD_WRITE: + return cdb_write(op); + case CMD_FORMAT: + case CMD_RESET: + case CMD_ISREADY: + case CMD_VERIFY: + case CMD_SEEK: + return DISK_RET_SUCCESS; + default: + op->count = 0; + return DISK_RET_EPARAM; + } +} + // Execute a disk_op request. int process_op(struct disk_op_s *op) @@ -293,12 +315,12 @@ process_op(struct disk_op_s *op) return process_ramdisk_op(op); case DTYPE_CDEMU: return process_cdemu_op(op); - case DTYPE_USB: - return process_usb_op(op); case DTYPE_VIRTIO: return process_virtio_op(op); case DTYPE_AHCI: return process_ahci_op(op); + case DTYPE_USB: + return process_scsi_op(op); default: op->count = 0; return DISK_RET_EPARAM; diff --git a/src/usb-msc.c b/src/usb-msc.c index 2ec5356..c350100 100644 --- a/src/usb-msc.c +++ b/src/usb-msc.c @@ -119,34 +119,6 @@ fail:
/**************************************************************** - * Drive ops - ****************************************************************/ - -// 16bit command demuxer for ATAPI cdroms. -int -process_usb_op(struct disk_op_s *op) -{ - if (!CONFIG_USB_MSC) - return 0; - switch (op->command) { - case CMD_READ: - return cdb_read(op); - case CMD_WRITE: - return cdb_write(op); - case CMD_FORMAT: - case CMD_RESET: - case CMD_ISREADY: - case CMD_VERIFY: - case CMD_SEEK: - return DISK_RET_SUCCESS; - default: - op->count = 0; - return DISK_RET_EPARAM; - } -} - - -/**************************************************************** * Setup ****************************************************************/
The mode page is marked as obsolete, but QEMU can provide the information.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/blockcmd.c | 32 ++++++++++++++++++++++++++++++++ src/blockcmd.h | 29 +++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 0 deletions(-)
diff --git a/src/blockcmd.c b/src/blockcmd.c index cbc31b2..085f960 100644 --- a/src/blockcmd.c +++ b/src/blockcmd.c @@ -127,6 +127,23 @@ scsi_init_drive(struct drive_s *drive, const char *s, int *pdt, char **desc) dprintf(1, "%s blksize=%d sectors=%d\n" , s, drive->blksize, (unsigned)drive->sectors);
+ struct cdbres_mode_sense_geom geomdata; + ret = cdb_mode_sense_geom(&dop, &geomdata); + if (ret == 0) { + u32 cylinders; + cylinders = geomdata.cyl[0] << 16; + cylinders |= geomdata.cyl[1] << 8; + cylinders |= geomdata.cyl[2]; + if (cylinders && geomdata.heads && + drive->sectors <= 0xFFFFFFFFULL && + ((u32)drive->sectors % (geomdata.heads * cylinders) == 0)) { + drive->pchs.cylinders = cylinders; + drive->pchs.heads = geomdata.heads; + drive->pchs.spt = (u32)drive->sectors + / (geomdata.heads * cylinders); + } + } + *desc = znprintf(MAXDESCSIZE, "%s Drive %s %s %s" , s, vendor, product, rev); return 0; @@ -181,6 +198,21 @@ cdb_read_capacity(struct disk_op_s *op, struct cdbres_read_capacity *data) return cdb_cmd_data(op, &cmd, sizeof(*data)); }
+// Mode sense, geometry page. +int +cdb_mode_sense_geom(struct disk_op_s *op, struct cdbres_mode_sense_geom *data) +{ + struct cdb_mode_sense cmd; + memset(&cmd, 0, sizeof(cmd)); + cmd.command = CDB_CMD_MODE_SENSE; + cmd.flags = 8; /* DBD */ + cmd.page = MODE_PAGE_HD_GEOMETRY; + cmd.count = htons(sizeof(*data)); + op->count = 1; + op->buf_fl = data; + return cdb_cmd_data(op, &cmd, sizeof(*data)); +} + // Read sectors. int cdb_read(struct disk_op_s *op) diff --git a/src/blockcmd.h b/src/blockcmd.h index 13ae991..bace649 100644 --- a/src/blockcmd.h +++ b/src/blockcmd.h @@ -71,11 +71,40 @@ struct cdbres_inquiry { char rev[4]; } PACKED;
+#define CDB_CMD_MODE_SENSE 0x5A +#define MODE_PAGE_HD_GEOMETRY 0x04 + +struct cdb_mode_sense { + u8 command; + u8 flags; + u8 page; + u32 reserved_03; + u16 count; + u8 reserved_09; + u8 pad[6]; +} PACKED; + +struct cdbres_mode_sense_geom { + u8 unused_00[3]; + u8 read_only; + u32 unused_04; + u8 page; + u8 length; + u8 cyl[3]; + u8 heads; + u8 precomp[3]; + u8 reduced[3]; + u16 step_rate; + u8 landing[3]; + u16 rpm; +} PACKED; + // blockcmd.c int cdb_get_inquiry(struct disk_op_s *op, struct cdbres_inquiry *data); int cdb_get_sense(struct disk_op_s *op, struct cdbres_request_sense *data); int cdb_test_unit_ready(struct disk_op_s *op); int cdb_read_capacity(struct disk_op_s *op, struct cdbres_read_capacity *data); +int cdb_mode_sense_geom(struct disk_op_s *op, struct cdbres_mode_sense_geom *data); int cdb_inquiry(struct disk_op_s *op, struct cdbres_inquiry *data); int cdb_read(struct disk_op_s *op); int cdb_write(struct disk_op_s *op);
Avoid ambiguity when virtio-scsi will be introduced.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/Kconfig | 4 ++-- src/block.c | 6 +++--- src/disk.c | 4 ++-- src/disk.h | 18 +++++++++--------- src/virtio-blk.c | 4 ++-- src/virtio-blk.h | 2 +- 6 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/src/Kconfig b/src/Kconfig index 338f51a..f8d245a 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -109,10 +109,10 @@ menu "Hardware support" Support for AHCI disk code. config VIRTIO_BLK depends on DRIVES && !COREBOOT - bool "VirtIO controllers" + bool "virtio-blk controllers" default y help - Support boot from virtio storage. + Support boot from virtio-blk storage. config FLOPPY depends on DRIVES bool "Floppy controller" diff --git a/src/block.c b/src/block.c index ac9eb26..eeebd83 100644 --- a/src/block.c +++ b/src/block.c @@ -11,7 +11,7 @@ #include "util.h" // dprintf #include "ata.h" // process_ata_op #include "ahci.h" // process_ahci_op -#include "virtio-blk.h" // process_virtio_op +#include "virtio-blk.h" // process_virtio_blk_op #include "blockcmd.h" // cdb_*
u8 FloppyCount VAR16VISIBLE; @@ -315,8 +315,8 @@ process_op(struct disk_op_s *op) return process_ramdisk_op(op); case DTYPE_CDEMU: return process_cdemu_op(op); - case DTYPE_VIRTIO: - return process_virtio_op(op); + case DTYPE_VIRTIO_BLK: + return process_virtio_blk_op(op); case DTYPE_AHCI: return process_ahci_op(op); case DTYPE_USB: diff --git a/src/disk.c b/src/disk.c index 8f7c61f..f2c6621 100644 --- a/src/disk.c +++ b/src/disk.c @@ -546,7 +546,7 @@ disk_1348(struct bregs *regs, struct drive_s *drive_g) SET_INT13DPT(regs, blksize, blksize);
if (size < 30 || - (type != DTYPE_ATA && type != DTYPE_ATAPI && type != DTYPE_VIRTIO)) { + (type != DTYPE_ATA && type != DTYPE_ATAPI && type != DTYPE_VIRTIO_BLK)) { disk_ret(regs, DISK_RET_SUCCESS); return; } @@ -651,7 +651,7 @@ disk_1348(struct bregs *regs, struct drive_s *drive_g) SET_INT13DPT(regs, iface_path, iobase1); }
- if (type != DTYPE_VIRTIO) { + if (type != DTYPE_VIRTIO_BLK) { SET_INT13DPT(regs, iface_type[0], 'A'); SET_INT13DPT(regs, iface_type[1], 'T'); SET_INT13DPT(regs, iface_type[2], 'A'); diff --git a/src/disk.h b/src/disk.h index ac33518..dd7c46a 100644 --- a/src/disk.h +++ b/src/disk.h @@ -198,15 +198,15 @@ struct drive_s { #define DISK_SECTOR_SIZE 512 #define CDROM_SECTOR_SIZE 2048
-#define DTYPE_NONE 0x00 -#define DTYPE_FLOPPY 0x01 -#define DTYPE_ATA 0x02 -#define DTYPE_ATAPI 0x03 -#define DTYPE_RAMDISK 0x04 -#define DTYPE_CDEMU 0x05 -#define DTYPE_USB 0x06 -#define DTYPE_VIRTIO 0x07 -#define DTYPE_AHCI 0x08 +#define DTYPE_NONE 0x00 +#define DTYPE_FLOPPY 0x01 +#define DTYPE_ATA 0x02 +#define DTYPE_ATAPI 0x03 +#define DTYPE_RAMDISK 0x04 +#define DTYPE_CDEMU 0x05 +#define DTYPE_USB 0x06 +#define DTYPE_VIRTIO_BLK 0x07 +#define DTYPE_AHCI 0x08
#define MAXDESCSIZE 80
diff --git a/src/virtio-blk.c b/src/virtio-blk.c index b1274fc..a81a873 100644 --- a/src/virtio-blk.c +++ b/src/virtio-blk.c @@ -75,7 +75,7 @@ virtio_blk_op(struct disk_op_s *op, int write) }
int -process_virtio_op(struct disk_op_s *op) +process_virtio_blk_op(struct disk_op_s *op) { if (! CONFIG_VIRTIO_BLK || CONFIG_COREBOOT) return 0; @@ -110,7 +110,7 @@ init_virtio_blk(struct pci_device *pci) } memset(vdrive_g, 0, sizeof(*vdrive_g)); memset(vq, 0, sizeof(*vq)); - vdrive_g->drive.type = DTYPE_VIRTIO; + vdrive_g->drive.type = DTYPE_VIRTIO_BLK; vdrive_g->drive.cntl_id = bdf; vdrive_g->vq = vq;
diff --git a/src/virtio-blk.h b/src/virtio-blk.h index 7243704..0825f09 100644 --- a/src/virtio-blk.h +++ b/src/virtio-blk.h @@ -37,7 +37,7 @@ struct virtio_blk_outhdr { #define VIRTIO_BLK_S_UNSUPP 2
struct disk_op_s; -int process_virtio_op(struct disk_op_s *op); +int process_virtio_blk_op(struct disk_op_s *op); void virtio_blk_setup(void);
#endif /* _VIRTIO_BLK_H */
Put together the common parts of all virtio device initialization.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/virtio-blk.c | 9 +-------- src/virtio-pci.c | 11 +++++++++++ src/virtio-pci.h | 1 + 3 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/src/virtio-blk.c b/src/virtio-blk.c index a81a873..9ed82e0 100644 --- a/src/virtio-blk.c +++ b/src/virtio-blk.c @@ -114,15 +114,8 @@ init_virtio_blk(struct pci_device *pci) vdrive_g->drive.cntl_id = bdf; vdrive_g->vq = vq;
- u16 ioaddr = pci_config_readl(bdf, PCI_BASE_ADDRESS_0) & - PCI_BASE_ADDRESS_IO_MASK; - + u16 ioaddr = vp_init_simple(bdf); vdrive_g->ioaddr = ioaddr; - - vp_reset(ioaddr); - vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE | - VIRTIO_CONFIG_S_DRIVER ); - if (vp_find_vq(ioaddr, 0, vdrive_g->vq) < 0 ) { dprintf(1, "fail to find vq for virtio-blk %x:%x\n", pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf)); diff --git a/src/virtio-pci.c b/src/virtio-pci.c index db19e97..6a23d5d 100644 --- a/src/virtio-pci.c +++ b/src/virtio-pci.c @@ -67,3 +67,14 @@ int vp_find_vq(unsigned int ioaddr, int queue_index,
return num; } + +u16 vp_init_simple(u16 bdf) +{ + u16 ioaddr = pci_config_readl(bdf, PCI_BASE_ADDRESS_0) & + PCI_BASE_ADDRESS_IO_MASK; + + vp_reset(ioaddr); + vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE | + VIRTIO_CONFIG_S_DRIVER ); + return ioaddr; +} diff --git a/src/virtio-pci.h b/src/virtio-pci.h index d21d5a5..60e7b35 100644 --- a/src/virtio-pci.h +++ b/src/virtio-pci.h @@ -99,6 +99,7 @@ static inline void vp_del_vq(unsigned int ioaddr, int queue_index) }
struct vring_virtqueue; +u16 vp_init_simple(u16 bdf); int vp_find_vq(unsigned int ioaddr, int queue_index, struct vring_virtqueue *vq); #endif /* _VIRTIO_PCI_H_ */
Another common bit that can be made generic.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/virtio-blk.c | 11 ++++------- src/virtio-pci.c | 22 +++++++++++++++++----- src/virtio-pci.h | 2 +- 3 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/src/virtio-blk.c b/src/virtio-blk.c index 9ed82e0..e85250c 100644 --- a/src/virtio-blk.c +++ b/src/virtio-blk.c @@ -103,20 +103,17 @@ init_virtio_blk(struct pci_device *pci) dprintf(1, "found virtio-blk at %x:%x\n", pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf)); struct virtiodrive_s *vdrive_g = malloc_fseg(sizeof(*vdrive_g)); - struct vring_virtqueue *vq = memalign_low(PAGE_SIZE, sizeof(*vq)); - if (!vdrive_g || !vq) { + if (!vdrive_g) { warn_noalloc(); - goto fail; + return; } memset(vdrive_g, 0, sizeof(*vdrive_g)); - memset(vq, 0, sizeof(*vq)); vdrive_g->drive.type = DTYPE_VIRTIO_BLK; vdrive_g->drive.cntl_id = bdf; - vdrive_g->vq = vq;
u16 ioaddr = vp_init_simple(bdf); vdrive_g->ioaddr = ioaddr; - if (vp_find_vq(ioaddr, 0, vdrive_g->vq) < 0 ) { + if (vp_find_vq(ioaddr, 0, &vdrive_g->vq) < 0 ) { dprintf(1, "fail to find vq for virtio-blk %x:%x\n", pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf)); goto fail; @@ -154,8 +151,8 @@ init_virtio_blk(struct pci_device *pci) return;
fail: + free(vdrive_g->vq); free(vdrive_g); - free(vq); }
void diff --git a/src/virtio-pci.c b/src/virtio-pci.c index 6a23d5d..7e0c1a5 100644 --- a/src/virtio-pci.c +++ b/src/virtio-pci.c @@ -21,12 +21,18 @@ #include "util.h" // dprintf
int vp_find_vq(unsigned int ioaddr, int queue_index, - struct vring_virtqueue *vq) + struct vring_virtqueue **p_vq) { - struct vring * vr = &vq->vring; u16 num;
ASSERT32FLAT(); + struct vring_virtqueue *vq = *p_vq = memalign_low(PAGE_SIZE, sizeof(*vq)); + if (!vq) { + warn_noalloc(); + goto fail; + } + memset(vq, 0, sizeof(*vq)); + /* select the queue */
outw(queue_index, ioaddr + VIRTIO_PCI_QUEUE_SEL); @@ -36,25 +42,26 @@ int vp_find_vq(unsigned int ioaddr, int queue_index, num = inw(ioaddr + VIRTIO_PCI_QUEUE_NUM); if (!num) { dprintf(1, "ERROR: queue size is 0\n"); - return -1; + goto fail; }
if (num > MAX_QUEUE_NUM) { dprintf(1, "ERROR: queue size %d > %d\n", num, MAX_QUEUE_NUM); - return -1; + goto fail; }
/* check if the queue is already active */
if (inl(ioaddr + VIRTIO_PCI_QUEUE_PFN)) { dprintf(1, "ERROR: queue already active\n"); - return -1; + goto fail; }
vq->queue_index = queue_index;
/* initialize the queue */
+ struct vring * vr = &vq->vring; vring_init(vr, num, (unsigned char*)&vq->queue);
/* activate the queue @@ -66,6 +73,11 @@ int vp_find_vq(unsigned int ioaddr, int queue_index, ioaddr + VIRTIO_PCI_QUEUE_PFN);
return num; + +fail: + free(vq); + *p_vq = NULL; + return -1; }
u16 vp_init_simple(u16 bdf) diff --git a/src/virtio-pci.h b/src/virtio-pci.h index 60e7b35..e1d972d 100644 --- a/src/virtio-pci.h +++ b/src/virtio-pci.h @@ -101,5 +101,5 @@ static inline void vp_del_vq(unsigned int ioaddr, int queue_index) struct vring_virtqueue; u16 vp_init_simple(u16 bdf); int vp_find_vq(unsigned int ioaddr, int queue_index, - struct vring_virtqueue *vq); + struct vring_virtqueue **p_vq); #endif /* _VIRTIO_PCI_H_ */
On Wed, Nov 16, 2011 at 01:02:41PM +0100, Paolo Bonzini wrote:
Hi all,
this are the preparatory bits for virtio-scsi support in SeaBIOS. They mostly aim at making code more generic in SeaBIOS for both SCSI and virtio, but (on the SCSI side) they also fix some bugs, so I'm sending them early to get more exposure.
New features are mostly visible on the USB side since that's currently the main consumer of the SCSI implementation. They include more robust LUN detection, support for hard disk geometry, and USB mass storage write support.
I also fixed for real the race that the RFC series was working around.
For virtio, the series adds a few extra interface bits that are common to both virtio-scsi and virtio-blk.
The two parts are logically separate but they have a small conflict, so I'm sending them together.
Thanks Paolo. I committed this series. I modified patch 4 slightly, as I'd prefer to be more explicit about avoiding the divide by zero.
Sven - FYI - this patch set conflicts with your pending patches. Hopefully, the fixes Paolo has will make your device more stable though.
-Kevin
Hi Kevin,
On 11/18/2011 03:48 AM, Kevin O'Connor wrote:
Thanks Paolo. I committed this series. I modified patch 4 slightly, as I'd prefer to be more explicit about avoiding the divide by zero.
Sven - FYI - this patch set conflicts with your pending patches. Hopefully, the fixes Paolo has will make your device more stable though.
I haven't tested (yet), but from reading the patches it looks like they will fix my USB MSC problem, so the TEST_UNIT_READY patch from me shouldn't be needed any longer.
So the only patch i would like to get in is the USB Multi-LUN patch. I can respin that patch based on Paolo's work if you like.
Regards Sven
On Fri, Nov 18, 2011 at 08:46:00AM +0100, Sven Schnelle wrote:
Hi Kevin,
On 11/18/2011 03:48 AM, Kevin O'Connor wrote:
Thanks Paolo. I committed this series. I modified patch 4 slightly, as I'd prefer to be more explicit about avoiding the divide by zero.
Sven - FYI - this patch set conflicts with your pending patches. Hopefully, the fixes Paolo has will make your device more stable though.
I haven't tested (yet), but from reading the patches it looks like they will fix my USB MSC problem, so the TEST_UNIT_READY patch from me shouldn't be needed any longer.
So the only patch i would like to get in is the USB Multi-LUN patch. I can respin that patch based on Paolo's work if you like.
Yes - thanks.
Paolo in your last series, you indicated there were some LUN changes - but I didn't see it in the patches. Do you have something planned here also?
-Kevin
On 11/19/2011 01:49 AM, Kevin O'Connor wrote:
Paolo in your last series, you indicated there were some LUN changes - but I didn't see it in the patches. Do you have something planned here also?
I didn't have LUN changes, in fact I had a FIXME that the virtio-scsi driver was scanning just LUN0.
However, having extracted scsi_init_drive should make Sven's job easier in implementing USB multi-LUN support.
Paolo
-
Kevin O'Connor -
Paolo Bonzini -
Sven Schnelle