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