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

[John Snow <jsnow@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 <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];
>>   };
>>
>

-- 
—js