* [Qemu-devel] [PATCH for 2.6 v4 0/3] ide: fix loss of the dma/atapi state during migration
@ 2016-04-06 6:40 Denis V. Lunev
2016-04-06 6:40 ` [Qemu-devel] [PATCH 1/3] ide: don't lose pending dma state Denis V. Lunev
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Denis V. Lunev @ 2016-04-06 6:40 UTC (permalink / raw)
To: qemu-devel; +Cc: den, John Snow, Pavel Butsykin
This patch set fixes bugs in the IDE DMA and the IDE ATAPI on operations to
save/restore the state.
>From the user point of view this results in IDE timeouts in the guest
when the user reads from the DVD like the following:
[424332.169229] ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
[424332.170423] sr 0:0:0:0: [sr0] CDB:
[424332.171234] Read(10): 28 00 00 00 02 e4 00 00 01 00
[424332.172418] ata1.00: cmd a0/01:00:00:00:08/00:00:00:00:00/a0 tag 0 dma 2048 in
res 40/00:02:00:0c:00/00:00:00:00:00/a0 Emask 0x4 (timeout)
[424332.174877] ata1.00: status: { DRDY }
[424337.212099] ata1: link is slow to respond, please be patient (ready=0)
[424342.220084] ata1: device not ready (errno=-16), forcing hardreset
[424342.222700] ata1: soft resetting link
[424342.381059] ata1.00: configured for MWDMA2
[424342.383693] ata1: EH complete
Another similar nasty effects are possible.
Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: John Snow <jsnow@redhat.com>
Changes from v1:
- added converter of IDE_DMA_* to IDE_RETRY_* (1)
- fixed handling of the IDE_RETRY_HBA at the ide_restart_bh function (3)
Changes from v2:
- fixed enumeration value ‘IDE_DMA_ATAPI’ not handled in switch for macio.c (3)
Changes from v3:
- move the IDE_DMA_ATAPI setting in the cmd_packet func (3)
Pavel Butsykin (3):
ide: don't lose pending dma state
ide: restart atapi dma by re-evaluating command packet
ide: really restart pending and in-flight atapi dma
hw/ide/atapi.c | 26 ++++++++++++--------------
hw/ide/core.c | 39 ++++++++++++++++-----------------------
hw/ide/internal.h | 36 ++++++++++++++++++++++++++++++++++++
hw/ide/macio.c | 2 ++
hw/ide/pci.c | 4 ++++
5 files changed, 70 insertions(+), 37 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 1/3] ide: don't lose pending dma state 2016-04-06 6:40 [Qemu-devel] [PATCH for 2.6 v4 0/3] ide: fix loss of the dma/atapi state during migration Denis V. Lunev @ 2016-04-06 6:40 ` Denis V. Lunev 2016-04-06 6:40 ` [Qemu-devel] [PATCH 2/3] ide: restart atapi dma by re-evaluating command packet Denis V. Lunev ` (2 subsequent siblings) 3 siblings, 0 replies; 17+ messages in thread From: Denis V. Lunev @ 2016-04-06 6:40 UTC (permalink / raw) To: qemu-devel; +Cc: den, Pavel Butsykin From: Pavel Butsykin <pbutsykin@virtuozzo.com> If the migration occurs after the IDE DMA has been set up but before it has been initiated, the state gets lost upon save/restore. Specifically, ->dma_cb callback gets cleared, so, when the guest eventually starts bus mastering, the DMA never completes, causing the guest to time out the operation. OTOH all the infrastructure is already in place to restart the DMA if the migration happens while the DMA is in progress. So reuse that infrastructure, by setting bus->error_status based on ->dma_cmd in pre_save if ->dma_cb callback is already set but DMAING is clear. This will indicate the need for restart and make sure ->dma_cb is restored in ide_restart_bh(); howeover since DMAING is clear the state upon restore will be exactly "ready for DMA" as before the save. Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> Signed-off-by: Denis V. Lunev <den@openvz.org> Reviewed-by: John Snow <jsnow@redhat.com> --- hw/ide/core.c | 9 +-------- hw/ide/internal.h | 15 +++++++++++++++ hw/ide/pci.c | 4 ++++ 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/hw/ide/core.c b/hw/ide/core.c index 90524d5..58d0687 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -804,14 +804,7 @@ static void ide_dma_cb(void *opaque, int ret) return; } if (ret < 0) { - int op = IDE_RETRY_DMA; - - if (s->dma_cmd == IDE_DMA_READ) - op |= IDE_RETRY_READ; - else if (s->dma_cmd == IDE_DMA_TRIM) - op |= IDE_RETRY_TRIM; - - if (ide_handle_rw_error(s, -ret, op)) { + if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s->dma_cmd))) { return; } } diff --git a/hw/ide/internal.h b/hw/ide/internal.h index 86bde26..68c7d0d 100644 --- a/hw/ide/internal.h +++ b/hw/ide/internal.h @@ -513,6 +513,21 @@ struct IDEDevice { #define IDE_RETRY_TRIM 0x80 #define IDE_RETRY_HBA 0x100 +static inline uint8_t ide_dma_cmd_to_retry(uint8_t dma_cmd) +{ + switch (dma_cmd) { + case IDE_DMA_READ: + return IDE_RETRY_DMA | IDE_RETRY_READ; + case IDE_DMA_WRITE: + return IDE_RETRY_DMA; + case IDE_DMA_TRIM: + return IDE_RETRY_DMA | IDE_RETRY_TRIM; + default: + break; + } + return 0; +} + static inline IDEState *idebus_active_if(IDEBus *bus) { return bus->ifs + bus->unit; diff --git a/hw/ide/pci.c b/hw/ide/pci.c index 92ffee7..8d56a00 100644 --- a/hw/ide/pci.c +++ b/hw/ide/pci.c @@ -308,6 +308,10 @@ static void ide_bmdma_pre_save(void *opaque) BMDMAState *bm = opaque; uint8_t abused_bits = BM_MIGRATION_COMPAT_STATUS_BITS; + if (!(bm->status & BM_STATUS_DMAING) && bm->dma_cb) { + bm->bus->error_status = + ide_dma_cmd_to_retry(bmdma_active_if(bm)->dma_cmd); + } bm->migration_retry_unit = bm->bus->retry_unit; bm->migration_retry_sector_num = bm->bus->retry_sector_num; bm->migration_retry_nsector = bm->bus->retry_nsector; -- 2.1.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 2/3] ide: restart atapi dma by re-evaluating command packet 2016-04-06 6:40 [Qemu-devel] [PATCH for 2.6 v4 0/3] ide: fix loss of the dma/atapi state during migration Denis V. Lunev 2016-04-06 6:40 ` [Qemu-devel] [PATCH 1/3] ide: don't lose pending dma state Denis V. Lunev @ 2016-04-06 6:40 ` Denis V. Lunev 2016-04-06 6:40 ` [Qemu-devel] [PATCH 3/3] ide: really restart pending and in-flight atapi dma Denis V. Lunev 2016-04-11 14:12 ` [Qemu-devel] [PATCH for 2.6 v4 0/3] ide: fix loss of the dma/atapi state during migration Denis V. Lunev 3 siblings, 0 replies; 17+ messages in thread From: Denis V. Lunev @ 2016-04-06 6:40 UTC (permalink / raw) To: qemu-devel; +Cc: den, Pavel Butsykin From: Pavel Butsykin <pbutsykin@virtuozzo.com> ide_atapi_dma_restart() used to just complete the DMA with an error, under the assumption that there isn't enough information to restart it. However, as the contents of the ->io_buffer is preserved, it looks safe to just re-evaluate it and dispatch the ATAPI command again. Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> Signed-off-by: Denis V. Lunev <den@openvz.org> --- hw/ide/atapi.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c index 1fe58ab..acc52cd 100644 --- a/hw/ide/atapi.c +++ b/hw/ide/atapi.c @@ -488,14 +488,13 @@ static void ide_atapi_cmd_read(IDEState *s, int lba, int nb_sectors, void ide_atapi_dma_restart(IDEState *s) { /* - * I'm not sure we have enough stored to restart the command - * safely, so give the guest an error it should recover from. - * I'm assuming most guests will try to recover from something - * listed as a medium error on a CD; it seems to work on Linux. - * This would be more of a problem if we did any other type of - * DMA operation. + * At this point we can just re-evaluate the packet command and start over. + * The presence of ->dma_cb callback in the pre_save ensures that the packet + * command has been completely sent and we can safely restart command. */ - ide_atapi_cmd_error(s, MEDIUM_ERROR, ASC_NO_SEEK_COMPLETE); + s->unit = s->bus->retry_unit; + s->bus->dma->ops->restart_dma(s->bus->dma); + ide_atapi_cmd(s); } static inline uint8_t ide_atapi_set_profile(uint8_t *buf, uint8_t *index, -- 2.1.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 3/3] ide: really restart pending and in-flight atapi dma 2016-04-06 6:40 [Qemu-devel] [PATCH for 2.6 v4 0/3] ide: fix loss of the dma/atapi state during migration Denis V. Lunev 2016-04-06 6:40 ` [Qemu-devel] [PATCH 1/3] ide: don't lose pending dma state Denis V. Lunev 2016-04-06 6:40 ` [Qemu-devel] [PATCH 2/3] ide: restart atapi dma by re-evaluating command packet Denis V. Lunev @ 2016-04-06 6:40 ` Denis V. Lunev 2016-04-11 22:18 ` Eric Blake 2016-04-11 14:12 ` [Qemu-devel] [PATCH for 2.6 v4 0/3] ide: fix loss of the dma/atapi state during migration Denis V. Lunev 3 siblings, 1 reply; 17+ messages in thread From: Denis V. Lunev @ 2016-04-06 6:40 UTC (permalink / raw) To: qemu-devel; +Cc: den, Pavel Butsykin From: Pavel Butsykin <pbutsykin@virtuozzo.com> Restart of ATAPI DMA used to be unreachable, because the request to do so wasn't indicated in bus->error_status due to the lack of spare bits, and ide_restart_bh() would return early doing nothing. This patch makes use of the observation that not all bit combinations were possible in ->error_status. In particular, IDE_RETRY_READ only made sense together with IDE_RETRY_DMA or IDE_RETRY_PIO. This allows to re-use IDE_RETRY_READ alone as an indicator of ATAPI DMA restart request. To makes things more uniform, ATAPI DMA gets its own value for ->dma_cmd. As a means against confusion, macros are added to test the state of ->error_status. The patch fixes the restart of both in-flight and pending ATAPI DMA, following the scheme similar to that of IDE DMA. Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> Signed-off-by: Denis V. Lunev <den@openvz.org> --- hw/ide/atapi.c | 13 ++++++------- hw/ide/core.c | 30 +++++++++++++++--------------- hw/ide/internal.h | 21 +++++++++++++++++++++ hw/ide/macio.c | 2 ++ 4 files changed, 44 insertions(+), 22 deletions(-) diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c index acc52cd..2bb606c 100644 --- a/hw/ide/atapi.c +++ b/hw/ide/atapi.c @@ -375,15 +375,18 @@ static void ide_atapi_cmd_check_status(IDEState *s) } /* ATAPI DMA support */ -/* XXX: handle read errors */ static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret) { IDEState *s = opaque; int data_offset, n; if (ret < 0) { - ide_atapi_io_error(s, ret); - goto eot; + if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s->dma_cmd))) { + if (s->bus->error_status) { + return; + } + goto eot; + } } if (s->io_buffer_size > 0) { @@ -481,10 +484,6 @@ static void ide_atapi_cmd_read(IDEState *s, int lba, int nb_sectors, } } - -/* Called by *_restart_bh when the transfer function points - * to ide_atapi_cmd - */ void ide_atapi_dma_restart(IDEState *s) { /* diff --git a/hw/ide/core.c b/hw/ide/core.c index 58d0687..41e6a2d 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -57,7 +57,6 @@ static const int smart_attributes[][12] = { { 190, 0x03, 0x00, 0x45, 0x45, 0x1f, 0x00, 0x1f, 0x1f, 0x00, 0x00, 0x32}, }; -static int ide_handle_rw_error(IDEState *s, int error, int op); static void ide_dummy_transfer_stop(IDEState *s); static void padstr(char *str, const char *src, int len) @@ -773,7 +772,7 @@ void ide_dma_error(IDEState *s) ide_set_irq(s->bus); } -static int ide_handle_rw_error(IDEState *s, int error, int op) +int ide_handle_rw_error(IDEState *s, int error, int op) { bool is_read = (op & IDE_RETRY_READ) != 0; BlockErrorAction action = blk_get_error_action(s->blk, is_read, error); @@ -783,8 +782,10 @@ static int ide_handle_rw_error(IDEState *s, int error, int op) s->bus->error_status = op; } else if (action == BLOCK_ERROR_ACTION_REPORT) { block_acct_failed(blk_get_stats(s->blk), &s->acct); - if (op & IDE_RETRY_DMA) { + if (IS_IDE_RETRY_DMA(op)) { ide_dma_error(s); + } else if (IS_IDE_RETRY_ATAPI(op)) { + ide_atapi_io_error(s, -error); } else { ide_rw_error(s); } @@ -872,6 +873,8 @@ static void ide_dma_cb(void *opaque, int ret) ide_issue_trim, ide_dma_cb, s, DMA_DIRECTION_TO_DEVICE); break; + default: + abort(); } return; @@ -1634,6 +1637,9 @@ static bool cmd_packet(IDEState *s, uint8_t cmd) s->status = READY_STAT | SEEK_STAT; s->atapi_dma = s->feature & 1; + if (s->atapi_dma) { + s->dma_cmd = IDE_DMA_ATAPI; + } s->nsector = 1; ide_transfer_start(s, s->io_buffer, ATAPI_PACKET_SIZE, ide_atapi_cmd); @@ -2518,15 +2524,13 @@ static void ide_restart_bh(void *opaque) if (s->bus->dma->ops->restart) { s->bus->dma->ops->restart(s->bus->dma); } - } - - if (error_status & IDE_RETRY_DMA) { + } else if (IS_IDE_RETRY_DMA(error_status)) { if (error_status & IDE_RETRY_TRIM) { ide_restart_dma(s, IDE_DMA_TRIM); } else { ide_restart_dma(s, is_read ? IDE_DMA_READ : IDE_DMA_WRITE); } - } else if (error_status & IDE_RETRY_PIO) { + } else if (IS_IDE_RETRY_PIO(error_status)) { if (is_read) { ide_sector_read(s); } else { @@ -2534,15 +2538,11 @@ static void ide_restart_bh(void *opaque) } } else if (error_status & IDE_RETRY_FLUSH) { ide_flush_cache(s); + } else if (IS_IDE_RETRY_ATAPI(error_status)) { + assert(s->end_transfer_func == ide_atapi_cmd); + ide_atapi_dma_restart(s); } else { - /* - * We've not got any bits to tell us about ATAPI - but - * we do have the end_transfer_func that tells us what - * we're trying to do. - */ - if (s->end_transfer_func == ide_atapi_cmd) { - ide_atapi_dma_restart(s); - } + abort(); } } diff --git a/hw/ide/internal.h b/hw/ide/internal.h index 68c7d0d..eb006c2 100644 --- a/hw/ide/internal.h +++ b/hw/ide/internal.h @@ -338,6 +338,7 @@ enum ide_dma_cmd { IDE_DMA_READ, IDE_DMA_WRITE, IDE_DMA_TRIM, + IDE_DMA_ATAPI }; #define ide_cmd_is_read(s) \ @@ -508,11 +509,27 @@ struct IDEDevice { /* These are used for the error_status field of IDEBus */ #define IDE_RETRY_DMA 0x08 #define IDE_RETRY_PIO 0x10 +#define IDE_RETRY_ATAPI 0x20 /* reused IDE_RETRY_READ bit */ #define IDE_RETRY_READ 0x20 #define IDE_RETRY_FLUSH 0x40 #define IDE_RETRY_TRIM 0x80 #define IDE_RETRY_HBA 0x100 +#define IS_IDE_RETRY_DMA(_status) \ + ((_status) & IDE_RETRY_DMA) + +#define IS_IDE_RETRY_PIO(_status) \ + ((_status) & IDE_RETRY_PIO) + +/* + * The method of the IDE_RETRY_ATAPI determination is to use a previously + * impossible bit combination as a new status value. + */ +#define IS_IDE_RETRY_ATAPI(_status) \ + (((_status) & IDE_RETRY_ATAPI) && \ + !IS_IDE_RETRY_DMA(_status) && \ + !IS_IDE_RETRY_PIO(_status)) + static inline uint8_t ide_dma_cmd_to_retry(uint8_t dma_cmd) { switch (dma_cmd) { @@ -522,6 +539,8 @@ static inline uint8_t ide_dma_cmd_to_retry(uint8_t dma_cmd) return IDE_RETRY_DMA; case IDE_DMA_TRIM: return IDE_RETRY_DMA | IDE_RETRY_TRIM; + case IDE_DMA_ATAPI: + return IDE_RETRY_ATAPI; default: break; } @@ -612,4 +631,6 @@ void ide_bus_new(IDEBus *idebus, size_t idebus_size, DeviceState *dev, int bus_id, int max_units); IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive); +int ide_handle_rw_error(IDEState *s, int error, int op); + #endif /* HW_IDE_INTERNAL_H */ diff --git a/hw/ide/macio.c b/hw/ide/macio.c index 1725e5b..76256eb 100644 --- a/hw/ide/macio.c +++ b/hw/ide/macio.c @@ -346,6 +346,8 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) case IDE_DMA_TRIM: pmac_dma_trim(s->blk, offset, io->len, pmac_ide_transfer_cb, io); break; + default: + abort(); } return; -- 2.1.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] ide: really restart pending and in-flight atapi dma 2016-04-06 6:40 ` [Qemu-devel] [PATCH 3/3] ide: really restart pending and in-flight atapi dma Denis V. Lunev @ 2016-04-11 22:18 ` Eric Blake 2016-04-12 12:17 ` Pavel Butsykin 2016-04-12 12:53 ` [Qemu-devel] [PATCH] ide: coding style fix Pavel Butsykin 0 siblings, 2 replies; 17+ messages in thread From: Eric Blake @ 2016-04-11 22:18 UTC (permalink / raw) To: Denis V. Lunev, qemu-devel; +Cc: Pavel Butsykin [-- Attachment #1: Type: text/plain, Size: 3826 bytes --] On 04/06/2016 12:40 AM, Denis V. Lunev wrote: > From: Pavel Butsykin <pbutsykin@virtuozzo.com> > > Restart of ATAPI DMA used to be unreachable, because the request to do > so wasn't indicated in bus->error_status due to the lack of spare bits, and > ide_restart_bh() would return early doing nothing. > > This patch makes use of the observation that not all bit combinations were > possible in ->error_status. In particular, IDE_RETRY_READ only made sense > together with IDE_RETRY_DMA or IDE_RETRY_PIO. This allows to re-use > IDE_RETRY_READ alone as an indicator of ATAPI DMA restart request. > > To makes things more uniform, ATAPI DMA gets its own value for ->dma_cmd. > As a means against confusion, macros are added to test the state of > ->error_status. > > The patch fixes the restart of both in-flight and pending ATAPI DMA, > following the scheme similar to that of IDE DMA. > > Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> > Signed-off-by: Denis V. Lunev <den@openvz.org> > --- I'll leave the technical feasibility of this to others, but have some coding style comments: > @@ -783,8 +782,10 @@ static int ide_handle_rw_error(IDEState *s, int error, int op) > s->bus->error_status = op; > } else if (action == BLOCK_ERROR_ACTION_REPORT) { > block_acct_failed(blk_get_stats(s->blk), &s->acct); > - if (op & IDE_RETRY_DMA) { > + if (IS_IDE_RETRY_DMA(op)) { > ide_dma_error(s); I'd probably have split this into two patches; one adding the accessor macros for existing access, and the other adding the new bit pattern (mixing a conversion along with new code is a bit trickier to review in one patch). > +++ b/hw/ide/internal.h > @@ -338,6 +338,7 @@ enum ide_dma_cmd { > IDE_DMA_READ, > IDE_DMA_WRITE, > IDE_DMA_TRIM, > + IDE_DMA_ATAPI Please keep the trailing comma, so that the next addition to this enum won't have to adjust an existing line. > }; > > #define ide_cmd_is_read(s) \ > @@ -508,11 +509,27 @@ struct IDEDevice { > /* These are used for the error_status field of IDEBus */ > #define IDE_RETRY_DMA 0x08 > #define IDE_RETRY_PIO 0x10 > +#define IDE_RETRY_ATAPI 0x20 /* reused IDE_RETRY_READ bit */ > #define IDE_RETRY_READ 0x20 > #define IDE_RETRY_FLUSH 0x40 > #define IDE_RETRY_TRIM 0x80 > #define IDE_RETRY_HBA 0x100 Seems rather sparse on the comments about which bit patterns are valid together. If IDE_RETRY_READ is always used with at least one other bit, it might make more sense to have an IDE_RETRY_MASK that selects the set of bits being multiplexed, and/or macros that define the bits used in combination. Something along the lines of: #define IDE_RETRY_MASK 0x38 #define IDE_RETRY_READ_DMA 0x28 #define IDE_RETRY_READ_PIO 0x30 #define IDE_RETRY_ATAPI 0x20 > > +#define IS_IDE_RETRY_DMA(_status) \ > + ((_status) & IDE_RETRY_DMA) > + > +#define IS_IDE_RETRY_PIO(_status) \ > + ((_status) & IDE_RETRY_PIO) and these seem prone to false positives; where it might be better to do: #define IS_IDE_RETRY_DMA(_status) \ (((_status) & IDE_RETRY_MASK) == IDE_RETRY_READ_DMA) > + > +/* > + * The method of the IDE_RETRY_ATAPI determination is to use a previously > + * impossible bit combination as a new status value. > + */ > +#define IS_IDE_RETRY_ATAPI(_status) \ > + (((_status) & IDE_RETRY_ATAPI) && \ > + !IS_IDE_RETRY_DMA(_status) && \ > + !IS_IDE_RETRY_PIO(_status)) > + And this evaluates _status more than once, compared to: #define IS_IDE_RETRY_ATAPI(_status) \ (((_status) & IDE_RETRY_MASK) == IDE_RETRY_ATAPI) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] ide: really restart pending and in-flight atapi dma 2016-04-11 22:18 ` Eric Blake @ 2016-04-12 12:17 ` Pavel Butsykin 2016-04-12 16:47 ` John Snow 2016-04-12 12:53 ` [Qemu-devel] [PATCH] ide: coding style fix Pavel Butsykin 1 sibling, 1 reply; 17+ messages in thread From: Pavel Butsykin @ 2016-04-12 12:17 UTC (permalink / raw) To: Eric Blake, Denis V. Lunev, qemu-devel On 12.04.2016 01:18, Eric Blake wrote: > On 04/06/2016 12:40 AM, Denis V. Lunev wrote: >> From: Pavel Butsykin <pbutsykin@virtuozzo.com> >> >> Restart of ATAPI DMA used to be unreachable, because the request to do >> so wasn't indicated in bus->error_status due to the lack of spare bits, and >> ide_restart_bh() would return early doing nothing. >> >> This patch makes use of the observation that not all bit combinations were >> possible in ->error_status. In particular, IDE_RETRY_READ only made sense >> together with IDE_RETRY_DMA or IDE_RETRY_PIO. This allows to re-use >> IDE_RETRY_READ alone as an indicator of ATAPI DMA restart request. >> >> To makes things more uniform, ATAPI DMA gets its own value for ->dma_cmd. >> As a means against confusion, macros are added to test the state of >> ->error_status. >> >> The patch fixes the restart of both in-flight and pending ATAPI DMA, >> following the scheme similar to that of IDE DMA. >> >> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> --- > > I'll leave the technical feasibility of this to others, but have some > coding style comments: > > >> @@ -783,8 +782,10 @@ static int ide_handle_rw_error(IDEState *s, int error, int op) >> s->bus->error_status = op; >> } else if (action == BLOCK_ERROR_ACTION_REPORT) { >> block_acct_failed(blk_get_stats(s->blk), &s->acct); >> - if (op & IDE_RETRY_DMA) { >> + if (IS_IDE_RETRY_DMA(op)) { >> ide_dma_error(s); > > I'd probably have split this into two patches; one adding the accessor > macros for existing access, and the other adding the new bit pattern > (mixing a conversion along with new code is a bit trickier to review in > one patch). > > >> +++ b/hw/ide/internal.h >> @@ -338,6 +338,7 @@ enum ide_dma_cmd { >> IDE_DMA_READ, >> IDE_DMA_WRITE, >> IDE_DMA_TRIM, >> + IDE_DMA_ATAPI > > Please keep the trailing comma, so that the next addition to this enum > won't have to adjust an existing line. > ok. >> }; >> >> #define ide_cmd_is_read(s) \ >> @@ -508,11 +509,27 @@ struct IDEDevice { >> /* These are used for the error_status field of IDEBus */ >> #define IDE_RETRY_DMA 0x08 >> #define IDE_RETRY_PIO 0x10 >> +#define IDE_RETRY_ATAPI 0x20 /* reused IDE_RETRY_READ bit */ >> #define IDE_RETRY_READ 0x20 >> #define IDE_RETRY_FLUSH 0x40 >> #define IDE_RETRY_TRIM 0x80 >> #define IDE_RETRY_HBA 0x100 > > Seems rather sparse on the comments about which bit patterns are valid > together. If IDE_RETRY_READ is always used with at least one other bit, > it might make more sense to have an IDE_RETRY_MASK that selects the set > of bits being multiplexed, and/or macros that define the bits used in > combination. Something along the lines of: > > #define IDE_RETRY_MASK 0x38 > #define IDE_RETRY_READ_DMA 0x28 > #define IDE_RETRY_READ_PIO 0x30 > #define IDE_RETRY_ATAPI 0x20 > >> >> +#define IS_IDE_RETRY_DMA(_status) \ >> + ((_status) & IDE_RETRY_DMA) >> + >> +#define IS_IDE_RETRY_PIO(_status) \ >> + ((_status) & IDE_RETRY_PIO) > > and these seem prone to false positives; where it might be better to do: > > #define IS_IDE_RETRY_DMA(_status) \ > (((_status) & IDE_RETRY_MASK) == IDE_RETRY_READ_DMA) > This is not entirely true, because IDE_RETRY_DMA can be used for READ or WRITE operation. >> + >> +/* >> + * The method of the IDE_RETRY_ATAPI determination is to use a previously >> + * impossible bit combination as a new status value. >> + */ >> +#define IS_IDE_RETRY_ATAPI(_status) \ >> + (((_status) & IDE_RETRY_ATAPI) && \ >> + !IS_IDE_RETRY_DMA(_status) && \ >> + !IS_IDE_RETRY_PIO(_status)) >> + > > And this evaluates _status more than once, compared to: > > #define IS_IDE_RETRY_ATAPI(_status) \ > (((_status) & IDE_RETRY_MASK) == IDE_RETRY_ATAPI) > > Yes, it looks much nicer. I can make the change as a follow-up patch. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] ide: really restart pending and in-flight atapi dma 2016-04-12 12:17 ` Pavel Butsykin @ 2016-04-12 16:47 ` John Snow 0 siblings, 0 replies; 17+ messages in thread From: John Snow @ 2016-04-12 16:47 UTC (permalink / raw) To: Pavel Butsykin, Eric Blake, Denis V. Lunev, qemu-devel On 04/12/2016 08:17 AM, Pavel Butsykin wrote: > On 12.04.2016 01:18, Eric Blake wrote: >> On 04/06/2016 12:40 AM, Denis V. Lunev wrote: >>> From: Pavel Butsykin <pbutsykin@virtuozzo.com> >>> >>> Restart of ATAPI DMA used to be unreachable, because the request to do >>> so wasn't indicated in bus->error_status due to the lack of spare >>> bits, and >>> ide_restart_bh() would return early doing nothing. >>> >>> This patch makes use of the observation that not all bit combinations >>> were >>> possible in ->error_status. In particular, IDE_RETRY_READ only made >>> sense >>> together with IDE_RETRY_DMA or IDE_RETRY_PIO. This allows to re-use >>> IDE_RETRY_READ alone as an indicator of ATAPI DMA restart request. >>> >>> To makes things more uniform, ATAPI DMA gets its own value for >>> ->dma_cmd. >>> As a means against confusion, macros are added to test the state of >>> ->error_status. >>> >>> The patch fixes the restart of both in-flight and pending ATAPI DMA, >>> following the scheme similar to that of IDE DMA. >>> >>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>> and these seem prone to false positives; where it might be better to do: >>> Signed-off-by: Denis V. Lunev <den@openvz.org> >>> --- >> >> I'll leave the technical feasibility of this to others, but have some >> coding style comments: >> >> >>> @@ -783,8 +782,10 @@ static int ide_handle_rw_error(IDEState *s, int >>> error, int op) >>> s->bus->error_status = op; >>> } else if (action == BLOCK_ERROR_ACTION_REPORT) { >>> block_acct_failed(blk_get_stats(s->blk), &s->acct); >>> - if (op & IDE_RETRY_DMA) { >>> + if (IS_IDE_RETRY_DMA(op)) { >>> ide_dma_error(s); >> >> I'd probably have split this into two patches; one adding the accessor >> macros for existing access, and the other adding the new bit pattern >> (mixing a conversion along with new code is a bit trickier to review in >> one patch). >> >> >>> +++ b/hw/ide/internal.h >>> @@ -338,6 +338,7 @@ enum ide_dma_cmd { >>> IDE_DMA_READ, >>> IDE_DMA_WRITE, >>> IDE_DMA_TRIM, >>> + IDE_DMA_ATAPI >> >> Please keep the trailing comma, so that the next addition to this enum >> won't have to adjust an existing line. >> > ok. > >>> }; >>> >>> #define ide_cmd_is_read(s) \ >>> @@ -508,11 +509,27 @@ struct IDEDevice { >>> /* These are used for the error_status field of IDEBus */ >>> #define IDE_RETRY_DMA 0x08 >>> #define IDE_RETRY_PIO 0x10 >>> +#define IDE_RETRY_ATAPI 0x20 /* reused IDE_RETRY_READ bit */ >>> #define IDE_RETRY_READ 0x20 >>> #define IDE_RETRY_FLUSH 0x40 >>> #define IDE_RETRY_TRIM 0x80 >>> #define IDE_RETRY_HBA 0x100 >> >> Seems rather sparse on the comments about which bit patterns are valid >> together. If IDE_RETRY_READ is always used with at least one other bit, >> it might make more sense to have an IDE_RETRY_MASK that selects the set >> of bits being multiplexed, and/or macros that define the bits used in >> combination. Something along the lines of: >> >> #define IDE_RETRY_MASK 0x38 >> #define IDE_RETRY_READ_DMA 0x28 >> #define IDE_RETRY_READ_PIO 0x30 >> #define IDE_RETRY_ATAPI 0x20 >> >>> >>> +#define IS_IDE_RETRY_DMA(_status) \ >>> + ((_status) & IDE_RETRY_DMA) >>> + >>> +#define IS_IDE_RETRY_PIO(_status) \ >>> + ((_status) & IDE_RETRY_PIO) >> >> and these seem prone to false positives; where it might be better to do: >> >> #define IS_IDE_RETRY_DMA(_status) \ >> (((_status) & IDE_RETRY_MASK) == IDE_RETRY_READ_DMA) >> > This is not entirely true, because IDE_RETRY_DMA can be used for READ or > WRITE operation. > >>> + >>> +/* >>> + * The method of the IDE_RETRY_ATAPI determination is to use a >>> previously >>> + * impossible bit combination as a new status value. >>> + */ >>> +#define IS_IDE_RETRY_ATAPI(_status) \ >>> + (((_status) & IDE_RETRY_ATAPI) && \ >>> + !IS_IDE_RETRY_DMA(_status) && \ >>> + !IS_IDE_RETRY_PIO(_status)) >>> + >> >> And this evaluates _status more than once, compared to: >> >> #define IS_IDE_RETRY_ATAPI(_status) \ >> (((_status) & IDE_RETRY_MASK) == IDE_RETRY_ATAPI) >> >> > Yes, it looks much nicer. I can make the change as a follow-up patch. > I can squash the patch in staging. Thanks, --js ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH] ide: coding style fix 2016-04-11 22:18 ` Eric Blake 2016-04-12 12:17 ` Pavel Butsykin @ 2016-04-12 12:53 ` Pavel Butsykin 1 sibling, 0 replies; 17+ messages in thread From: Pavel Butsykin @ 2016-04-12 12:53 UTC (permalink / raw) To: jsnow; +Cc: qemu-devel, eblake, den, Pavel Butsykin Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> --- hw/ide/internal.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/hw/ide/internal.h b/hw/ide/internal.h index eb006c2..d2c458f 100644 --- a/hw/ide/internal.h +++ b/hw/ide/internal.h @@ -338,7 +338,7 @@ enum ide_dma_cmd { IDE_DMA_READ, IDE_DMA_WRITE, IDE_DMA_TRIM, - IDE_DMA_ATAPI + IDE_DMA_ATAPI, }; #define ide_cmd_is_read(s) \ @@ -507,6 +507,7 @@ struct IDEDevice { }; /* These are used for the error_status field of IDEBus */ +#define IDE_RETRY_MASK 0xf8 #define IDE_RETRY_DMA 0x08 #define IDE_RETRY_PIO 0x10 #define IDE_RETRY_ATAPI 0x20 /* reused IDE_RETRY_READ bit */ @@ -526,9 +527,7 @@ struct IDEDevice { * impossible bit combination as a new status value. */ #define IS_IDE_RETRY_ATAPI(_status) \ - (((_status) & IDE_RETRY_ATAPI) && \ - !IS_IDE_RETRY_DMA(_status) && \ - !IS_IDE_RETRY_PIO(_status)) + (((_status) & IDE_RETRY_MASK) == IDE_RETRY_ATAPI) static inline uint8_t ide_dma_cmd_to_retry(uint8_t dma_cmd) { -- 2.7.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH for 2.6 v4 0/3] ide: fix loss of the dma/atapi state during migration 2016-04-06 6:40 [Qemu-devel] [PATCH for 2.6 v4 0/3] ide: fix loss of the dma/atapi state during migration Denis V. Lunev ` (2 preceding siblings ...) 2016-04-06 6:40 ` [Qemu-devel] [PATCH 3/3] ide: really restart pending and in-flight atapi dma Denis V. Lunev @ 2016-04-11 14:12 ` Denis V. Lunev 2016-04-11 21:47 ` John Snow 3 siblings, 1 reply; 17+ messages in thread From: Denis V. Lunev @ 2016-04-11 14:12 UTC (permalink / raw) To: qemu-devel; +Cc: Pavel Butsykin, John Snow On 04/06/2016 09:40 AM, Denis V. Lunev wrote: > This patch set fixes bugs in the IDE DMA and the IDE ATAPI on operations to > save/restore the state. > > From the user point of view this results in IDE timeouts in the guest > when the user reads from the DVD like the following: > > [424332.169229] ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen > [424332.170423] sr 0:0:0:0: [sr0] CDB: > [424332.171234] Read(10): 28 00 00 00 02 e4 00 00 01 00 > [424332.172418] ata1.00: cmd a0/01:00:00:00:08/00:00:00:00:00/a0 tag 0 dma 2048 in > res 40/00:02:00:0c:00/00:00:00:00:00/a0 Emask 0x4 (timeout) > [424332.174877] ata1.00: status: { DRDY } > [424337.212099] ata1: link is slow to respond, please be patient (ready=0) > [424342.220084] ata1: device not ready (errno=-16), forcing hardreset > [424342.222700] ata1: soft resetting link > [424342.381059] ata1.00: configured for MWDMA2 > [424342.383693] ata1: EH complete > > Another similar nasty effects are possible. > > Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: John Snow <jsnow@redhat.com> > > Changes from v1: > - added converter of IDE_DMA_* to IDE_RETRY_* (1) > - fixed handling of the IDE_RETRY_HBA at the ide_restart_bh function (3) > > Changes from v2: > - fixed enumeration value ‘IDE_DMA_ATAPI’ not handled in switch for macio.c (3) > > Changes from v3: > - move the IDE_DMA_ATAPI setting in the cmd_packet func (3) > > Pavel Butsykin (3): > ide: don't lose pending dma state > ide: restart atapi dma by re-evaluating command packet > ide: really restart pending and in-flight atapi dma > > hw/ide/atapi.c | 26 ++++++++++++-------------- > hw/ide/core.c | 39 ++++++++++++++++----------------------- > hw/ide/internal.h | 36 ++++++++++++++++++++++++++++++++++++ > hw/ide/macio.c | 2 ++ > hw/ide/pci.c | 4 ++++ > 5 files changed, 70 insertions(+), 37 deletions(-) > ping ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH for 2.6 v4 0/3] ide: fix loss of the dma/atapi state during migration 2016-04-11 14:12 ` [Qemu-devel] [PATCH for 2.6 v4 0/3] ide: fix loss of the dma/atapi state during migration Denis V. Lunev @ 2016-04-11 21:47 ` John Snow 2016-04-12 5:33 ` Denis V. Lunev 0 siblings, 1 reply; 17+ messages in thread From: John Snow @ 2016-04-11 21:47 UTC (permalink / raw) To: Denis V. Lunev, qemu-devel; +Cc: Pavel Butsykin On 04/11/2016 10:12 AM, Denis V. Lunev wrote: > On 04/06/2016 09:40 AM, Denis V. Lunev wrote: >> This patch set fixes bugs in the IDE DMA and the IDE ATAPI on >> operations to >> save/restore the state. >> >> From the user point of view this results in IDE timeouts in the guest >> when the user reads from the DVD like the following: >> >> [424332.169229] ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action >> 0x6 frozen >> [424332.170423] sr 0:0:0:0: [sr0] CDB: >> [424332.171234] Read(10): 28 00 00 00 02 e4 00 00 01 00 >> [424332.172418] ata1.00: cmd a0/01:00:00:00:08/00:00:00:00:00/a0 tag 0 >> dma 2048 in >> res 40/00:02:00:0c:00/00:00:00:00:00/a0 Emask 0x4 (timeout) >> [424332.174877] ata1.00: status: { DRDY } >> [424337.212099] ata1: link is slow to respond, please be patient >> (ready=0) >> [424342.220084] ata1: device not ready (errno=-16), forcing hardreset >> [424342.222700] ata1: soft resetting link >> [424342.381059] ata1.00: configured for MWDMA2 >> [424342.383693] ata1: EH complete >> >> Another similar nasty effects are possible. >> >> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> >> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> CC: John Snow <jsnow@redhat.com> >> >> Changes from v1: >> - added converter of IDE_DMA_* to IDE_RETRY_* (1) >> - fixed handling of the IDE_RETRY_HBA at the ide_restart_bh function (3) >> >> Changes from v2: >> - fixed enumeration value ‘IDE_DMA_ATAPI’ not handled in switch for >> macio.c (3) >> >> Changes from v3: >> - move the IDE_DMA_ATAPI setting in the cmd_packet func (3) >> >> Pavel Butsykin (3): >> ide: don't lose pending dma state >> ide: restart atapi dma by re-evaluating command packet >> ide: really restart pending and in-flight atapi dma >> >> hw/ide/atapi.c | 26 ++++++++++++-------------- >> hw/ide/core.c | 39 ++++++++++++++++----------------------- >> hw/ide/internal.h | 36 ++++++++++++++++++++++++++++++++++++ >> hw/ide/macio.c | 2 ++ >> hw/ide/pci.c | 4 ++++ >> 5 files changed, 70 insertions(+), 37 deletions(-) >> > ping Sorry, I don't appear to have been CC'd on the actual patch emails, so I missed them. The individual patch emails also miss the "v4" tag which makes them hard to spot.... and it's hard to figure out what's been changed or not since my reviewed-by was added to all three patches, even though I only acknowledged the first 2/3. Anyway, Thanks, applied to my IDE tree: https://github.com/jnsnow/qemu/commits/ide https://github.com/jnsnow/qemu.git --js ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH for 2.6 v4 0/3] ide: fix loss of the dma/atapi state during migration 2016-04-11 21:47 ` John Snow @ 2016-04-12 5:33 ` Denis V. Lunev 2016-04-12 16:24 ` John Snow 0 siblings, 1 reply; 17+ messages in thread From: Denis V. Lunev @ 2016-04-12 5:33 UTC (permalink / raw) To: John Snow, qemu-devel; +Cc: Pavel Butsykin On 04/12/2016 12:47 AM, John Snow wrote: > > On 04/11/2016 10:12 AM, Denis V. Lunev wrote: >> On 04/06/2016 09:40 AM, Denis V. Lunev wrote: >>> This patch set fixes bugs in the IDE DMA and the IDE ATAPI on >>> operations to >>> save/restore the state. >>> >>> From the user point of view this results in IDE timeouts in the guest >>> when the user reads from the DVD like the following: >>> >>> [424332.169229] ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action >>> 0x6 frozen >>> [424332.170423] sr 0:0:0:0: [sr0] CDB: >>> [424332.171234] Read(10): 28 00 00 00 02 e4 00 00 01 00 >>> [424332.172418] ata1.00: cmd a0/01:00:00:00:08/00:00:00:00:00/a0 tag 0 >>> dma 2048 in >>> res 40/00:02:00:0c:00/00:00:00:00:00/a0 Emask 0x4 (timeout) >>> [424332.174877] ata1.00: status: { DRDY } >>> [424337.212099] ata1: link is slow to respond, please be patient >>> (ready=0) >>> [424342.220084] ata1: device not ready (errno=-16), forcing hardreset >>> [424342.222700] ata1: soft resetting link >>> [424342.381059] ata1.00: configured for MWDMA2 >>> [424342.383693] ata1: EH complete >>> >>> Another similar nasty effects are possible. >>> >>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> >>> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> >>> Signed-off-by: Denis V. Lunev <den@openvz.org> >>> CC: John Snow <jsnow@redhat.com> >>> >>> Changes from v1: >>> - added converter of IDE_DMA_* to IDE_RETRY_* (1) >>> - fixed handling of the IDE_RETRY_HBA at the ide_restart_bh function (3) >>> >>> Changes from v2: >>> - fixed enumeration value ‘IDE_DMA_ATAPI’ not handled in switch for >>> macio.c (3) >>> >>> Changes from v3: >>> - move the IDE_DMA_ATAPI setting in the cmd_packet func (3) >>> >>> Pavel Butsykin (3): >>> ide: don't lose pending dma state >>> ide: restart atapi dma by re-evaluating command packet >>> ide: really restart pending and in-flight atapi dma >>> >>> hw/ide/atapi.c | 26 ++++++++++++-------------- >>> hw/ide/core.c | 39 ++++++++++++++++----------------------- >>> hw/ide/internal.h | 36 ++++++++++++++++++++++++++++++++++++ >>> hw/ide/macio.c | 2 ++ >>> hw/ide/pci.c | 4 ++++ >>> 5 files changed, 70 insertions(+), 37 deletions(-) >>> >> ping > Sorry, I don't appear to have been CC'd on the actual patch emails, so I > missed them. The individual patch emails also miss the "v4" tag which > makes them hard to spot.... and it's hard to figure out what's been > changed or not since my reviewed-by was added to all three patches, even > though I only acknowledged the first 2/3. > this is strange for me - you R-b: was added only to patches 1-2, here is a quote "The patch fixes the restart of both in-flight and pending ATAPI DMA, following the scheme similar to that of IDE DMA. Signed-off-by: Pavel Butsykin<pbutsykin@virtuozzo.com> Signed-off-by: Denis V. Lunev<den@openvz.org> --- hw/ide/atapi.c | 13 ++++++------- hw/ide/core.c | 30 +++++++++++++++--------------- hw/ide/internal.h | 21 +++++++++++++++++++++ hw/ide/macio.c | 2 ++ 4 files changed, 44 insertions(+), 22 deletions(-)" this letter was sent at "04/06/2016 09:40 AM" Den ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH for 2.6 v4 0/3] ide: fix loss of the dma/atapi state during migration 2016-04-12 5:33 ` Denis V. Lunev @ 2016-04-12 16:24 ` John Snow 0 siblings, 0 replies; 17+ messages in thread From: John Snow @ 2016-04-12 16:24 UTC (permalink / raw) To: Denis V. Lunev, qemu-devel; +Cc: Pavel Butsykin On 04/12/2016 01:33 AM, Denis V. Lunev wrote: > On 04/12/2016 12:47 AM, John Snow wrote: >> >> On 04/11/2016 10:12 AM, Denis V. Lunev wrote: >>> On 04/06/2016 09:40 AM, Denis V. Lunev wrote: >>>> This patch set fixes bugs in the IDE DMA and the IDE ATAPI on >>>> operations to >>>> save/restore the state. >>>> >>>> From the user point of view this results in IDE timeouts in the guest >>>> when the user reads from the DVD like the following: >>>> >>>> [424332.169229] ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action >>>> 0x6 frozen >>>> [424332.170423] sr 0:0:0:0: [sr0] CDB: >>>> [424332.171234] Read(10): 28 00 00 00 02 e4 00 00 01 00 >>>> [424332.172418] ata1.00: cmd a0/01:00:00:00:08/00:00:00:00:00/a0 tag 0 >>>> dma 2048 in >>>> res 40/00:02:00:0c:00/00:00:00:00:00/a0 Emask 0x4 (timeout) >>>> [424332.174877] ata1.00: status: { DRDY } >>>> [424337.212099] ata1: link is slow to respond, please be patient >>>> (ready=0) >>>> [424342.220084] ata1: device not ready (errno=-16), forcing hardreset >>>> [424342.222700] ata1: soft resetting link >>>> [424342.381059] ata1.00: configured for MWDMA2 >>>> [424342.383693] ata1: EH complete >>>> >>>> Another similar nasty effects are possible. >>>> >>>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> >>>> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> >>>> Signed-off-by: Denis V. Lunev <den@openvz.org> >>>> CC: John Snow <jsnow@redhat.com> >>>> >>>> Changes from v1: >>>> - added converter of IDE_DMA_* to IDE_RETRY_* (1) >>>> - fixed handling of the IDE_RETRY_HBA at the ide_restart_bh function >>>> (3) >>>> >>>> Changes from v2: >>>> - fixed enumeration value ‘IDE_DMA_ATAPI’ not handled in switch for >>>> macio.c (3) >>>> >>>> Changes from v3: >>>> - move the IDE_DMA_ATAPI setting in the cmd_packet func (3) >>>> >>>> Pavel Butsykin (3): >>>> ide: don't lose pending dma state >>>> ide: restart atapi dma by re-evaluating command packet >>>> ide: really restart pending and in-flight atapi dma >>>> >>>> hw/ide/atapi.c | 26 ++++++++++++-------------- >>>> hw/ide/core.c | 39 ++++++++++++++++----------------------- >>>> hw/ide/internal.h | 36 ++++++++++++++++++++++++++++++++++++ >>>> hw/ide/macio.c | 2 ++ >>>> hw/ide/pci.c | 4 ++++ >>>> 5 files changed, 70 insertions(+), 37 deletions(-) >>>> >>> ping >> Sorry, I don't appear to have been CC'd on the actual patch emails, so I >> missed them. The individual patch emails also miss the "v4" tag which >> makes them hard to spot.... and it's hard to figure out what's been >> changed or not since my reviewed-by was added to all three patches, even >> though I only acknowledged the first 2/3.an reaction time of about 265 millise >> > this is strange for me - you R-b: was added only to patches 1-2, here is > a quote > > "The patch fixes the restart of both in-flight and pending ATAPI DMA, > following the scheme similar to that of IDE DMA. > > Signed-off-by: Pavel Butsykin<pbutsykin@virtuozzo.com> > Signed-off-by: Denis V. Lunev<den@openvz.org> > --- > hw/ide/atapi.c | 13 ++++++------- > hw/ide/core.c | 30 +++++++++++++++--------------- > hw/ide/internal.h | 21 +++++++++++++++++++++ > hw/ide/macio.c | 2 ++ > 4 files changed, 44 insertions(+), 22 deletions(-)" > > this letter was sent at "04/06/2016 09:40 AM" > > Den Odd. Maybe because the "V4" tags were missing, it confused our patches tool? You're right, I don't see it in the mail itself, sorry. --js ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v3 0/3] ide: fix loss of the dma/atapi state during migration
@ 2016-04-01 14:32 Denis V. Lunev
2016-04-01 14:32 ` [Qemu-devel] [PATCH 2/3] ide: restart atapi dma by re-evaluating command packet Denis V. Lunev
0 siblings, 1 reply; 17+ messages in thread
From: Denis V. Lunev @ 2016-04-01 14:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Denis V. Lunev, jsnow, rkagan, Pavel Butsykin
This patch set fixes bugs in the IDE DMA and the IDE ATAPI on operations to
save/restore the state.
>From the user point of view this results in IDE timeouts in the guest
when the user reads from the DVD like the following:
[424332.169229] ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
[424332.170423] sr 0:0:0:0: [sr0] CDB:
[424332.171234] Read(10): 28 00 00 00 02 e4 00 00 01 00
[424332.172418] ata1.00: cmd a0/01:00:00:00:08/00:00:00:00:00/a0 tag 0 dma 2048 in
res 40/00:02:00:0c:00/00:00:00:00:00/a0 Emask 0x4 (timeout)
[424332.174877] ata1.00: status: { DRDY }
[424337.212099] ata1: link is slow to respond, please be patient (ready=0)
[424342.220084] ata1: device not ready (errno=-16), forcing hardreset
[424342.222700] ata1: soft resetting link
[424342.381059] ata1.00: configured for MWDMA2
[424342.383693] ata1: EH complete
Another similar nasty effects are possible.
Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: John Snow <jsnow@redhat.com>
Changes from v1:
- added converter of IDE_DMA_* to IDE_RETRY_* (1)
- fixed handling of the IDE_RETRY_HBA at the ide_restart_bh function (3)
Changes from v2:
- fixed enumeration value ‘IDE_DMA_ATAPI’ not handled in switch for macio.c (3)
Pavel Butsykin (3):
ide: don't lose pending dma state
ide: restart atapi dma by re-evaluating command packet
ide: really restart pending and in-flight atapi dma
hw/ide/atapi.c | 28 ++++++++++++++--------------
hw/ide/core.c | 36 +++++++++++++-----------------------
hw/ide/internal.h | 36 ++++++++++++++++++++++++++++++++++++
hw/ide/macio.c | 2 ++
hw/ide/pci.c | 4 ++++
5 files changed, 69 insertions(+), 37 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 2/3] ide: restart atapi dma by re-evaluating command packet 2016-04-01 14:32 [Qemu-devel] [PATCH v3 " Denis V. Lunev @ 2016-04-01 14:32 ` Denis V. Lunev 2016-04-01 21:01 ` John Snow 0 siblings, 1 reply; 17+ messages in thread From: Denis V. Lunev @ 2016-04-01 14:32 UTC (permalink / raw) To: qemu-devel; +Cc: Denis V. Lunev, jsnow, rkagan, Pavel Butsykin From: Pavel Butsykin <pbutsykin@virtuozzo.com> ide_atapi_dma_restart() used to just complete the DMA with an error, under the assumption that there isn't enough information to restart it. However, as the contents of the ->io_buffer is preserved, it looks safe to just re-evaluate it and dispatch the ATAPI command again. Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> Signed-off-by: Denis V. Lunev <den@openvz.org> --- hw/ide/atapi.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c index 1fe58ab..acc52cd 100644 --- a/hw/ide/atapi.c +++ b/hw/ide/atapi.c @@ -488,14 +488,13 @@ static void ide_atapi_cmd_read(IDEState *s, int lba, int nb_sectors, void ide_atapi_dma_restart(IDEState *s) { /* - * I'm not sure we have enough stored to restart the command - * safely, so give the guest an error it should recover from. - * I'm assuming most guests will try to recover from something - * listed as a medium error on a CD; it seems to work on Linux. - * This would be more of a problem if we did any other type of - * DMA operation. + * At this point we can just re-evaluate the packet command and start over. + * The presence of ->dma_cb callback in the pre_save ensures that the packet + * command has been completely sent and we can safely restart command. */ - ide_atapi_cmd_error(s, MEDIUM_ERROR, ASC_NO_SEEK_COMPLETE); + s->unit = s->bus->retry_unit; + s->bus->dma->ops->restart_dma(s->bus->dma); + ide_atapi_cmd(s); } static inline uint8_t ide_atapi_set_profile(uint8_t *buf, uint8_t *index, -- 2.1.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] ide: restart atapi dma by re-evaluating command packet 2016-04-01 14:32 ` [Qemu-devel] [PATCH 2/3] ide: restart atapi dma by re-evaluating command packet Denis V. Lunev @ 2016-04-01 21:01 ` John Snow 2016-04-04 10:32 ` Pavel Butsykin 0 siblings, 1 reply; 17+ messages in thread From: John Snow @ 2016-04-01 21:01 UTC (permalink / raw) To: Denis V. Lunev, qemu-devel; +Cc: Paolo Bonzini, rkagan, Pavel Butsykin On 04/01/2016 10:32 AM, Denis V. Lunev wrote: > From: Pavel Butsykin <pbutsykin@virtuozzo.com> > > ide_atapi_dma_restart() used to just complete the DMA with an error, > under the assumption that there isn't enough information to restart it. > > However, as the contents of the ->io_buffer is preserved, it looks safe to > just re-evaluate it and dispatch the ATAPI command again. > > Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> > Signed-off-by: Denis V. Lunev <den@openvz.org> > --- > hw/ide/atapi.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 dweletions(-) > > diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c > index 1fe58ab..acc52cd 100644 > --- a/hw/ide/atapi.c > +++ b/hw/ide/atapi.c > @@ -488,14 +488,13 @@ static void ide_atapi_cmd_read(IDEState *s, int lba, int nb_sectors, > void ide_atapi_dma_restart(IDEState *s) > { > /* > - * I'm not sure we have enough stored to restart the command > - * safely, so give the guest an error it should recover from. > - * I'm assuming most guests will try to recover from something > - * listed as a medium error on a CD; it seems to work on Linux. > - * This would be more of a problem if we did any other type of > - * DMA operation. > + * At this point we can just re-evaluate the packet command and start over. > + * The presence of ->dma_cb callback in the pre_save ensures that the packet > + * command has been completely sent and we can safely restart command. > */ > - ide_atapi_cmd_error(s, MEDIUM_ERROR, ASC_NO_SEEK_COMPLETE); > + s->unit = s->bus->retry_unit; > + s->bus->dma->ops->restart_dma(s->bus->dma); > + ide_atapi_cmd(s); > } > > static inline uint8_t ide_atapi_set_profile(uint8_t *buf, uint8_t *index, > Is it at all possible that a previous command may have edited the s->io_buffer that ide_atapi_cmd() uses for SCSI command dispatch? Let me try to answer my own question. Here's my understanding: On state change, ide_restart_bh is invoked unconditionally. If end_transfer_func is ide_atapi_cmd, we invoke ide_atapi_dma_restart. What are the conditions for end_transfer_func being set to ide_atapi_cmd on state change? well... mostly that any ATAPI command got interrupted before it finished, which is generally not possible with PIO or synchronous commands because the AIO flush on savevm or migrate should clear those requests out. I *think* the only time we run into this problem is with e.g. PCI HBAs where the DMA controller is programmed before we kick the HBA with the start signal... which I *think* means that we have no chance of actually editing the io_buffer before we attempt to "resume" this command -- because if the command *starts* at all, it should *finish* and the only time we run into this migration case is if we didn't actually start the command. Did you audit this at all? Do I sound crazy or correct? (I really should document this or clean up our restore/resume code, but that's not up to you. Just a passing thought...) Thanks, --js ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] ide: restart atapi dma by re-evaluating command packet 2016-04-01 21:01 ` John Snow @ 2016-04-04 10:32 ` Pavel Butsykin 0 siblings, 0 replies; 17+ messages in thread From: Pavel Butsykin @ 2016-04-04 10:32 UTC (permalink / raw) To: John Snow, Denis V. Lunev, qemu-devel; +Cc: Paolo Bonzini, rkagan On 02.04.2016 00:01, John Snow wrote: > > > On 04/01/2016 10:32 AM, Denis V. Lunev wrote: >> From: Pavel Butsykin <pbutsykin@virtuozzo.com> >> >> ide_atapi_dma_restart() used to just complete the DMA with an error, >> under the assumption that there isn't enough information to restart it. >> >> However, as the contents of the ->io_buffer is preserved, it looks safe to >> just re-evaluate it and dispatch the ATAPI command again. >> >> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> >> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> --- >> hw/ide/atapi.c | 13 ++++++------- >> 1 file changed, 6 insertions(+), 7 dweletions(-) >> >> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c >> index 1fe58ab..acc52cd 100644 >> --- a/hw/ide/atapi.c >> +++ b/hw/ide/atapi.c >> @@ -488,14 +488,13 @@ static void ide_atapi_cmd_read(IDEState *s, int lba, int nb_sectors, >> void ide_atapi_dma_restart(IDEState *s) >> { >> /* >> - * I'm not sure we have enough stored to restart the command >> - * safely, so give the guest an error it should recover from. >> - * I'm assuming most guests will try to recover from something >> - * listed as a medium error on a CD; it seems to work on Linux. >> - * This would be more of a problem if we did any other type of >> - * DMA operation. >> + * At this point we can just re-evaluate the packet command and start over. >> + * The presence of ->dma_cb callback in the pre_save ensures that the packet >> + * command has been completely sent and we can safely restart command. >> */ >> - ide_atapi_cmd_error(s, MEDIUM_ERROR, ASC_NO_SEEK_COMPLETE); >> + s->unit = s->bus->retry_unit; >> + s->bus->dma->ops->restart_dma(s->bus->dma); >> + ide_atapi_cmd(s); >> } >> >> static inline uint8_t ide_atapi_set_profile(uint8_t *buf, uint8_t *index, >> > > Is it at all possible that a previous command may have edited the > s->io_buffer that ide_atapi_cmd() uses for SCSI command dispatch? > > Let me try to answer my own question. > > Here's my understanding: On state change, ide_restart_bh is invoked > unconditionally. If end_transfer_func is ide_atapi_cmd, we invoke > ide_atapi_dma_restart. > > What are the conditions for end_transfer_func being set to ide_atapi_cmd > on state change? well... mostly that any ATAPI command got interrupted > before it finished, which is generally not possible with PIO or > synchronous commands because the AIO flush on savevm or migrate should > clear those requests out. > In general, it's impossible for ATAPI commands that don't need to set the dma_cb for execution after bus mastering.. > I *think* the only time we run into this problem is with e.g. PCI HBAs > where the DMA controller is programmed before we kick the HBA with the > start signal... which I *think* means that we have no chance of actually > editing the io_buffer before we attempt to "resume" this command -- > because if the command *starts* at all, it should *finish* and the only > time we run into this migration case is if we didn't actually start the > command. > > Did you audit this at all? Do I sound crazy or correct? :) All looks correct to me! In the form in which there is now the realization of the DMA ATAPI it should work. Although it doesn't look so transparent. > (I really should document this or clean up our restore/resume code, but > that's not up to you. Just a passing thought...) > > Thanks, > --js > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v2 for 2.6 0/3] ide: fix loss of the dma/atapi state during migration
@ 2016-03-28 11:48 Denis V. Lunev
2016-03-28 11:48 ` [Qemu-devel] [PATCH 2/3] ide: restart atapi dma by re-evaluating command packet Denis V. Lunev
0 siblings, 1 reply; 17+ messages in thread
From: Denis V. Lunev @ 2016-03-28 11:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Denis V. Lunev, John Snow, rkagan, Pavel Butsykin
This patch set fixes bugs in the IDE DMA and the IDE ATAPI on operations to
save/restore the state.
>From the user point of view this results in IDE timeouts in the guest
when the user reads from the DVD like the following:
[424332.169229] ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
[424332.170423] sr 0:0:0:0: [sr0] CDB:
[424332.171234] Read(10): 28 00 00 00 02 e4 00 00 01 00
[424332.172418] ata1.00: cmd a0/01:00:00:00:08/00:00:00:00:00/a0 tag 0 dma 2048 in
res 40/00:02:00:0c:00/00:00:00:00:00/a0 Emask 0x4 (timeout)
[424332.174877] ata1.00: status: { DRDY }
[424337.212099] ata1: link is slow to respond, please be patient (ready=0)
[424342.220084] ata1: device not ready (errno=-16), forcing hardreset
[424342.222700] ata1: soft resetting link
[424342.381059] ata1.00: configured for MWDMA2
[424342.383693] ata1: EH complete
Another similar nasty effects are possible.
Changes from v1:
- added converter of IDE_DMA_* to IDE_RETRY_* (1)
- fixed handling of the IDE_RETRY_HBA at the ide_restart_bh function (3)
Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: John Snow <jsnow@redhat.com>
Pavel Butsykin (3):
ide: don't lose pending dma state
ide: restart atapi dma by re-evaluating command packet
ide: really restart pending and in-flight atapi dma
hw/ide/atapi.c | 28 ++++++++++++++--------------
hw/ide/core.c | 36 +++++++++++++-----------------------
hw/ide/internal.h | 36 ++++++++++++++++++++++++++++++++++++
hw/ide/pci.c | 4 ++++
4 files changed, 67 insertions(+), 37 deletions(-)
--
2.1.4
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 2/3] ide: restart atapi dma by re-evaluating command packet 2016-03-28 11:48 [Qemu-devel] [PATCH v2 for 2.6 0/3] ide: fix loss of the dma/atapi state during migration Denis V. Lunev @ 2016-03-28 11:48 ` Denis V. Lunev 0 siblings, 0 replies; 17+ messages in thread From: Denis V. Lunev @ 2016-03-28 11:48 UTC (permalink / raw) To: qemu-devel; +Cc: Denis V. Lunev, John Snow, rkagan, Pavel Butsykin From: Pavel Butsykin <pbutsykin@virtuozzo.com> ide_atapi_dma_restart() used to just complete the DMA with an error, under the assumption that there isn't enough information to restart it. However, as the contents of the ->io_buffer is preserved, it looks safe to just re-evaluate it and dispatch the ATAPI command again. Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> Signed-off-by: Denis V. Lunev <den@openvz.org> CC: John Snow <jsnow@redhat.com> --- hw/ide/atapi.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c index 1fe58ab..acc52cd 100644 --- a/hw/ide/atapi.c +++ b/hw/ide/atapi.c @@ -488,14 +488,13 @@ static void ide_atapi_cmd_read(IDEState *s, int lba, int nb_sectors, void ide_atapi_dma_restart(IDEState *s) { /* - * I'm not sure we have enough stored to restart the command - * safely, so give the guest an error it should recover from. - * I'm assuming most guests will try to recover from something - * listed as a medium error on a CD; it seems to work on Linux. - * This would be more of a problem if we did any other type of - * DMA operation. + * At this point we can just re-evaluate the packet command and start over. + * The presence of ->dma_cb callback in the pre_save ensures that the packet + * command has been completely sent and we can safely restart command. */ - ide_atapi_cmd_error(s, MEDIUM_ERROR, ASC_NO_SEEK_COMPLETE); + s->unit = s->bus->retry_unit; + s->bus->dma->ops->restart_dma(s->bus->dma); + ide_atapi_cmd(s); } static inline uint8_t ide_atapi_set_profile(uint8_t *buf, uint8_t *index, -- 2.1.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH for 2.6 0/3] ide: fix loss of the dma/atapi state during migration
@ 2016-03-23 10:26 Denis V. Lunev
2016-03-23 10:26 ` [Qemu-devel] [PATCH 2/3] ide: restart atapi dma by re-evaluating command packet Denis V. Lunev
0 siblings, 1 reply; 17+ messages in thread
From: Denis V. Lunev @ 2016-03-23 10:26 UTC (permalink / raw)
To: qemu-devel; +Cc: den, John Snow, Pavel Butsykin
This patch set fixes bugs in the IDE DMA and the IDE ATAPI on operations to
save/restore the state.
>From the user point of view this results in IDE timeouts in the guest
when the user reads from the DVD like the following:
[424332.169229] ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
[424332.170423] sr 0:0:0:0: [sr0] CDB:
[424332.171234] Read(10): 28 00 00 00 02 e4 00 00 01 00
[424332.172418] ata1.00: cmd a0/01:00:00:00:08/00:00:00:00:00/a0 tag 0 dma 2048 in
res 40/00:02:00:0c:00/00:00:00:00:00/a0 Emask 0x4 (timeout)
[424332.174877] ata1.00: status: { DRDY }
[424337.212099] ata1: link is slow to respond, please be patient (ready=0)
[424342.220084] ata1: device not ready (errno=-16), forcing hardreset
[424342.222700] ata1: soft resetting link
[424342.381059] ata1.00: configured for MWDMA2
[424342.383693] ata1: EH complete
Another similar nasty effects are possible.
Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: John Snow <jsnow@redhat.com>
Pavel Butsykin (3):
ide: don't loose pending dma state
ide: restart atapi dma by re-evaluating command packet
ide: really restart pending and in-flight atapi dma
hw/ide/atapi.c | 33 +++++++++++++++++++--------------
hw/ide/core.c | 32 ++++++++++++++++++--------------
hw/ide/internal.h | 25 +++++++++++++++++++++++++
hw/ide/pci.c | 3 +++
4 files changed, 65 insertions(+), 28 deletions(-)
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 2/3] ide: restart atapi dma by re-evaluating command packet 2016-03-23 10:26 [Qemu-devel] [PATCH for 2.6 0/3] ide: fix loss of the dma/atapi state during migration Denis V. Lunev @ 2016-03-23 10:26 ` Denis V. Lunev 0 siblings, 0 replies; 17+ messages in thread From: Denis V. Lunev @ 2016-03-23 10:26 UTC (permalink / raw) To: qemu-devel; +Cc: den, John Snow, Pavel Butsykin From: Pavel Butsykin <pbutsykin@virtuozzo.com> ide_atapi_dma_restart() used to just complete the DMA with an error, under the assumption that there isn't enough information to restart it. However, as the contents of the ->io_buffer is preserved, it looks safe to just re-evaluate it and dispatch the ATAPI command again. Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> Signed-off-by: Denis V. Lunev <den@openvz.org> CC: John Snow <jsnow@redhat.com> --- hw/ide/atapi.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c index 268220d..8816b38 100644 --- a/hw/ide/atapi.c +++ b/hw/ide/atapi.c @@ -495,14 +495,13 @@ static void ide_atapi_cmd_read(IDEState *s, int lba, int nb_sectors, void ide_atapi_dma_restart(IDEState *s) { /* - * I'm not sure we have enough stored to restart the command - * safely, so give the guest an error it should recover from. - * I'm assuming most guests will try to recover from something - * listed as a medium error on a CD; it seems to work on Linux. - * This would be more of a problem if we did any other type of - * DMA operation. + * At this point we can just re-evaluate the packet command and start over. + * The presence of ->dma_cb callback in the pre_save ensures that the packet + * command has been completely sent and we can safely restart command. */ - ide_atapi_cmd_error(s, MEDIUM_ERROR, ASC_NO_SEEK_COMPLETE); + s->unit = s->bus->retry_unit; + s->bus->dma->ops->restart_dma(s->bus->dma); + ide_atapi_cmd(s); } static inline uint8_t ide_atapi_set_profile(uint8_t *buf, uint8_t *index, -- 2.1.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-04-12 16:47 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-04-06 6:40 [Qemu-devel] [PATCH for 2.6 v4 0/3] ide: fix loss of the dma/atapi state during migration Denis V. Lunev 2016-04-06 6:40 ` [Qemu-devel] [PATCH 1/3] ide: don't lose pending dma state Denis V. Lunev 2016-04-06 6:40 ` [Qemu-devel] [PATCH 2/3] ide: restart atapi dma by re-evaluating command packet Denis V. Lunev 2016-04-06 6:40 ` [Qemu-devel] [PATCH 3/3] ide: really restart pending and in-flight atapi dma Denis V. Lunev 2016-04-11 22:18 ` Eric Blake 2016-04-12 12:17 ` Pavel Butsykin 2016-04-12 16:47 ` John Snow 2016-04-12 12:53 ` [Qemu-devel] [PATCH] ide: coding style fix Pavel Butsykin 2016-04-11 14:12 ` [Qemu-devel] [PATCH for 2.6 v4 0/3] ide: fix loss of the dma/atapi state during migration Denis V. Lunev 2016-04-11 21:47 ` John Snow 2016-04-12 5:33 ` Denis V. Lunev 2016-04-12 16:24 ` John Snow -- strict thread matches above, loose matches on Subject: below -- 2016-04-01 14:32 [Qemu-devel] [PATCH v3 " Denis V. Lunev 2016-04-01 14:32 ` [Qemu-devel] [PATCH 2/3] ide: restart atapi dma by re-evaluating command packet Denis V. Lunev 2016-04-01 21:01 ` John Snow 2016-04-04 10:32 ` Pavel Butsykin 2016-03-28 11:48 [Qemu-devel] [PATCH v2 for 2.6 0/3] ide: fix loss of the dma/atapi state during migration Denis V. Lunev 2016-03-28 11:48 ` [Qemu-devel] [PATCH 2/3] ide: restart atapi dma by re-evaluating command packet Denis V. Lunev 2016-03-23 10:26 [Qemu-devel] [PATCH for 2.6 0/3] ide: fix loss of the dma/atapi state during migration Denis V. Lunev 2016-03-23 10:26 ` [Qemu-devel] [PATCH 2/3] ide: restart atapi dma by re-evaluating command packet Denis V. Lunev
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.