From patchwork Mon Dec 17 23:16:42 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Bonzini X-Patchwork-Id: 1014874 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=nongnu.org (client-ip=2001:4830:134:3::11; helo=lists.gnu.org; envelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 43Jcyn2dxrz9s4s for ; Tue, 18 Dec 2018 10:37:53 +1100 (AEDT) Received: from localhost ([::1]:50552 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gZ2SM-0007jC-T0 for incoming@patchwork.ozlabs.org; Mon, 17 Dec 2018 18:37:50 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43330) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gZ28t-0000ZG-U0 for qemu-devel@nongnu.org; Mon, 17 Dec 2018 18:17:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gZ28s-0003nb-Fv for qemu-devel@nongnu.org; Mon, 17 Dec 2018 18:17:43 -0500 Received: from mx1.redhat.com ([209.132.183.28]:44736) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1gZ28r-0003mw-VC for qemu-devel@nongnu.org; Mon, 17 Dec 2018 18:17:42 -0500 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 44CF0C050DFB; Mon, 17 Dec 2018 23:17:41 +0000 (UTC) Received: from donizetti.redhat.com (unknown [10.36.112.10]) by smtp.corp.redhat.com (Postfix) with ESMTP id 281255D9C5; Mon, 17 Dec 2018 23:17:39 +0000 (UTC) From: Paolo Bonzini To: qemu-devel@nongnu.org Date: Tue, 18 Dec 2018 00:16:42 +0100 Message-Id: <20181217231700.24482-18-pbonzini@redhat.com> In-Reply-To: <20181217231700.24482-1-pbonzini@redhat.com> References: <20181217231700.24482-1-pbonzini@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Mon, 17 Dec 2018 23:17:41 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PULL 17/35] scsi: esp: Defer command completion until previous interrupts have been handled X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Guenter Roeck Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" From: Guenter Roeck The guest OS reads RSTAT, RSEQ, and RINTR, and expects those registers to reflect a consistent state. However, it is possible that the registers can change after RSTAT was read, but before RINTR is read, when esp_command_complete() is called. Guest OS qemu -------- ---- [handle interrupt] Read RSTAT esp_command_complete() RSTAT = STAT_ST esp_dma_done() RSTAT |= STAT_TC RSEQ = 0 RINTR = INTR_BS Read RSEQ Read RINTR RINTR = 0 RSTAT &= ~STAT_TC RSEQ = SEQ_CD The guest OS would then try to handle INTR_BS combined with an old value of RSTAT. This sometimes resulted in lost events, spurious interrupts, guest OS confusion, and stalled SCSI operations. A typical guest error log (observed with various versions of Linux) looks as follows. scsi host1: Spurious irq, sreg=13. ... scsi host1: Aborting command [84531f10:2a] scsi host1: Current command [f882eea8:35] scsi host1: Queued command [84531f10:2a] scsi host1: Active command [f882eea8:35] scsi host1: Dumping command log scsi host1: ent[15] CMD val[44] sreg[90] seqreg[00] sreg2[00] ireg[20] ss[00] event[0c] scsi host1: ent[16] CMD val[01] sreg[90] seqreg[00] sreg2[00] ireg[20] ss[02] event[0c] scsi host1: ent[17] CMD val[43] sreg[90] seqreg[00] sreg2[00] ireg[20] ss[02] event[0c] scsi host1: ent[18] EVENT val[0d] sreg[92] seqreg[04] sreg2[00] ireg[18] ss[00] event[0c] ... Defer handling command completion until previous interrupts have been handled to fix the problem. Signed-off-by: Guenter Roeck --- hw/scsi/esp-pci.c | 4 ++-- hw/scsi/esp.c | 33 ++++++++++++++++++++++++++------- hw/scsi/trace-events | 1 + include/hw/scsi/esp.h | 2 ++ 4 files changed, 31 insertions(+), 9 deletions(-) diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c index d956909186..6b0bbb9b7f 100644 --- a/hw/scsi/esp-pci.c +++ b/hw/scsi/esp-pci.c @@ -313,8 +313,8 @@ static void esp_pci_hard_reset(DeviceState *dev) static const VMStateDescription vmstate_esp_pci_scsi = { .name = "pciespscsi", - .version_id = 0, - .minimum_version_id = 0, + .version_id = 1, + .minimum_version_id = 1, .fields = (VMStateField[]) { VMSTATE_PCI_DEVICE(parent_obj, PCIESPState), VMSTATE_BUFFER_UNSAFE(dma_regs, PCIESPState, 0, 8 * sizeof(uint32_t)), diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 630d923623..ca8b36c0c5 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -286,11 +286,8 @@ static void esp_do_dma(ESPState *s) esp_dma_done(s); } -void esp_command_complete(SCSIRequest *req, uint32_t status, - size_t resid) +static void esp_report_command_complete(ESPState *s, uint32_t status) { - ESPState *s = req->hba_private; - trace_esp_command_complete(); if (s->ti_size != 0) { trace_esp_command_complete_unexpected(); @@ -311,6 +308,23 @@ void esp_command_complete(SCSIRequest *req, uint32_t status, } } +void esp_command_complete(SCSIRequest *req, uint32_t status, + size_t resid) +{ + ESPState *s = req->hba_private; + + if (s->rregs[ESP_RSTAT] & STAT_INT) { + /* Defer handling command complete until the previous + * interrupt has been handled. + */ + trace_esp_command_complete_deferred(); + s->deferred_status = status; + s->deferred_complete = true; + return; + } + esp_report_command_complete(s, status); +} + void esp_transfer_data(SCSIRequest *req, uint32_t len) { ESPState *s = req->hba_private; @@ -422,7 +436,10 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr) s->rregs[ESP_RSTAT] &= ~STAT_TC; s->rregs[ESP_RSEQ] = SEQ_CD; esp_lower_irq(s); - + if (s->deferred_complete) { + esp_report_command_complete(s, s->deferred_status); + s->deferred_complete = false; + } return old_val; case ESP_TCHI: /* Return the unique id if the value has never been written */ @@ -582,6 +599,8 @@ const VMStateDescription vmstate_esp = { VMSTATE_UINT32(ti_wptr, ESPState), VMSTATE_BUFFER(ti_buf, ESPState), VMSTATE_UINT32(status, ESPState), + VMSTATE_UINT32(deferred_status, ESPState), + VMSTATE_BOOL(deferred_complete, ESPState), VMSTATE_UINT32(dma, ESPState), VMSTATE_PARTIAL_BUFFER(cmdbuf, ESPState, 16), VMSTATE_BUFFER_START_MIDDLE_V(cmdbuf, ESPState, 16, 4), @@ -671,8 +690,8 @@ static void sysbus_esp_hard_reset(DeviceState *dev) static const VMStateDescription vmstate_sysbus_esp_scsi = { .name = "sysbusespscsi", - .version_id = 0, - .minimum_version_id = 0, + .version_id = 1, + .minimum_version_id = 1, .fields = (VMStateField[]) { VMSTATE_STRUCT(esp, SysBusESPState, 0, vmstate_esp, ESPState), VMSTATE_END_OF_LIST() diff --git a/hw/scsi/trace-events b/hw/scsi/trace-events index 0fb6a99616..2fe8a7c062 100644 --- a/hw/scsi/trace-events +++ b/hw/scsi/trace-events @@ -167,6 +167,7 @@ esp_handle_satn_stop(uint32_t cmdlen) "cmdlen %d" esp_write_response(uint32_t status) "Transfer status (status=%d)" esp_do_dma(uint32_t cmdlen, uint32_t len) "command len %d + %d" esp_command_complete(void) "SCSI Command complete" +esp_command_complete_deferred(void) "SCSI Command complete deferred" esp_command_complete_unexpected(void) "SCSI command completed unexpectedly" esp_command_complete_fail(void) "Command failed" esp_transfer_data(uint32_t dma_left, int32_t ti_size) "transfer %d/%d" diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h index 682a0d2de0..adab63d1c9 100644 --- a/include/hw/scsi/esp.h +++ b/include/hw/scsi/esp.h @@ -23,6 +23,8 @@ struct ESPState { int32_t ti_size; uint32_t ti_rptr, ti_wptr; uint32_t status; + uint32_t deferred_status; + bool deferred_complete; uint32_t dma; uint8_t ti_buf[TI_BUFSZ]; SCSIBus bus;