[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH 09/13] ahci: add ahci emulation
From: |
Kevin Wolf |
Subject: |
[Qemu-devel] Re: [PATCH 09/13] ahci: add ahci emulation |
Date: |
Thu, 09 Dec 2010 16:53:18 +0100 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.15) Gecko/20101027 Fedora/3.0.10-1.fc12 Thunderbird/3.0.10 |
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
[Qemu-devel] [PATCH 09/13] ahci: add ahci emulation, Alexander Graf, 2010/12/08