| Message ID | 1410582855-21870-4-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <jsnow@redhat.com> > --- > 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 = 0; > pr->scr_act = 0; > d->busy_slot = -1; > + d->dp_intr_req = false; > d->init_d2h_sent = false; > > ide_state = &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 = le32_to_cpu(tbl[off_idx].flags_size) & AHCI_PRDT_INTR; Why is this also not an "OR"? It feels safer that way. > for (i = 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 = 1; > + } > } > } > > @@ -1151,6 +1156,11 @@ static int ahci_commit_buf(IDEDMA *dma, int xfer_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 = 0; > + } Is it also needed in the error case? Especially the short-PRDT case that you are adding in the next patch. > return s->sg.size != 0; > } > > @@ -1307,6 +1317,7 @@ static const VMStateDescription vmstate_ahci_device = { > 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]; > }; >
Il 13/09/2014 15:26, Paolo Bonzini ha scritto: >> > >> > + if (ad->dp_intr_req) { >> > + ahci_trigger_irq(ad->hba, ad, PORT_IRQ_SG_DONE); >> > + ad->dp_intr_req = 0; >> > + } > Is it also needed in the error case? Especially the short-PRDT case > that you are adding in the next patch. > ... and is it also needed for NCQ, albeit with a separate flag? Paolo
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 <jsnow@redhat.com> >> --- >> 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 = 0; >> pr->scr_act = 0; >> d->busy_slot = -1; >> + d->dp_intr_req = false; >> d->init_d2h_sent = false; >> >> ide_state = &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 = le32_to_cpu(tbl[off_idx].flags_size) & AHCI_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 any reason to preserve the previous value if there was one. I suspect that if the internal interrupt request bit is still one here, it's because I goofed up elsewhere. > >> for (i = 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 = 1; >> + } >> } >> } >> >> @@ -1151,6 +1156,11 @@ static int ahci_commit_buf(IDEDMA *dma, int xfer_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 = 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% sure that the segment that requested the DPS bit was transferred. > >> return s->sg.size != 0; >> } >> >> @@ -1307,6 +1317,7 @@ static const VMStateDescription vmstate_ahci_device = { >> 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]; >> }; >> >
On 09/13/2014 03:50 PM, Paolo Bonzini wrote: > Il 13/09/2014 15:26, Paolo Bonzini ha scritto: >>>> >>>> + if (ad->dp_intr_req) { >>>> + ahci_trigger_irq(ad->hba, ad, PORT_IRQ_SG_DONE); >>>> + ad->dp_intr_req = 0; >>>> + } >> Is it also needed in the error case? Especially the short-PRDT case >> that you are adding in the next patch. >> > > ... and is it also needed for NCQ, albeit with a separate flag? > > Paolo > I had actually thought that it wasn't; due to the more asynchronous nature of NCQ. But... looking back at the specification, I can't find anywhere that explicitly says you can't use these bits for NCQ.
Il 15/09/2014 18:31, John Snow ha scritto: > > > On 09/13/2014 03:50 PM, Paolo Bonzini wrote: >> Il 13/09/2014 15:26, Paolo Bonzini ha scritto: >>>>> >>>>> + if (ad->dp_intr_req) { >>>>> + ahci_trigger_irq(ad->hba, ad, PORT_IRQ_SG_DONE); >>>>> + ad->dp_intr_req = 0; >>>>> + } >>> Is it also needed in the error case? Especially the short-PRDT case >>> that you are adding in the next patch. >>> >> >> ... and is it also needed for NCQ, albeit with a separate flag? >> >> Paolo >> > > I had actually thought that it wasn't; due to the more asynchronous > nature of NCQ. But... looking back at the specification, I can't find > anywhere that explicitly says you can't use these bits for NCQ. I suggest that you leave out this bit for a moment, so that we can get Windows fixed. Paolo
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 = 0; pr->scr_act = 0; d->busy_slot = -1; + d->dp_intr_req = false; d->init_d2h_sent = false; ide_state = &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 = le32_to_cpu(tbl[off_idx].flags_size) & AHCI_PRDT_INTR; for (i = 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 = 1; + } } } @@ -1151,6 +1156,11 @@ static int ahci_commit_buf(IDEDMA *dma, int xfer_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 = 0; + } + return s->sg.size != 0; } @@ -1307,6 +1317,7 @@ static const VMStateDescription vmstate_ahci_device = { 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]; };
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 <jsnow@redhat.com> --- hw/ide/ahci.c | 11 +++++++++++ hw/ide/ahci.h | 5 ++++- 2 files changed, 15 insertions(+), 1 deletion(-)