> Am 13.11.2015 um 23:42 schrieb John Snow <js...@redhat.com>:
>
>
>
>> 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 <p...@kamp.de>
>> ---
>> 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 <js...@redhat.com>
in Patch 1 the indent is off at the end. Otherwise looks good to me.
Peter
