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

[Alexander Graf <agraf@suse.de>]

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