From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=36010 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PQinK-0004Ry-Ro for qemu-devel@nongnu.org; Thu, 09 Dec 2010 10:52:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PQinJ-0000Sb-JA for qemu-devel@nongnu.org; Thu, 09 Dec 2010 10:52:34 -0500 Received: from mx1.redhat.com ([209.132.183.28]:30587) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PQinJ-0000ST-AQ for qemu-devel@nongnu.org; Thu, 09 Dec 2010 10:52:33 -0500 Message-ID: <4D00FB6E.7030807@redhat.com> Date: Thu, 09 Dec 2010 16:53:18 +0100 From: Kevin Wolf MIME-Version: 1.0 References: <1291810400-11309-1-git-send-email-agraf@suse.de> <1291810400-11309-10-git-send-email-agraf@suse.de> <4D00FA44.8090207@suse.de> In-Reply-To: <4D00FA44.8090207@suse.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 09/13] ahci: add ahci emulation List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: Stefan Hajnoczi , Paul Brook , QEMU-devel Developers , Blue Swirl , Gerd Hoffmann , Joerg Roedel , tj@kernel.org, Roland Elek , Sebastian Herbszt 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