From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59274) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alKAB-00036R-88 for qemu-devel@nongnu.org; Wed, 30 Mar 2016 13:44:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1alKA7-0005rD-3Z for qemu-devel@nongnu.org; Wed, 30 Mar 2016 13:44:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37349) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alKA6-0005r2-SR for qemu-devel@nongnu.org; Wed, 30 Mar 2016 13:44:11 -0400 References: <1458728792-15779-1-git-send-email-den@openvz.org> <1458728792-15779-2-git-send-email-den@openvz.org> <56FC0E61.1080709@redhat.com> <56FC0F0A.5080805@openvz.org> From: John Snow Message-ID: <56FC1069.3010509@redhat.com> Date: Wed, 30 Mar 2016 13:44:09 -0400 MIME-Version: 1.0 In-Reply-To: <56FC0F0A.5080805@openvz.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/3] ide: don't loose pending dma state List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" , qemu-devel@nongnu.org Cc: Pavel Butsykin On 03/30/2016 01:38 PM, Denis V. Lunev wrote: > On 03/30/2016 08:35 PM, John Snow wrote: >> >> On 03/23/2016 06:26 AM, Denis V. Lunev wrote: >>> From: Pavel Butsykin >>> >>> 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 calling the DMA callback with an >>> artificial error code in pre_save if the callback is already set but >>> DMAING is clear. The callback then sets bus->error_status to indicate >>> the need for restart; however since DMAING is clear the state upon >>> restore will be exactly "ready forDMA" as before the save. >>> >>> This patch only fixes the IDE DMA case; the ATAPI DMA is left with a >>> stub >>> for now since its restart hadn't been implemented yet. It will be >>> addressed in the followup patch. >>> >>> Signed-off-by: Pavel Butsykin >>> Reviewed-by: Roman Kagan >>> Signed-off-by: Denis V. Lunev >>> CC: John Snow >>> --- >>> hw/ide/atapi.c | 9 ++++++++- >>> hw/ide/core.c | 9 ++++++++- >>> hw/ide/internal.h | 7 +++++++ >>> hw/ide/pci.c | 3 +++ >>> 4 files changed, 26 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c >>> index 1fe58ab..268220d 100644 >>> --- a/hw/ide/atapi.c >>> +++ b/hw/ide/atapi.c >>> @@ -381,7 +381,14 @@ static void ide_atapi_cmd_read_dma_cb(void >>> *opaque, int ret) >>> IDEState *s = opaque; >>> int data_offset, n; >>> - if (ret < 0) { >>> + if (ret < 0) { /* XXX: handle -ESTATESAVE for migration */ >>> + if (ret == -ESTATESAVE) { >>> + /* >>> + * This case is not really an error >>> + * but a request to save the state. >>> + */ >>> + return; >>> + } >>> ide_atapi_io_error(s, ret); >>> goto eot; >>> } >>> diff --git a/hw/ide/core.c b/hw/ide/core.c >>> index 241e840..872e11f 100644 >>> --- a/hw/ide/core.c >>> +++ b/hw/ide/core.c >>> @@ -810,7 +810,14 @@ static void ide_dma_cb(void *opaque, int ret) >>> else if (s->dma_cmd == IDE_DMA_TRIM) >>> op |= IDE_RETRY_TRIM; >>> - if (ide_handle_rw_error(s, -ret, op)) { >>> + if (ret == -ESTATESAVE) { >>> + /* >>> + * This case is not really an error >>> + * but a request to save the state. >>> + */ >>> + s->bus->error_status = op; >>> + return; >>> + } else if (ide_handle_rw_error(s, -ret, op)) { >>> return; >>> } >>> } >>> diff --git a/hw/ide/internal.h b/hw/ide/internal.h >>> index 86bde26..dcd8627 100644 >>> --- a/hw/ide/internal.h >>> +++ b/hw/ide/internal.h >>> @@ -513,6 +513,13 @@ struct IDEDevice { >>> #define IDE_RETRY_TRIM 0x80 >>> #define IDE_RETRY_HBA 0x100 >>> +/* >>> + * a code to trigger entering error path and save/restore the "ready >>> to DMA" >>> + * state just like DMA-ing state. (Ab)use EINPROGRESS as it's not >>> supposed to >>> + * come from the block layer >>> + */ >>> +#define ESTATESAVE EINPROGRESS >>> + >>> 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..e1f89fa 100644 >>> --- a/hw/ide/pci.c >>> +++ b/hw/ide/pci.c >>> @@ -308,6 +308,9 @@ 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->dma_cb(bmdma_active_if(bm), -ESTATESAVE); >>> + } >>> 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; >>> >> Reviewed-by: John Snow > there is never patch sent [PATCH 1/3] ide: don't lose pending dma state > 28.03 > as v2. > > Den My mistake. I tested the new one but replied to the wrong mail.