[Qemu-devel] Re: [PATCH 09/13] ahci: add ahci emulation

[Kevin Wolf <kwolf@redhat.com>]

Am 09.12.2010 16:48, schrieb Alexander Graf:
>>> +static void ncq_cb(void *opaque, int ret)
>>> +{
>>> +    NCQTransferState *ncq_tfs = (NCQTransferState *)opaque;
>>> +    IDEState *ide_state;
>>> +
>>> +    if (ret < 0) {
>>> +        /* XXX error */
>>> +    }
>>>     
>>
>> Missing error handling.
>>   
> 
> Yes, that's what the XXX stands for :).

I think Stefan wanted to tell us that he thinks this XXX should be
addressed. I don't disagree, by the way. ;-)

>>> +static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
>>> +                                int slot, QEMUSGList *sg)
>>> +{
>>> +    NCQFrame *ncq_fis = (NCQFrame*)cmd_fis;
>>> +    uint8_t tag = ncq_fis->tag >> 3;
>>> +    NCQTransferState *ncq_tfs = &s->dev[port].ncq_tfs[tag];
>>> +
>>> +    if (ncq_tfs->used) {
>>> +        /* error - already in use */
>>> +        fprintf(stderr, "%s: tag %d already used\n", __FUNCTION__, tag);
>>> +        return;
>>> +    }
>>> +
>>> +    ncq_tfs->used = 1;
>>> +    ncq_tfs->drive = &s->dev[port];
>>> +    ncq_tfs->drive->cmd_fis = cmd_fis;
>>> +    ncq_tfs->drive->cmd_fis_len = 0x20;
>>> +    ncq_tfs->slot = slot;
>>> +    ncq_tfs->lba = ((uint64_t)ncq_fis->lba5 << 40) |
>>> +                   ((uint64_t)ncq_fis->lba4 << 32) |
>>> +                   ((uint64_t)ncq_fis->lba3 << 24) |
>>> +                   ((uint64_t)ncq_fis->lba2 << 16) |
>>> +                   ((uint64_t)ncq_fis->lba1 << 8) |
>>> +                   (uint64_t)ncq_fis->lba0;
>>> +
>>> +    /* Note: We calculate the sector count, but don't currently rely on it.
>>> +     * The total size of the DMA buffer tells us the transfer size instead. */
>>> +    ncq_tfs->sector_count = ((uint16_t)ncq_fis->sector_count_high << 8) |
>>> +                                ncq_fis->sector_count_low;
>>> +
>>> +    DPRINTF(port, "NCQ transfer LBA from %ld to %ld, drive max %ld\n",
>>> +            ncq_tfs->lba, ncq_tfs->lba + ncq_tfs->sector_count - 2,
>>> +            s->dev[port].port.ifs[0].nb_sectors - 1);
>>> +
>>> +    ncq_tfs->sglist = *sg;
>>> +    ncq_tfs->tag = tag;
>>> +
>>> +    switch(ncq_fis->command) {
>>> +        case READ_FPDMA_QUEUED:
>>> +            DPRINTF(port, "NCQ reading %d sectors from LBA %ld, tag %d\n",
>>> +                    ncq_tfs->sector_count-1, ncq_tfs->lba, ncq_tfs->tag);
>>> +            ncq_tfs->is_read = 1;
>>> +
>>> +            /* XXX: The specification is unclear about whether the DMA Setup
>>> +             * FIS here should have the I bit set, but it suggest that it should
>>> +             * not. Linux works without this interrupt, so I disabled it.
>>> +             * If someone knows if it is needed, please tell me, or fix this. */
>>> +
>>> +            /* ahci_trigger_irq(s,s->dev[port],PORT_IRQ_STAT_DSS); */
>>> +            DPRINTF(port, "tag %d aio read %ld\n", ncq_tfs->tag, ncq_tfs->lba);
>>> +            dma_bdrv_read(ncq_tfs->drive->port.ifs[0].bs, &ncq_tfs->sglist,
>>> +                          ncq_tfs->lba, ncq_cb, ncq_tfs);
>>> +            break;
>>> +        case WRITE_FPDMA_QUEUED:
>>> +            DPRINTF(port, "NCQ writing %d sectors to LBA %ld, tag %d\n",
>>> +                    ncq_tfs->sector_count-1, ncq_tfs->lba, ncq_tfs->tag);
>>> +            ncq_tfs->is_read = 0;
>>> +            /* ahci_trigger_irq(s,s->dev[port],PORT_IRQ_STAT_DSS); */
>>> +            DPRINTF(port, "tag %d aio write %ld\n", ncq_tfs->tag, ncq_tfs->lba);
>>> +            dma_bdrv_write(ncq_tfs->drive->port.ifs[0].bs, &ncq_tfs->sglist,
>>> +                           ncq_tfs->lba, ncq_cb, ncq_tfs);
>>> +            break;
>>> +        default:
>>> +            hw_error("ahci: tried to process non-NCQ command as NCQ\n");
>>>     
>>
>> Guest triggerable abort.
>>   
> 
> Those happen. The guest can shoot itself in the foot. We have more of
> these in other places. Just check virtio.c and search for abort() :).

They are bugs which should be fixed in virtio rather than being spread
to new code.

Kevin