Re: [Qemu-devel] [RFC 03/10] AHCI: Add PRD interrupt

[John Snow]

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 <address@hidden>
> > ---
> >   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];
> >   };

--
—js