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

[Stefan Hajnoczi]

On Wed, Dec 8, 2010 at 12:13 PM, Alexander Graf <address@hidden> wrote:
> +struct AHCIDevice {
> +    IDEBus port;
> +    int port_no;
> +    uint32_t port_state;
> +    uint32_t finished;
> +    AHCIPortRegs port_regs;
> +    struct AHCIState *hba;
> +    uint8_t *lst;
> +    uint8_t *res_fis;
> +    uint8_t *cmd_fis;

Are these unmapped on reset?

> +    int cmd_fis_len;
> +    int dma_status;
> +    BlockDriverCompletionFunc *dma_cb;
> +    AHCICmdHdr *cur_cmd;
> +    NCQTransferState ncq_tfs[AHCI_MAX_CMDS];

Are the ncq_tfs[] elements cleaned up on reset (i.e. cancellation and
free sglist)?

> +static void map_page(uint8_t **ptr, uint64_t addr, uint32_t wanted)
> +{
> +    target_phys_addr_t len = wanted;
> +
> +    if (*ptr) {
> +        cpu_physical_memory_unmap(*ptr, 1, len, len);
> +    }
> +
> +    *ptr = cpu_physical_memory_map(addr, &len, 1);
> +    if (len < wanted) {
> +        cpu_physical_memory_unmap(*ptr, 1, len, len);

*ptr = NULL;

> +static void ncq_cb(void *opaque, int ret)
> +{
> +    NCQTransferState *ncq_tfs = (NCQTransferState *)opaque;
> +    IDEState *ide_state;
> +
> +    if (ret < 0) {
> +        /* XXX error */
> +    }

Missing error handling.

> +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.

> +            break;
> +    }
> +}
> +
> +static int handle_cmd(AHCIState *s, int port, int slot)
> +{
> +    IDEState *ide_state;
> +
> +    int sglist_alloc_hint;
> +    QEMUSGList sglist;
> +    int atapi_packet_len = 0;
> +    AHCIPortRegs *pr;
> +    uint32_t opts;
> +    uint64_t tbl_addr;
> +    AHCICmdHdr *cmd;
> +    uint8_t *cmd_fis;
> +
> +    target_phys_addr_t cmd_len;
> +    int i;
> +
> +    if (s->dev[port].port.ifs[0].status & (BUSY_STAT|DRQ_STAT)) {
> +        /* Engine currently busy, try again later */
> +        DPRINTF(port, "engine busy\n");
> +        return -1;
> +    }
> +
> +    pr = &s->dev[port].port_regs;
> +    cmd = &((AHCICmdHdr *)s->dev[port].lst)[slot];
> +
> +    if (!s->dev[port].lst) {
> +        hw_error("%s: lst not given but cmd handled", __FUNCTION__);

Guest triggerable abort.

> +    }
> +
> +    opts = le32_to_cpu(cmd->opts);
> +    tbl_addr = le64_to_cpu(cmd->tbl_addr);
> +
> +    sglist_alloc_hint = opts >> AHCI_CMD_HDR_PRDT_LEN;
> +    cmd_len = 0x80 + (sglist_alloc_hint * sizeof(AHCI_SG));
> +    cmd_fis = cpu_physical_memory_map(tbl_addr, &cmd_len, 1);

NULL dereference later if cpu_physical_memory_map() fails due to
invalid address (tbl_addr).

> +
> +    /* The device we are working for */
> +    ide_state = &s->dev[port].port.ifs[0];
> +
> +    if (!ide_state->bs) {
> +        hw_error("%s: guest accessed unused port", __FUNCTION__);

Guest triggerable abort.

> +    }
> +
> +    /* Get number of entries in the PRDT, init a qemu sglist accordingly */
> +    memset(&sglist, 0, sizeof(sglist));
> +
> +    if (sglist_alloc_hint > 0) {
> +        AHCI_SG *tbl = (AHCI_SG *)(&cmd_fis[0x80]);
> +
> +        qemu_sglist_init(&sglist, sglist_alloc_hint);
> +        /* Parse the PRDs and create qemu sglist entries accordingly */
> +        for (i = 0; i < sglist_alloc_hint; i++) {
> +            /* flags_size is zero-based */
> +            qemu_sglist_add(&sglist, le64_to_cpu(tbl[i].addr),
> +                            le32_to_cpu(tbl[i].flags_size) + 1);
> +        }
> +    }

Only the SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER codepath seems to
clean up sglist.  The guest can leak host memory by setting
sglist_alloc_hint > 0 and not using
SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER.

> +
> +    debug_print_fis(cmd_fis, (opts & AHCI_CMD_HDR_CMD_FIS_LEN) * 4);
> +
> +    switch (cmd_fis[0]) {
> +        case SATA_FIS_TYPE_REGISTER_H2D:
> +            break;
> +        default:
> +            hw_error("unknown command cmd_fis[0]=%02x cmd_fis[1]=%02x 
> cmd_fis[2]=%02x\n",
> +                     cmd_fis[0], cmd_fis[1], cmd_fis[2]);

Guest triggerable abort.

> +            break;
> +    }
> +
> +    switch (cmd_fis[1]) {
> +        case SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER:
> +            break;
> +        case 0:
> +            break;
> +        default:
> +            hw_error("unknown command cmd_fis[0]=%02x cmd_fis[1]=%02x 
> cmd_fis[2]=%02x\n",
> +                     cmd_fis[0], cmd_fis[1], cmd_fis[2]);

Guest triggerable abort.

> +            break;
> +    }
> +
> +    switch (s->dev[port].port_state) {
> +        case STATE_RUN:
> +            if (cmd_fis[15] & ATA_SRST) {
> +                s->dev[port].port_state = STATE_RESET;
> +            }
> +            break;
> +        case STATE_RESET:
> +            if (!(cmd_fis[15] & ATA_SRST)) {
> +                ahci_reset_port(s, port);
> +            }
> +            break;
> +    }
> +
> +    if (cmd_fis[1] == SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER) {
> +
> +        /* Check for NCQ command */
> +        if ((cmd_fis[2] == READ_FPDMA_QUEUED) ||
> +            (cmd_fis[2] == WRITE_FPDMA_QUEUED)) {
> +            process_ncq_command(s, port, cmd_fis, slot, &sglist);
> +            goto out;
> +        }
> +
> +        /* If the command is not NCQ, the sglist is needed in the core */
> +        ide_state->sg = sglist;
> +
> +        /* Decompose the FIS  */
> +        ide_state->nsector = (int64_t)((cmd_fis[13] << 8) | cmd_fis[12]);
> +        ide_state->feature = cmd_fis[3];
> +        if (!ide_state->nsector) {
> +            ide_state->nsector = 256;
> +        }
> +
> +        if (ide_state->drive_kind != IDE_CD) {
> +            ide_set_sector(ide_state, (cmd_fis[6] << 16) | (cmd_fis[5] << 8) 
> |
> +                           cmd_fis[4]);
> +        }
> +
> +        /* Copy the ACMD field (ATAPI packet, if any) from the AHCI command
> +         * table to ide_state->io_buffer
> +         */
> +        if (opts & AHCI_CMD_ATAPI) {
> +            atapi_packet_len = ((ide_state->hcyl) << 8) + ide_state->lcyl;

Unused variable.

> +static int pci_ahci_init(PCIDevice *dev)
> +{
> +    struct AHCIPCIState *d;
> +    d = DO_UPCAST(struct AHCIPCIState, card, dev);
> +
> +    pci_config_set_vendor_id(d->card.config, PCI_VENDOR_ID_INTEL);
> +    pci_config_set_device_id(d->card.config,
> +                             PCI_DEVICE_ID_INTEL_ICH7_AHCI);
> +
> +    pci_config_set_class(d->card.config, PCI_CLASS_STORAGE_SATA);
> +    pci_config_set_prog_interface(d->card.config, AHCI_PROGMODE_MAJOR_REV_1);
> +
> +    d->card.config[PCI_CACHE_LINE_SIZE] = 0x08;  /* Cache line size */
> +    d->card.config[PCI_LATENCY_TIMER]   = 0x00;  /* Latency timer */
> +    pci_config_set_interrupt_pin(d->card.config, 1);
> +
> +    qemu_register_reset(ahci_reset, d);

Missing qemu_unregister_reset() in pci_ahci_uninit().

Stefan