Kevin Wolf wrote: > 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. >
Not sure about that. Would you prefer a broken guest to abort so you can debug it or to have it spew your log files with error messages or to silently ignore errors and never find bugs? Alex