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

[Paolo Bonzini]

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.

>          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];
>  };
>