From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42355) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTYu9-0008E5-8E for qemu-devel@nongnu.org; Mon, 15 Sep 2014 12:13:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XTYtz-0005h2-9w for qemu-devel@nongnu.org; Mon, 15 Sep 2014 12:13:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43441) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTYtz-0005gr-2I for qemu-devel@nongnu.org; Mon, 15 Sep 2014 12:13:19 -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 s8FGDHr0015069 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Mon, 15 Sep 2014 12:13:18 -0400 Message-ID: <5417101A.9080001@redhat.com> Date: Mon, 15 Sep 2014 12:13:14 -0400 From: John Snow MIME-Version: 1.0 References: <1410582855-21870-1-git-send-email-jsnow@redhat.com> <1410582855-21870-4-git-send-email-jsnow@redhat.com> <541445F8.7080909@redhat.com> In-Reply-To: <541445F8.7080909@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC 03/10] AHCI: Add PRD interrupt List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , qemu-devel@nongnu.org Cc: stefanha@redhat.com, mst@redhat.com On 09/13/2014 09:26 AM, Paolo Bonzini wrote: > Il 13/09/2014 06:34, John Snow ha scritto: >> AHCI devices support a feature where individual entries in the >> scatter-gather list may have interrupt request bits set, in order >> to receive notification partway through a command that a portion >> of a transfer has completed. AHCI specs refer to this as the >> DPS bit or Descriptor Processed Status. It is named the >> "Interrupt on Completion" bit inside the PRDT. >> >> This is not currently feasible in QEMU without reworking the inner >> DMA loop extensively, but we can at least record if we saw such >> a bit in advance and sent the interrupt at the end of the transfer, >> in case a guest is expecting to see the PRD/DPS interrupt bit set. >> >> Signed-off-by: John Snow >> --- >> hw/ide/ahci.c | 11 +++++++++++ >> hw/ide/ahci.h | 5 ++++- >> 2 files changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c >> index d3ece78..8e6a352 100644 >> --- a/hw/ide/ahci.c >> +++ b/hw/ide/ahci.c >> @@ -501,6 +501,7 @@ static void ahci_reset_port(AHCIState *s, int port= ) >> pr->scr_err =3D 0; >> pr->scr_act =3D 0; >> d->busy_slot =3D -1; >> + d->dp_intr_req =3D false; >> d->init_d2h_sent =3D false; >> >> ide_state =3D &s->dev[port].port.ifs[0]; >> @@ -775,11 +776,15 @@ static int ahci_populate_sglist(AHCIDevice *ad, = QEMUSGList *sglist, int offset) >> ad->hba->as); >> qemu_sglist_add(sglist, le64_to_cpu(tbl[off_idx].addr + off_= pos), >> prdt_tbl_entry_size(&tbl[off_idx]) - off_pos= ); >> + ad->dp_intr_req =3D le32_to_cpu(tbl[off_idx].flags_size) & AH= CI_PRDT_INTR; > > Why is this also not an "OR"? It feels safer that way. It's being added after sglist initialization; I didn't think there was=20 any reason to preserve the previous value if there was one. I suspect=20 that if the internal interrupt request bit is still one here, it's=20 because I goofed up elsewhere. > >> for (i =3D off_idx + 1; i < sglist_alloc_hint; i++) { >> /* flags_size is zero-based */ >> qemu_sglist_add(sglist, le64_to_cpu(tbl[i].addr), >> prdt_tbl_entry_size(&tbl[i])); >> + if (le32_to_cpu(tbl[i].flags_size) & AHCI_PRDT_INTR) { >> + ad->dp_intr_req =3D 1; >> + } >> } >> } >> >> @@ -1151,6 +1156,11 @@ static int ahci_commit_buf(IDEDMA *dma, int xfe= r_ok) >> >> qemu_sglist_destroy(&s->sg); >> >> + if (ad->dp_intr_req) { >> + ahci_trigger_irq(ad->hba, ad, PORT_IRQ_SG_DONE); >> + ad->dp_intr_req =3D 0; >> + } > > Is it also needed in the error case? Especially the short-PRDT case > that you are adding in the next patch. Aah! Good catch... No, we should only be interrupting if we are 100%=20 sure that the segment that requested the DPS bit was transferred. > >> return s->sg.size !=3D 0; >> } >> >> @@ -1307,6 +1317,7 @@ static const VMStateDescription vmstate_ahci_dev= ice =3D { >> VMSTATE_UINT32(port_regs.cmd_issue, AHCIDevice), >> VMSTATE_BOOL(done_atapi_packet, AHCIDevice), >> VMSTATE_INT32(busy_slot, AHCIDevice), >> + VMSTATE_BOOL(dp_intr_req, AHCIDevice), >> VMSTATE_BOOL(init_d2h_sent, AHCIDevice), >> VMSTATE_END_OF_LIST() >> }, >> diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h >> index 1543df7..31f32d3 100644 >> --- a/hw/ide/ahci.h >> +++ b/hw/ide/ahci.h >> @@ -180,7 +180,9 @@ >> >> #define AHCI_COMMAND_TABLE_ACMD 0x40 >> >> -#define AHCI_PRDT_SIZE_MASK 0x3fffff >> +#define AHCI_PRDT_SIZE_MASK 0x003fffff >> +#define AHCI_PRDT_RESERVED 0x7fc00000 >> +#define AHCI_PRDT_INTR 0x80000000 >> >> #define IDE_FEATURE_DMA 1 >> >> @@ -265,6 +267,7 @@ struct AHCIDevice { >> bool done_atapi_packet; >> int32_t busy_slot; >> bool init_d2h_sent; >> + bool dp_intr_req; >> AHCICmdHdr *cur_cmd; >> NCQTransferState ncq_tfs[AHCI_MAX_CMDS]; >> }; >> > --=20 =97js