From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=40892 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PQzNW-0003km-KR for qemu-devel@nongnu.org; Fri, 10 Dec 2010 04:35:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PQzNU-00077A-QP for qemu-devel@nongnu.org; Fri, 10 Dec 2010 04:35:02 -0500 Received: from mx1.redhat.com ([209.132.183.28]:25644) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PQzNU-00076x-JU for qemu-devel@nongnu.org; Fri, 10 Dec 2010 04:35:00 -0500 Message-ID: <4D01F473.4030206@redhat.com> Date: Fri, 10 Dec 2010 10:35:47 +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> <4D00FB6E.7030807@redhat.com> <4D01014E.2070002@suse.de> In-Reply-To: <4D01014E.2070002@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 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