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

[Kevin Wolf]

Am 09.12.2010 17:18, schrieb Alexander Graf:
> 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?

If you need it for debugging, maybe have a DPRINTF_ERROR macro that
aborts with an error message if DEBUG is defined, and doesn't do
anything in normal builds? I'm still not sure if aborting is a good idea
there, but I think it would be acceptable.

For normal builds, the preferred behaviour is doing the same as real
hardware in such cases. And real hardware doesn't kill the machine when
the driver is doing stupid things, but rather sets an error bit in some
register or something.

Kevin