Re: [Qemu-devel] [PATCH 7/7] esp: add AMD PCscsi emulation (PCI SCSI adapter)

[Michael S. Tsirkin]

On Sun, Jun 03, 2012 at 12:38:09PM +0200, Hervé Poussineau wrote:
> The PCI version is supported in lots of Operating Systems,
> and has been successfully tested on:
> - MS DOS 6.22 with MS Windows 3.11
> - MS Windows 98 SE
> - MS Windows NT 4.0
> 
> Signed-off-by: Hervé Poussineau <address@hidden>

I'm not sure why I'm Cc'd - for the pci_ids changes?
These look fine to me.
Some minor suggestions below but I only looked at it
briefly.
I don't have the time to do a proper review ATM, sorry.

> ---
>  default-configs/i386-softmmu.mak |    1 +
>  hw/esp.c                         |  340 
> +++++++++++++++++++++++++++++++++++++-
>  hw/pci_ids.h                     |    1 +
>  trace-events                     |    8 +
>  4 files changed, 349 insertions(+), 1 deletion(-)
> 
> diff --git a/default-configs/i386-softmmu.mak 
> b/default-configs/i386-softmmu.mak
> index 2c78175..fee8cde 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -25,3 +25,4 @@ CONFIG_HPET=y
>  CONFIG_APPLESMC=y
>  CONFIG_I8259=y
>  CONFIG_PFLASH_CFI01=y
> +CONFIG_ESP=y
> diff --git a/hw/esp.c b/hw/esp.c
> index e3425bb..f5c00fa 100644
> --- a/hw/esp.c
> +++ b/hw/esp.c
> @@ -2,6 +2,7 @@
>   * QEMU ESP/NCR53C9x emulation
>   *
>   * Copyright (c) 2005-2006 Fabrice Bellard
> + * Copyright (c) 2012 Herve Poussineau
>   *
>   * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
>   * of this software and associated documentation files (the "Software"), to 
> deal
> @@ -23,6 +24,7 @@
>   */
>  
>  #include "sysbus.h"
> +#include "pci.h"
>  #include "scsi.h"
>  #include "esp.h"
>  #include "trace.h"
> @@ -57,6 +59,7 @@ struct ESPState {
>      uint32_t cmdlen;
>      uint32_t do_cmd;
>  
> +    bool dma_autostart;
>      /* The amount of data left in the current DMA transfer.  */
>      uint32_t dma_left;
>      /* The size of the current DMA transfer.  Zero if no transfer is in
> @@ -141,6 +144,7 @@ struct ESPState {
>  #define CFG1_RESREPT 0x40
>  
>  #define TCHI_FAS100A 0x4
> +#define TCHI_AM53C974 0x12
>  
>  static void esp_raise_irq(ESPState *s)
>  {
> @@ -574,7 +578,9 @@ static void esp_reg_write(ESPState *s, uint32_t saddr, 
> uint64_t val)
>              }
>              break;
>          case CMD_TI:
> -            handle_ti(s);
> +            if (!s->dma || s->dma_autostart) {
> +                handle_ti(s);
> +            }
>              break;
>          case CMD_ICCS:
>              trace_esp_mem_writeb_cmd_iccs(val);
> @@ -669,6 +675,7 @@ static const VMStateDescription vmstate_esp = {
>          VMSTATE_UINT32(cmdlen, ESPState),
>          VMSTATE_UINT32(do_cmd, ESPState),
>          VMSTATE_UINT32(dma_left, ESPState),
> +        VMSTATE_BOOL(dma_autostart, ESPState),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> @@ -770,6 +777,7 @@ static int sysbus_esp_init(SysBusDevice *dev)
>      assert(sysbus->it_shift != -1);
>  
>      s->chip_id = TCHI_FAS100A;
> +    s->dma_autostart = true;
>      memory_region_init_io(&sysbus->iomem, &sysbus_esp_mem_ops, sysbus,
>                            "esp", ESP_REGS << sysbus->it_shift);
>      sysbus_init_mmio(dev, &sysbus->iomem);
> @@ -814,9 +822,339 @@ static TypeInfo sysbus_esp_info = {
>      .class_init    = sysbus_esp_class_init,
>  };
>  
> +#define DMA_CMD   0x0
> +#define DMA_STC   0x1
> +#define DMA_SPA   0x2
> +#define DMA_WBC   0x3
> +#define DMA_WAC   0x4
> +#define DMA_STAT  0x5
> +#define DMA_SMDLA 0x6
> +#define DMA_WMAC  0x7
> +
> +#define DMA_CMD_MASK   0x03
> +#define DMA_CMD_DIAG   0x04
> +#define DMA_CMD_MDL    0x10
> +#define DMA_CMD_INTE_P 0x20
> +#define DMA_CMD_INTE_D 0x40
> +#define DMA_CMD_DIR    0x80
> +
> +#define DMA_STAT_PWDN    0x01
> +#define DMA_STAT_ERROR   0x02
> +#define DMA_STAT_ABORT   0x04
> +#define DMA_STAT_DONE    0x08
> +#define DMA_STAT_SCSIINT 0x10
> +#define DMA_STAT_BCMBLT  0x20
> +
> +#define SBAC_STATUS 0x1000
> +
> +typedef struct {
> +    PCIDevice dev;
> +    MemoryRegion io;
> +    uint32_t dma_regs[8];
> +    uint32_t sbac;
> +    ESPState esp;
> +} PCIESPState;

minor: preferable typedef struct PCIESPState PCIESPState;

> +
> +static void pci_handle_start(PCIESPState *pci, uint32_t val)
> +{
> +    trace_esp_pci_dma_start(val);
> +
> +    pci->dma_regs[DMA_WBC] = pci->dma_regs[DMA_STC];
> +    pci->dma_regs[DMA_WAC] = pci->dma_regs[DMA_SPA];
> +    pci->dma_regs[DMA_WMAC] = pci->dma_regs[DMA_SMDLA];
> +
> +    pci->dma_regs[DMA_STAT] &= ~(DMA_STAT_BCMBLT | DMA_STAT_SCSIINT
> +                               | DMA_STAT_DONE | DMA_STAT_ABORT
> +                               | DMA_STAT_ERROR | DMA_STAT_PWDN);
> +
> +    handle_ti(&pci->esp);
> +}
> +
> +static void pci_handle_blast(PCIESPState *pci, uint32_t val)
> +{
> +    trace_esp_pci_dma_blast(val);
> +    hw_error("am53c974: cmd BLAST not implemented");
> +}
> +
> +static void pci_handle_abort(PCIESPState *pci, uint32_t val)
> +{
> +    trace_esp_pci_dma_abort(val);
> +    if (pci->esp.current_req) {
> +        scsi_req_cancel(pci->esp.current_req);
> +    }
> +}
> +

It is better to prefix all global scope symbols with
esp_ consistently. Reduces the chance of a conflict.

> +static void pci_esp_dma_write(PCIESPState *pci, uint32_t saddr, uint32_t val)
> +{
> +    trace_esp_pci_dma_write(saddr, pci->dma_regs[saddr], val);
> +    switch (saddr) {
> +    case DMA_CMD:
> +        pci->dma_regs[saddr] = val;
> +        switch (val & DMA_CMD_MASK) {
> +        case 0x0: /* IDLE */
> +            trace_esp_pci_dma_idle(val);
> +            break;
> +        case 0x1: /* BLAST */
> +            pci_handle_blast(pci, val);

fall-through intentional?

> +        case 0x2: /* ABORT */
> +            pci_handle_abort(pci, val);
> +        case 0x3: /* START */
> +            pci_handle_start(pci, val);
> +            break;
> +        default:
> +            error_report("am53c974: unhandled command (%2.2x)",
> +                         val & DMA_CMD_MASK);
> +            break;
> +        }
> +        break;
> +    case DMA_STC:
> +    case DMA_SPA:
> +    case DMA_SMDLA:
> +        pci->dma_regs[saddr] = val;
> +        break;
> +    case DMA_STAT:
> +        if (!(pci->sbac & SBAC_STATUS)) {
> +            /* clear some bits on write */
> +            uint32_t mask = DMA_STAT_ERROR | DMA_STAT_ABORT | DMA_STAT_DONE;
> +            pci->dma_regs[DMA_STAT] &= ~(val & mask);
> +        }
> +        break;
> +    default:
> +        error_report("am53c974: invalid write of 0x%08x at [0x%x]", val, 
> saddr);
> +        return;
> +    }
> +}
> +
> +static uint32_t pci_esp_dma_read(PCIESPState *pci, uint32_t saddr)
> +{
> +    uint32_t val;
> +
> +    val = pci->dma_regs[saddr];
> +    if (saddr == DMA_STAT) {
> +        if (pci->esp.rregs[ESP_RSTAT] & STAT_INT) {
> +            val |= DMA_STAT_SCSIINT;
> +        }
> +        if (pci->sbac & SBAC_STATUS) {
> +            pci->dma_regs[DMA_STAT] &= ~(DMA_STAT_ERROR | DMA_STAT_ABORT |
> +                                         DMA_STAT_DONE);
> +        }
> +    }
> +
> +    trace_esp_pci_dma_read(saddr, val);
> +    return val;
> +}
> +
> +static void pci_esp_io_write(void *opaque, target_phys_addr_t addr,
> +                             uint64_t val, unsigned int size)
> +{
> +    PCIESPState *pci = opaque;
> +
> +    if (addr < 0x40) {
> +        /* SCSI core reg */
> +        esp_reg_write(&pci->esp, addr >> 2, val);
> +    } else if (addr < 0x60) {
> +        /* PCI DMA CCB */
> +        pci_esp_dma_write(pci, (addr - 0x40) >> 2, val);
> +    } else if (addr == 0x70) {
> +        /* DMA SCSI Bus and control */
> +        trace_esp_pci_sbac_write(pci->sbac, val);
> +        pci->sbac = val;
> +    } else {
> +        error_report("am53c974: write access outside bounds (reg 0x%x)",
> +                     (int)addr);
> +    }
> +}
> +
> +static uint64_t pci_esp_io_read(void *opaque, target_phys_addr_t addr,
> +                                unsigned int size)
> +{
> +    PCIESPState *pci = opaque;
> +
> +    if (addr < 0x40) {
> +        /* SCSI core reg */
> +        return esp_reg_read(&pci->esp, addr >> 2);
> +    } else if (addr < 0x60) {
> +        /* PCI DMA CCB */
> +        return pci_esp_dma_read(pci, (addr - 0x40) >> 2);
> +    } else if (addr == 0x70) {
> +        /* DMA SCSI Bus and control */
> +        trace_esp_pci_sbac_read(pci->sbac);
> +        return pci->sbac;
> +    } else {
> +        /* Invalid region */
> +        error_report("am53c974: read access outside bounds (reg 0x%x)",
> +                     (int)addr);
> +        return 0;
> +    }
> +}
> +
> +static void pci_esp_dma_memory_rw(PCIESPState *pci, uint8_t *buf, int len,
> +                                  DMADirection dir)
> +{
> +    dma_addr_t addr;
> +    DMADirection expected_dir;
> +
> +    if (pci->dma_regs[DMA_CMD] & DMA_CMD_DIR) {
> +        expected_dir = DMA_DIRECTION_FROM_DEVICE;
> +    } else {
> +        expected_dir = DMA_DIRECTION_TO_DEVICE;
> +    }
> +
> +    if (dir != expected_dir) {
> +        error_report("am53c974: invalid dma transfer direction");
> +        return;
> +    }
> +
> +    if (pci->dma_regs[DMA_STAT] & DMA_CMD_MDL) {
> +        hw_error("am53c974: MDL transfer not implemented");
> +    }
> +
> +    addr = pci->dma_regs[DMA_SPA];
> +    if (pci->dma_regs[DMA_WBC] < len) {
> +        len = pci->dma_regs[DMA_WBC];
> +    }
> +
> +    pci_dma_rw(&pci->dev, addr, buf, len, dir);
> +
> +    /* update status registers */
> +    pci->dma_regs[DMA_WBC] -= len;
> +    pci->dma_regs[DMA_WAC] += len;
> +}
> +
> +static void pci_esp_dma_memory_read(void *opaque, uint8_t *buf, int len)
> +{
> +    PCIESPState *pci = opaque;
> +    pci_esp_dma_memory_rw(pci, buf, len, DMA_DIRECTION_TO_DEVICE);
> +}
> +
> +static void pci_esp_dma_memory_write(void *opaque, uint8_t *buf, int len)
> +{
> +    PCIESPState *pci = opaque;
> +    pci_esp_dma_memory_rw(pci, buf, len, DMA_DIRECTION_FROM_DEVICE);
> +}
> +
> +static const MemoryRegionOps pci_esp_io_ops = {
> +    .read = pci_esp_io_read,
> +    .write = pci_esp_io_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +};
> +
> +static void pci_esp_hard_reset(DeviceState *dev)
> +{
> +    PCIESPState *pci = DO_UPCAST(PCIESPState, dev.qdev, dev);
> +    esp_hard_reset(&pci->esp);
> +    pci->esp.dma_enabled = 1;
> +    pci->dma_regs[DMA_CMD] &= ~(DMA_CMD_DIR | DMA_CMD_INTE_D | DMA_CMD_INTE_P
> +                              | DMA_CMD_MDL | DMA_CMD_DIAG | DMA_CMD_MASK);
> +    pci->dma_regs[DMA_WBC] &= ~0xffff;
> +    pci->dma_regs[DMA_WAC] = 0xffffffff;
> +    pci->dma_regs[DMA_STAT] &= ~(DMA_STAT_BCMBLT | DMA_STAT_SCSIINT
> +                               | DMA_STAT_DONE | DMA_STAT_ABORT
> +                               | DMA_STAT_ERROR);
> +    pci->dma_regs[DMA_WMAC] = 0xfffffffd;
> +}
> +
> +static const VMStateDescription vmstate_pci_esp_scsi = {
> +    .name = "pciespscsi",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .minimum_version_id_old = 0,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_PCI_DEVICE(dev, PCIESPState),
> +        VMSTATE_BUFFER_UNSAFE(dma_regs, PCIESPState, 0, 8 * 
> sizeof(uint32_t)),
> +        VMSTATE_STRUCT(esp, PCIESPState, 0, vmstate_esp, ESPState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void pci_esp_command_complete(SCSIRequest *req, uint32_t status,
> +                                     size_t resid)
> +{
> +    ESPState *s = req->hba_private;
> +    PCIESPState *pci = container_of(s, PCIESPState, esp);
> +
> +    esp_command_complete(req, status, resid);
> +    pci->dma_regs[DMA_WBC] = 0;
> +    pci->dma_regs[DMA_STAT] |= DMA_STAT_DONE;
> +}
> +
> +static const struct SCSIBusInfo pci_esp_scsi_info = {
> +    .tcq = false,
> +    .max_target = ESP_MAX_DEVS,
> +    .max_lun = 7,
> +
> +    .transfer_data = esp_transfer_data,
> +    .complete = pci_esp_command_complete,
> +    .cancel = esp_request_cancelled
> +};
> +
> +static int pci_esp_scsi_init(PCIDevice *dev)
> +{
> +    PCIESPState *pci = DO_UPCAST(PCIESPState, dev, dev);
> +    ESPState *s = &pci->esp;
> +    uint8_t *pci_conf;
> +
> +    pci_conf = pci->dev.config;
> +
> +    /* Interrupt pin A */
> +    pci_conf[PCI_INTERRUPT_PIN] = 0x01;
> +
> +    s->dma_memory_read = pci_esp_dma_memory_read;
> +    s->dma_memory_write = pci_esp_dma_memory_write;
> +    s->dma_opaque = pci;
> +    s->chip_id = TCHI_AM53C974;
> +    memory_region_init_io(&pci->io, &pci_esp_io_ops, pci, "esp-io", 0x80);
> +
> +    pci_register_bar(&pci->dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &pci->io);
> +    s->irq = pci->dev.irq[0];
> +
> +    scsi_bus_new(&s->bus, &dev->qdev, &pci_esp_scsi_info);
> +    if (!dev->qdev.hotplugged) {
> +        return scsi_bus_legacy_handle_cmdline(&s->bus);
> +    }
> +    return 0;
> +}
> +
> +static int pci_esp_scsi_uninit(PCIDevice *d)
> +{
> +    PCIESPState *pci = DO_UPCAST(PCIESPState, dev, d);
> +
> +    memory_region_destroy(&pci->io);
> +
> +    return 0;
> +}
> +
> +static void pci_esp_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> +    k->init = pci_esp_scsi_init;
> +    k->exit = pci_esp_scsi_uninit;
> +    k->vendor_id = PCI_VENDOR_ID_AMD;
> +    k->device_id = PCI_DEVICE_ID_AMD_SCSI;
> +    k->revision = 0x10;
> +    k->class_id = PCI_CLASS_STORAGE_SCSI;
> +    dc->desc = "AMD Am53c974 PCscsi-PCI SCSI adapter";
> +    dc->reset = pci_esp_hard_reset;
> +    dc->vmsd = &vmstate_pci_esp_scsi;
> +}
> +
> +static TypeInfo pci_esp_info = {
> +    .name = "am53c974",
> +    .parent = TYPE_PCI_DEVICE,
> +    .instance_size = sizeof(PCIESPState),
> +    .class_init = pci_esp_class_init,
> +};
> +
>  static void esp_register_types(void)
>  {
>      type_register_static(&sysbus_esp_info);
> +    type_register_static(&pci_esp_info);
>  }
>  
>  type_init(esp_register_types)
> diff --git a/hw/pci_ids.h b/hw/pci_ids.h
> index e8235a7..9d6e30b 100644
> --- a/hw/pci_ids.h
> +++ b/hw/pci_ids.h
> @@ -57,6 +57,7 @@
>  
>  #define PCI_VENDOR_ID_AMD                0x1022
>  #define PCI_DEVICE_ID_AMD_LANCE          0x2000
> +#define PCI_DEVICE_ID_AMD_SCSI           0x2020
>  
>  #define PCI_VENDOR_ID_TI                 0x104c
>  
> diff --git a/trace-events b/trace-events
> index 81780f0..8acb647 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -639,6 +639,14 @@ esp_mem_writeb_cmd_selatn(uint32_t val) "Select with ATN 
> (%2.2x)"
>  esp_mem_writeb_cmd_selatns(uint32_t val) "Select with ATN & stop (%2.2x)"
>  esp_mem_writeb_cmd_ensel(uint32_t val) "Enable selection (%2.2x)"
>  esp_mem_writeb_cmd_dissel(uint32_t val) "Disable selection (%2.2x)"
> +esp_pci_dma_read(uint32_t saddr, uint32_t reg) "reg[%d]: 0x%8.8x"
> +esp_pci_dma_write(uint32_t saddr, uint32_t reg, uint32_t val) "reg[%d]: 
> 0x%8.8x -> 0x%8.8x"
> +esp_pci_dma_idle(uint32_t val) "IDLE (%.8x)"
> +esp_pci_dma_blast(uint32_t val) "BLAST (%.8x)"
> +esp_pci_dma_abort(uint32_t val) "ABORT (%.8x)"
> +esp_pci_dma_start(uint32_t val) "START (%.8x)"
> +esp_pci_sbac_read(uint32_t reg) "sbac: 0x%8.8x"
> +esp_pci_sbac_write(uint32_t reg, uint32_t val) "sbac: 0x%8.8x -> 0x%8.8x"
>  
>  # monitor.c
>  handle_qmp_command(void *mon, const char *cmd_name) "mon %p cmd_name \"%s\""
> -- 
> 1.7.10