On 11/13/2015 06:00 PM, Peter Lieven wrote: > > >> Am 13.11.2015 um 23:42 schrieb John Snow <[email protected]>: >> >> >> >>> On 11/12/2015 11:30 AM, Peter Lieven wrote: >>> PIO read requests on the ATAPI interface used to be sync blk requests. >>> This has two significant drawbacks. First the main loop hangs util an >>> I/O request is completed and secondly if the I/O request does not >>> complete (e.g. due to an unresponsive storage) Qemu hangs completely. >>> >>> Note: Due to possible race conditions requests during an ongoing >>> elementary transfer are still sync. >>> >>> Signed-off-by: Peter Lieven <[email protected]> >>> --- >>> hw/ide/atapi.c | 97 >>> ++++++++++++++++++++++++++++++++++++++++++++++++++-------- >>> 1 file changed, 85 insertions(+), 12 deletions(-) >>> >>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c >>> index 747f466..cfd2d63 100644 >>> --- a/hw/ide/atapi.c >>> +++ b/hw/ide/atapi.c >>> @@ -105,33 +105,98 @@ static void cd_data_to_raw(uint8_t *buf, int lba) >>> memset(buf, 0, 288); >>> } >>> >>> -static int cd_read_sector(IDEState *s, int lba, uint8_t *buf, int >>> sector_size) >>> +static int >>> +cd_read_sector_sync(IDEState *s) >>> { >>> int ret; >>> >>> - switch(sector_size) { >>> +#ifdef DEBUG_IDE_ATAPI >>> + printf("cd_read_sector_sync: lba=%d\n", s->lba); >>> +#endif >>> + >>> + switch (s->cd_sector_size) { >>> case 2048: >>> block_acct_start(blk_get_stats(s->blk), &s->acct, >>> 4 * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ); >>> - ret = blk_read(s->blk, (int64_t)lba << 2, buf, 4); >>> + ret = blk_read(s->blk, (int64_t)s->lba << 2, >>> + s->io_buffer, 4); >>> block_acct_done(blk_get_stats(s->blk), &s->acct); >>> break; >>> case 2352: >>> block_acct_start(blk_get_stats(s->blk), &s->acct, >>> 4 * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ); >>> - ret = blk_read(s->blk, (int64_t)lba << 2, buf + 16, 4); >>> + ret = blk_read(s->blk, (int64_t)s->lba << 2, >>> + s->io_buffer + 16, 4); >>> block_acct_done(blk_get_stats(s->blk), &s->acct); >>> if (ret < 0) >>> return ret; >>> - cd_data_to_raw(buf, lba); >>> + cd_data_to_raw(s->io_buffer, s->lba); >>> break; >>> default: >>> ret = -EIO; >>> break; >>> } >>> + >>> + if (!ret) { >>> + s->lba++; >>> + s->io_buffer_index = 0; >>> + } >>> + >>> return ret; >>> } >>> >>> +static void cd_read_sector_cb(void *opaque, int ret) >>> +{ >>> + IDEState *s = opaque; >>> + >>> + block_acct_done(blk_get_stats(s->blk), &s->acct); >>> + >>> +#ifdef DEBUG_IDE_ATAPI >>> + printf("cd_read_sector_cb: lba=%d ret=%d\n", s->lba, ret); >>> +#endif >>> + >>> + if (ret < 0) { >>> + ide_atapi_io_error(s, ret); >>> + return; >>> + } >>> + >>> + if (s->cd_sector_size == 2352) { >>> + cd_data_to_raw(s->io_buffer, s->lba); >>> + } >>> + >>> + s->lba++; >>> + s->io_buffer_index = 0; >>> + s->status &= ~BUSY_STAT; >>> + >>> + ide_atapi_cmd_reply_end(s); >>> +} >>> + >>> +static int cd_read_sector(IDEState *s) >>> +{ >>> + if (s->cd_sector_size != 2048 && s->cd_sector_size != 2352) { >>> + return -EINVAL; >>> + } >>> + >>> + s->iov.iov_base = (s->cd_sector_size == 2352) ? >>> + s->io_buffer + 16 : s->io_buffer; >>> + >>> + s->iov.iov_len = 4 * BDRV_SECTOR_SIZE; >>> + qemu_iovec_init_external(&s->qiov, &s->iov, 1); >>> + >>> +#ifdef DEBUG_IDE_ATAPI >>> + printf("cd_read_sector: lba=%d\n", s->lba); >>> +#endif >>> + >>> + block_acct_start(blk_get_stats(s->blk), &s->acct, >>> + 4 * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ); >>> + >>> + blk_aio_readv(s->blk, (int64_t)s->lba << 2, &s->qiov, 4, >>> + cd_read_sector_cb, s); >>> + >>> + s->status |= BUSY_STAT; >>> + return 0; >>> +} >>> + >>> void ide_atapi_cmd_ok(IDEState *s) >>> { >>> s->error = 0; >>> @@ -182,18 +247,27 @@ void ide_atapi_cmd_reply_end(IDEState *s) >>> ide_atapi_cmd_ok(s); >>> ide_set_irq(s->bus); >>> #ifdef DEBUG_IDE_ATAPI >>> - printf("status=0x%x\n", s->status); >>> + printf("end of transfer, status=0x%x\n", s->status); >>> #endif >>> } else { >>> /* see if a new sector must be read */ >>> if (s->lba != -1 && s->io_buffer_index >= s->cd_sector_size) { >>> - ret = cd_read_sector(s, s->lba, s->io_buffer, >>> s->cd_sector_size); >>> - if (ret < 0) { >>> - ide_atapi_io_error(s, ret); >>> + if (!s->elementary_transfer_size) { >>> + ret = cd_read_sector(s); >>> + if (ret < 0) { >>> + ide_atapi_io_error(s, ret); >>> + } >>> return; >>> + } else { >>> + /* rebuffering within an elementary transfer is >>> + * only possible with a sync request because we >>> + * end up with a race condition otherwise */ >>> + ret = cd_read_sector_sync(s); >>> + if (ret < 0) { >>> + ide_atapi_io_error(s, ret); >>> + return; >>> + } >>> } >>> - s->lba++; >>> - s->io_buffer_index = 0; >>> } >>> if (s->elementary_transfer_size > 0) { >>> /* there are some data left to transmit in this elementary >>> @@ -275,7 +349,6 @@ static void ide_atapi_cmd_read_pio(IDEState *s, int >>> lba, int nb_sectors, >>> s->io_buffer_index = sector_size; >>> s->cd_sector_size = sector_size; >>> >>> - s->status = READY_STAT | SEEK_STAT; >>> ide_atapi_cmd_reply_end(s); >>> } >> >> Had to rebase against Berto's patches, but nothing major. You can see my >> rebase at: >> >> https://github.com/jnsnow/qemu/tree/review-plieven >> >> Reviewed-by: John Snow <[email protected]> > > in Patch 1 the indent is off at the end. Otherwise looks good to me. > > Peter >
Huh, so it is. That part wasn't even in the chunk that conflicted. :\
