From patchwork Tue May 31 10:09:55 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Wolf X-Patchwork-Id: 97999 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [140.186.70.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 33CD2B6F6C for ; Tue, 31 May 2011 20:07:25 +1000 (EST) Received: from localhost ([::1]:42753 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QRLr7-0000uu-Mr for incoming@patchwork.ozlabs.org; Tue, 31 May 2011 06:07:21 -0400 Received: from eggs.gnu.org ([140.186.70.92]:52156) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QRLr0-0000un-44 for qemu-devel@nongnu.org; Tue, 31 May 2011 06:07:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QRLqt-00006t-Cx for qemu-devel@nongnu.org; Tue, 31 May 2011 06:07:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41089) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QRLqt-00006h-5g for qemu-devel@nongnu.org; Tue, 31 May 2011 06:07:07 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p4VA7638004401 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 31 May 2011 06:07:06 -0400 Received: from dhcp-5-188.str.redhat.com (dhcp-5-175.str.redhat.com [10.32.5.175]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p4VA74ZH003575; Tue, 31 May 2011 06:07:05 -0400 From: Kevin Wolf To: qemu-devel@nongnu.org Date: Tue, 31 May 2011 12:09:55 +0200 Message-Id: <1306836595-8481-1-git-send-email-kwolf@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.132.183.28 Cc: kwolf@redhat.com, armbru@redhat.com, quintela@redhat.com Subject: [Qemu-devel] [RFC][PATCH] ide: Break migration by splitting error status from status register X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org When adding the werror=stop mode, some flags were added to s->status which are used to determine what kind of operation should be restarted when the VM is continued. Unfortunately, it turns out that s->status is in fact a device register and as such is visible to the guest (some of the abused bits are even writable for the guest). Splitting the internal status and the status register into two different variables is easy enough, but this will break migration: We must have a way to detect what s->status really means. Is it only the status register (as used by new versions) or do we have to extract internal error status flags? Here we seem to be lacking some kind of optional subsection that would be simply ignored by older versions, but can contain information for new versions. Is there any precedence on how to solve this? --- hw/ide/core.c | 22 +++++++++++++++++++++- hw/ide/internal.h | 5 +++++ hw/ide/pci.c | 19 +++++++++++++------ 3 files changed, 39 insertions(+), 7 deletions(-) diff --git a/hw/ide/core.c b/hw/ide/core.c index 95beb17..dc9b94b 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -446,7 +446,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op) if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC) || action == BLOCK_ERR_STOP_ANY) { s->bus->dma->ops->set_unit(s->bus->dma, s->unit); - s->bus->dma->ops->add_status(s->bus->dma, op); + s->error_status = op; bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read); vm_stop(VMSTOP_DISKFULL); } else { @@ -1847,6 +1847,13 @@ static bool ide_atapi_gesn_needed(void *opaque) return s->events.new_media || s->events.eject_request; } +static bool ide_error_needed(void *opaque) +{ + IDEState *s = opaque; + + return (s->error_status != 0); +} + /* Fields for GET_EVENT_STATUS_NOTIFICATION ATAPI command */ const VMStateDescription vmstate_ide_atapi_gesn_state = { .name ="ide_drive/atapi/gesn_state", @@ -1879,6 +1886,16 @@ const VMStateDescription vmstate_ide_drive_pio_state = { } }; +const VMStateDescription vmstate_ide_error_status = { + .name ="ide_drive/error", + .version_id = 1, + .minimum_version_id = 1, + .minimum_version_id_old = 1, + .fields = (VMStateField []) { + VMSTATE_INT32(error_status, IDEState), + } +}; + const VMStateDescription vmstate_ide_drive = { .name = "ide_drive", .version_id = 3, @@ -1916,6 +1933,9 @@ const VMStateDescription vmstate_ide_drive = { .vmsd = &vmstate_ide_atapi_gesn_state, .needed = ide_atapi_gesn_needed, }, { + .vmsd = &vmstate_ide_error_status, + .needed = ide_error_needed, + }, { /* empty */ } } diff --git a/hw/ide/internal.h b/hw/ide/internal.h index c2b35ec..9ba4b34 100644 --- a/hw/ide/internal.h +++ b/hw/ide/internal.h @@ -440,6 +440,9 @@ struct IDEState { uint8_t end_transfer_fn_idx; QEMUTimer *sector_write_timer; /* only used for win2k install hack */ uint32_t irq_count; /* counts IRQs when using win2k install hack */ + + int error_status; + /* CF-ATA extended error */ uint8_t ext_error; /* CF-ATA metadata storage */ @@ -505,6 +508,8 @@ struct IDEDeviceInfo { #define BM_STATUS_DMAING 0x01 #define BM_STATUS_ERROR 0x02 #define BM_STATUS_INT 0x04 + +/* FIXME These are not status register bits */ #define BM_STATUS_DMA_RETRY 0x08 #define BM_STATUS_PIO_RETRY 0x10 #define BM_STATUS_RETRY_READ 0x20 diff --git a/hw/ide/pci.c b/hw/ide/pci.c index a4726ad..531ad97 100644 --- a/hw/ide/pci.c +++ b/hw/ide/pci.c @@ -183,27 +183,34 @@ static void bmdma_restart_dma(BMDMAState *bm, int is_read) bmdma_start_dma(&bm->dma, s, bm->dma_cb); } +/* TODO This should be common IDE code */ static void bmdma_restart_bh(void *opaque) { BMDMAState *bm = opaque; + IDEState *s; int is_read; qemu_bh_delete(bm->bh); bm->bh = NULL; - is_read = !!(bm->status & BM_STATUS_RETRY_READ); + if (bm->unit == (uint8_t) -1) { + return; + } + + s = bmdma_active_if(bm); + is_read = !!(s->error_status & BM_STATUS_RETRY_READ); - if (bm->status & BM_STATUS_DMA_RETRY) { - bm->status &= ~(BM_STATUS_DMA_RETRY | BM_STATUS_RETRY_READ); + if (s->error_status & BM_STATUS_DMA_RETRY) { + s->error_status &= ~(BM_STATUS_DMA_RETRY | BM_STATUS_RETRY_READ); bmdma_restart_dma(bm, is_read); - } else if (bm->status & BM_STATUS_PIO_RETRY) { - bm->status &= ~(BM_STATUS_PIO_RETRY | BM_STATUS_RETRY_READ); + } else if (s->error_status & BM_STATUS_PIO_RETRY) { + s->error_status &= ~(BM_STATUS_PIO_RETRY | BM_STATUS_RETRY_READ); if (is_read) { ide_sector_read(bmdma_active_if(bm)); } else { ide_sector_write(bmdma_active_if(bm)); } - } else if (bm->status & BM_STATUS_RETRY_FLUSH) { + } else if (s->error_status & BM_STATUS_RETRY_FLUSH) { ide_flush_cache(bmdma_active_if(bm)); } }