From d1fc684f3692d6bbe85d5ee35c2568f0bc68fa1c Mon Sep 17 00:00:00 2001 From: Anthony PERARD Date: Tue, 12 Apr 2016 16:47:52 -0400 Subject: xen: Fix IDE unplug After commit e5e7855 (blockdev: Separate BB name management), starting a guest with PVHVM support result in this assert: qemu-system-i386: block/block-backend.c:173: blk_delete: Assertion `!blk->name' failed. A backtrace show that a caller is pci_piix3_xen_ide_unplug(). This patch fix it. Signed-off-by: Anthony PERARD Message-id: 1460382666-29885-1-git-send-email-anthony.perard@citrix.com Signed-off-by: John Snow --- hw/ide/piix.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/ide/piix.c b/hw/ide/piix.c index 0a4cbcbcbb..6d76ce980b 100644 --- a/hw/ide/piix.c +++ b/hw/ide/piix.c @@ -189,6 +189,7 @@ int pci_piix3_xen_ide_unplug(DeviceState *dev) idedev = pci_ide->bus[di->bus].slave; } idedev->conf.blk = NULL; + monitor_remove_blk(blk); blk_unref(blk); } } -- cgit v1.2.1 From 218fd37c68e450dffe37832e82a1f0a6592e7169 Mon Sep 17 00:00:00 2001 From: Pavel Butsykin Date: Tue, 12 Apr 2016 16:47:52 -0400 Subject: ide: don't lose pending dma state 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 Reviewed-by: Roman Kagan Signed-off-by: Denis V. Lunev Reviewed-by: John Snow Message-id: 1459924806-306-2-git-send-email-den@openvz.org Signed-off-by: John Snow --- 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 90524d5e16..58d06876dc 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 86bde26551..68c7d0d87e 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 92ffee7264..8d56a00b1b 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; -- cgit v1.2.1 From 9a41826f3843bf11bf8f52f176071f8c1fb0aa7e Mon Sep 17 00:00:00 2001 From: Pavel Butsykin Date: Tue, 12 Apr 2016 16:47:52 -0400 Subject: ide: restart atapi dma by re-evaluating command packet 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 Reviewed-by: Roman Kagan Signed-off-by: Denis V. Lunev Reviewed-by: John Snow Message-id: 1459924806-306-3-git-send-email-den@openvz.org Signed-off-by: John Snow --- 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 1fe58ab7fd..acc52cd11f 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, -- cgit v1.2.1 From 502356eeeb5fd2bdd92b2d5156e511626c1c3814 Mon Sep 17 00:00:00 2001 From: Pavel Butsykin Date: Tue, 12 Apr 2016 18:48:15 -0400 Subject: ide: really restart pending and in-flight atapi dma 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. [Including a fixup patch: Message-id: 1460465594-15777-1-git-send-email-pbutsykin@virtuozzo.com --js] Signed-off-by: Pavel Butsykin Signed-off-by: Denis V. Lunev Reviewed-by: Roman Kagan Reviewed-by: John Snow Message-id: 1459924806-306-4-git-send-email-den@openvz.org Signed-off-by: John Snow --- hw/ide/atapi.c | 13 ++++++------- hw/ide/core.c | 30 +++++++++++++++--------------- hw/ide/internal.h | 20 ++++++++++++++++++++ hw/ide/macio.c | 2 ++ 4 files changed, 43 insertions(+), 22 deletions(-) diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c index acc52cd11f..2bb606c1c5 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 58d06876dc..41e6a2dc45 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 68c7d0d87e..d2c458f579 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) \ @@ -506,13 +507,28 @@ 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 */ #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_MASK) == IDE_RETRY_ATAPI) + static inline uint8_t ide_dma_cmd_to_retry(uint8_t dma_cmd) { switch (dma_cmd) { @@ -522,6 +538,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 +630,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 1725e5b23f..76256eb8a8 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; -- cgit v1.2.1