qemu-devel
[Top][All Lists]
Advanced

[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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]