[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: |
Alexander Graf |
Subject: |
[Qemu-devel] Re: [PATCH 09/13] ahci: add ahci emulation |
Date: |
Thu, 09 Dec 2010 17:18:22 +0100 |
User-agent: |
Thunderbird 2.0.0.23 (X11/20090817) |
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
- [Qemu-devel] Re: [PATCH 03/13] ide: Split out BMDMA code from ATA core, (continued)
[Qemu-devel] Re: [PATCH 03/13] ide: Split out BMDMA code from ATA core, Kevin Wolf, 2010/12/09
[Qemu-devel] [PATCH 09/13] ahci: add ahci emulation, Alexander Graf, 2010/12/08