Re: [Qemu-devel] [PATCH] EHCI emulation module for review

[Paul Bolle]

On Fri, 2008-10-31 at 00:05 +0000, Mark Burkley wrote:
> Thank for your suggestions to improve the EHCI emulation patch.  As
> this is a new feature it isn't really possible to break it into
> smaller patches, but I have as suggested diffed and tested against
> trunk, updated the coding style and dropped outlook as a mailer.  I
> just tested with a host usb msd device under FC8 and it works fine
> with a WinXP target.

A number of comments have been made about (previous versions of) this
patch (the last substantial remark I found was
http://lists.gnu.org/archive/html/qemu-devel/2009-03/msg01291.html), but
not much has seem to be happening with this patch since this was sent.
For what it's still worth - after about six months - here will follow
some issues I ran into with the code.

I should note that after fixing those issues I could get a Fedora 11
Beta guest to notice the EHCI host controller but it somehow never
noticed devices I added. I have not yet investigated what is (not) going
on there. 

> This patch does include the mod to usb-linux.c to return STALL instead
> of NAK for EPIPE errors as discussed earlier.

I've been playing with this part of the patch with good results for UHCI
emulation. It was already ACK'ed here:
http://lists.gnu.org/archive/html/qemu-devel/2008-11/msg00032.html Is
this ready to be committed?

> Any other comments, suggestions and corrections welcome.

This patch was badly mangled and needed a lot of manual correction.

> Index: usb-linux.c
> ===================================================================
> --- usb-linux.c       (revision 5571)
> +++ usb-linux.c       (working copy)
> @@ -68,7 +68,7 @@
>  static int usb_host_find_device(int *pbus_num, int *paddr,
>                                  char *product_name, int
> product_name_size,
>                                  const char *devname);
> -//#define DEBUG
> +// #define DEBUG
>  
>  #ifdef DEBUG
>  #define dprintf printf
> @@ -276,7 +276,9 @@
>  
>              case -EPIPE:
>                  set_halt(s, p->devep);
> -                /* fall through */
> +             p->len = USB_RET_STALL;
> +             break;
> +
>              default:
>                  p->len = USB_RET_NAK;
>                  break;

See above: include as separate patch (minus the whitespace part)?

> Index: hw/usb-ehci.c
> ===================================================================
> --- hw/usb-ehci.c     (revision 0)
> +++ hw/usb-ehci.c     (revision 0)
> @@ -0,0 +1,2041 @@

[...]

> +typedef struct {
> +    PCIDevice dev;
> +    qemu_irq irq;
> +    target_phys_addr_t mem_base;

I had to remove this member (see below).

> +    int mem;
> +    int num_ports;
> +    /*  
> +     *  EHCI spec version 1.0 Section 2.3
> +     *  Host Controller Operational Registers
> +     */
> +    union {
> +        uint8_t mmio[MMIO_SIZE];
> +        struct {
> +            uint8_t cap[OPREGBASE];
> +            uint32_t usbcmd;
> +            uint32_t usbsts;
> +            uint32_t usbintr;
> +            uint32_t frindex;
> +            uint32_t ctrldssegment;
> +            uint32_t periodiclistbase;
> +            uint32_t asynclistaddr;
> +            uint32_t notused[9];
> +            uint32_t configflag;
> +            uint32_t portsc[NB_PORTS];
> +        };
> +    };
> +    /*
> +     *  Internal states, shadow registers, etc
> +     */
> +    uint32_t sofv;
> +    QEMUTimer *frame_timer;
> +    int attach_poll_counter;
> +    int astate;                        // Current state in asynchronous 
> schedule
> +    int pstate;                        // Current state in periodic schedule
> +    USBPort ports[NB_PORTS];
> +    unsigned char buffer[BUFF_SIZE];
> +    EHCIqh qh;
> +    EHCIqtd qtd;
> +    USBPacket usb_packet;
> +    int async_port_in_progress;
> +    int async_complete;
> +    uint32_t qhaddr;
> +    uint32_t itdaddr;
> +    uint32_t qtdaddr;
> +    uint32_t tbytes;
> +    int pid;
> +    int exec_status;
> +    int isoch_pause;
> +    uint32_t last_run_usec;
> +    uint32_t frame_end_usec;
> +} EHCIState;

[...]

> +static void dump_ptr(char *s, uint32_t ptr, int has_type)

"const char *s" to keep gcc silent?

> +{
> +    int t = NLPTR_TYPE_GET(ptr);
> +
> +    printf("%s%08X", s, NLPTR_GET(ptr));
> +    
> +    if (has_type) {
> +        printf("(PTR type is %s)", 
> +            t == NLPTR_TYPE_ITD ? "ITD" :
> +           (t == NLPTR_TYPE_QH ? "QH" :
> +           (t == NLPTR_TYPE_STITD ? "STITD" :
> +           (t == NLPTR_TYPE_FSTN ? "FSTN" : "****"))));
> +    }
> +        
> +    printf("%s\n", NLPTR_TBIT(ptr) ? " TBIT set" : "");
> +}

[...]

> +int timed_printf(char *fmt, ...)
> +{
> +    int msec, usec;
> +    static int usec_last;
> +    va_list ap;
> +
> +    usec = qemu_get_clock(vm_clock) / 1000;
> +
> +    msec = usec - usec_last;
> +    usec_last = usec;
> +    usec = msec;
> +
> +    msec /= 1000;
> +    msec %= 1000;
> +
> +    usec %= 1000;
> +
> +    va_start(ap, fmt);
> +    printf("%03d.%03d ", msec, usec);
> +    vprintf(fmt, ap);
> +    va_end(ap);
> +
> +    return 0;
> +}

Wrap timed_printf() in #ifdef DEBUG / #endif?

[...]

> +static uint32_t ehci_mem_readb(void *ptr, target_phys_addr_t addr)
> +{
> +    EHCIState *s = ptr;
> +    uint32_t val;
> +
> +    addr -= s->mem_base;

mem_base: see below.

> +    val = s->mmio[addr];
> +
> +    DEBUG(("mem_readb : addr=0x%08X, val=%02X\n",(int)addr,(int)val));
> +
> +    return val;
> +}
> +
> +static uint32_t ehci_mem_readw(void *ptr, target_phys_addr_t addr)
> +{
> +    EHCIState *s = ptr;
> +    uint32_t val;
> +
> +    addr -= s->mem_base;

mem_base: see below.

> +    val = s->mmio[addr] |(s->mmio[addr+1] << 8);
> +
> +    DEBUG(("mem_readw : addr=0x%08X, val=0x%04X\n",(int)addr,(int)val));
> +
> +    return val;
> +}
> +
> +static uint32_t ehci_mem_readl(void *ptr, target_phys_addr_t addr)
> +{
> +    EHCIState *s = ptr;
> +    uint32_t val;
> +
> +    addr -= s->mem_base;

If I did not remove the (above line and the other) references to
mem_base this code would always segfault here.

> +    val = s->mmio[addr] |(s->mmio[addr+1] << 8) |
> +        (s->mmio[addr+2] << 16) |(s->mmio[addr+3] << 24);
> +
> +    DEBUG(("mem_readl : addr=0x%08X, val=0x%08X\n",(unsigned) addr,
> val));
> +
> +    return val;
> +}

[...]

> +static void ehci_mem_writel(void *ptr, target_phys_addr_t addr, uint32_t val)
> +{
> +    EHCIState *s = ptr;
> +    int i;
> +
> +    addr -= s->mem_base;

mem_base: see above.

> +
> +    /* Only aligned reads are allowed on OHCI */
> +    if (addr & 3) {
> +        printf("usb-ehci: Mis-aligned write\n");
> +        return;
> +    }
> +
> +    if (addr >= PORTSC && addr < PORTSC + 4 * NB_PORTS) {
> +        DEBUG(("mem_writel : PORTSC%d->0x%08X \n",(int)(addr-PORTSC)/4, 
> val));
> +        handle_port_status_write(s,(addr-PORTSC)/4, val);
> +        return;
> +    }
> +
> +    /* Do any register specific pre-write processing here.  */
> +
> +    switch(addr) 
> +    {
> +    case USBCMD:
> +        if ((val & USBCMD_RUNSTOP) && !(s->usbcmd & USBCMD_RUNSTOP)) {
> +            DEBUG(("mem_writel : USBCMD run, clear halt\n"));
> +            // printf("Call mod_timer for %p\n", s->frame_timer);
> +            qemu_mod_timer(s->frame_timer, qemu_get_clock(vm_clock));
> +            s->last_run_usec = qemu_get_clock(vm_clock) / 1000;
> +            s->usbsts &= ~USBSTS_HALT;
> +        }
> +
> +        if (!(val & USBCMD_RUNSTOP) &&(s->usbcmd & USBCMD_RUNSTOP)) {
> +            DEBUG(("mem_writel : USBCMD  ** STOP **\n"));
> +            // printf("Call del_timer for %p\n", s->frame_timer);
> +#if 0
> +            qemu_del_timer(s->frame_timer);
> +#endif
> +            // TODO - should finish out some stuff before setting halt
> +            s->usbsts |= USBSTS_HALT;
> +        }
> +
> +        if (val & USBCMD_HCRESET) {
> +            DEBUG(("mem_writel : USBCMD resetting ...\n"));
> +            ehci_reset(s);
> +            DEBUG(("mem_writel : USBCMD reset done, clear reset request 
> bit\n"));
> +            val &= ~USBCMD_HCRESET;
> +        }
> +
> +            break;
> +
> +    case USBSTS:
> +        val &= USBSTS_RO_MASK;              // bits 6 thru 31 are RO
> +        DEBUG(("mem_writel : USBSTS RWC set to 0x%08X\n", val));
> +
> +        val =(s->usbsts &= ~val);         // bits 0 thru 5 are R/WC
> +        DEBUG(("mem_writel : USBSTS updating interrupt condition\n"));
> +        ehci_set_interrupt(s, 0);
> +
> +        break;
> +
> +    case USBINTR:
> +        val &= USBINTR_MASK;
> +        // DEBUG(("mem_writel : USBINTR set to 0x%08X\n", val));
> +        break;
> +    
> +    case FRINDEX:
> +        s->sofv = val >> 3;
> +        break;
> +        
> +    case CONFIGFLAG:
> +        val &= 0x1;
> +
> +        if (val) {
> +            for(i = 0; i < NB_PORTS; i++)
> +                s->portsc[i] &= ~PORTSC_POWNER;
> +        }
> +
> +        break;
> +    }
> +
> +    if (addr != 0x28) {
> +        DEBUG(("mem_writel : addr=0x%08X, val=0x%08X\n",(unsigned) addr, 
> val));
> +    }
> +
> +    *(uint32_t *)(&s->mmio[addr]) = val;
> +}

[...]
 
> +// 4.10.2
> +
> +static void ehci_qh_do_overlay(EHCIState *ehci, EHCIqh *qh, EHCIqtd *qtd)
> +{
> +    int i;
> +    int dtoggle;
> +    int ping;
> +
> +    // remember values in fields to preserve in qh after overlay
> +
> +    dtoggle = qh->token & QTD_TOKEN_DTOGGLE;
> +    ping = qh->token & QTD_TOKEN_PING;
> +
> +    DEBUG(("setting qh.current from %08X to 0x%08X\n", qh->current,
> +            ehci->qtdaddr));
> +    qh->current  = ehci->qtdaddr;
> +    qh->qtdnext  = qtd->next;
> +    qh->altnext  = qtd->altnext;
> +    qh->token    = qtd->token;
> +
> +    if (qh->current < 0x1000) {
> +#ifdef DEBUG_PACKET
> +        dump_qh(qh, qh->current);
> +#endif
> +        ASSERT(1==2);
> +    }
> +    
> +    if (((qh->epchar & QH_EPCHAR_EPS_MASK) >> QH_EPCHAR_EPS_SH) == 2) {
> +        qh->token &= ~QTD_TOKEN_PING;
> +        qh->token |= ping;
> +    }
> +
> +    for(i = 0; i < 6; i++)

gcc warned here. It seemed happier with: "i < 5".

> +        qh->bufptr[i] = qtd->bufptr[i];
> +
> +    if (!(qh->epchar & QH_EPCHAR_DTC)) {
> +        // preserve QH DT bit
> +        qh->token &= ~QTD_TOKEN_DTOGGLE;
> +        qh->token |= dtoggle;
> +    }
> +
> +    qh->bufptr[1] &= ~BUFPTR_CPROGMASK_MASK;
> +    qh->bufptr[2] &= ~BUFPTR_FRAMETAG_MASK;
> +
> +    // TODO NakCnt
> +}

[...]        

> +static int transactid;

Wrap that line in #ifdef TDEBUG / #endif?

> +static void ehci_async_complete_packet(USBPacket *packet, void *opaque)
> +{
> +    EHCIState *ehci = opaque;
> +#ifdef DEBUG_PACKET
> +    dprintf("Async packet complete\n");
> +#endif
> +    ehci->async_complete = 1;
> +    ehci->exec_status = packet->len;
> +}
> +
> +static int ehci_execute_complete(EHCIState *ehci, 
> +                                  EHCIqh *qh,
> +                                  int ret)
> +{
> +    if (ret == USB_RET_ASYNC && !ehci->async_complete) {
> +        DEBUG(("not done yet\n"));
> +        return ret;
> +    }
> +
> +    ehci->async_complete = 0;
> +    ehci->async_port_in_progress = -1;
> +
> +    if (ret < 0) {
> +        switch(ret) {
> +        case USB_RET_NODEV:
> +            printf("USB no device\n");
> +            break;
> +        case USB_RET_STALL:
> +            printf("USB stall\n");

In the one guest that actually noticed a USB device under EHCI emulation
I always hit this messages. After that I had to kill qemu, as it seemed
to hang or to be stuck in a tight loop, but the USB device seemed to
sort of hang even after qemu was killed (its "activity" led was busy
blinking for minutes after qemu was killed)! Strange?

> +            qh->token |= QTD_TOKEN_HALT;
> +            break;
> +        case USB_RET_NAK:
> +            DEBUG(("USBTRAN RSP NAK, returning without clear active
> \n"));
> +            return USB_RET_NAK;
> +            break;
> +        case USB_RET_BABBLE:
> +            printf("USB babble TODO\n");
> +            ASSERT(ret >= 0);
> +            break;
> +        default:
> +            printf("USB invalid response %d to handle\n",
> +                    ret);
> +            ASSERT(ret >= 0);
> +            break;
> +        }
> +    } else {
> +        // if (ret < maxpkt)
> +        // {
> +        //     DEBUG(("Short packet condition\n"));
> +        //     // TODO check 4.12 for splits
> +         // }
> +    
> +        if (ehci->tbytes && ehci->pid == USB_TOKEN_IN) {
> +            ASSERT(ret > 0);
> +            ehci_buffer_rw(ehci, qh, ret, 1);
> +#ifdef TDEBUG
> +            printf("Data after execution:\n");
> +            // dump_data(ehci->buffer, ehci->tbytes < 64 ?
> ehci->tbytes : 64);
> +            // decode_data(ehci->pid, ehci->buffer, ret);
> +#endif
> +            ehci->tbytes -= ret;
> +        } else
> +            ehci->tbytes = 0;
> +
> +        ASSERT(ehci->tbytes >= 0);
> +
> +        set_field(&qh->token, ehci->tbytes, 
> +                   QTD_TOKEN_TBYTES_MASK, QTD_TOKEN_TBYTES_SH);
> +    }
> +
> +    qh->token ^= QTD_TOKEN_DTOGGLE;
> +    qh->token &= ~QTD_TOKEN_ACTIVE;
> +
> +    if (qh->token & QTD_TOKEN_IOC) {
> +        // TODO should do this after writeback to memory
> +        ehci_set_interrupt(ehci, USBSTS_INT);
> +    }
> +#ifdef DEBUG_PACKET
> +    DEBUG(("QH after execute:-\n"));
> +    dump_qh(qh, NLPTR_GET(ehci->qhaddr));
> +#endif
> +
> +#ifdef TDEBUG
> +    printf("USBTRAN RSP %3d                          return:(%5d) ",
> +            transactid,
> +            ret);
> +
> +    if (ehci->pid == USB_TOKEN_IN) {
> +        printf("[%02X %02X %02X %02X ...]\n",
> +            *ehci->buffer, *(ehci->buffer+1), 
> +            *(ehci->buffer+2), *(ehci->buffer+3));
> +    }
> +    else
> +        printf("\n");
> +#endif
> +    return ret;
> +}

[...]

> +static void ehci_map(PCIDevice *pci_dev, int region_num, 
> +                    uint32_t addr, uint32_t size, int type)
> +{
> +    EHCIState *s =(EHCIState *)pci_dev;
> +
> +    DEBUG(("ehci_map: region %d, addr %08X, size %d, s->mem %08X\n",
> +            region_num, addr, size, s->mem));
> +    s->mem_base = addr;

mem_base: see above.

> +    cpu_register_physical_memory(addr, size, s->mem);
> +}
> +

[...]

> +
> +
> +void usb_ehci_init(PCIBus *bus, int devfn)

usb_ehci_init() or usb_ehci_init_pci()?

> +{
> +    EHCIState *ehci;
> +    uint8_t *pci_conf;
> +    int i;
> +
> +    ehci =(EHCIState *)pci_register_device(bus,
> +                                        "USB-EHCI", sizeof(EHCIState),
> +                                        devfn, NULL, NULL);
> +
> +    pci_conf = ehci->dev.config;
> +    pci_conf[0x00] = 0x86;
> +    pci_conf[0x01] = 0x80; // Intel
> +    pci_conf[0x02] = 0xCD;
> +    pci_conf[0x03] = 0x24; // ICH4 / 82801D
> +    pci_conf[0x08] = 0x10; // revision number
> +    pci_conf[0x09] = 0x20;
> +    pci_conf[0x0a] = 0x03;
> +    pci_conf[0x0b] = 0x0c;

Why not use 
    pci_config_set_class(pci_conf, PCI_CLASS_SERIAL_USB);
here?

> +    pci_conf[0x0e] = 0x00; // header_type 0
> +    pci_conf[0x34] = 0x00; // capabilities pointer
> +
> +    // pci_conf[0x34] = 0x50; // capabilities pointer
> +
> +    pci_conf[0x3d] = 4; // interrupt pin 3
> +    pci_conf[0x3e] = 0; // MaxLat
> +    pci_conf[0x3f] = 0; // MinGnt
> +
> +    // pci_conf[0x50] = 0x01; // power management caps
> +
> +    pci_conf[0x60] = 0x20;  // SBRN
> +    pci_conf[0x61] = 0x20;  // FLADJ
> +    pci_conf[0x62] = 0x7f;
> +    pci_conf[0x63] = 0x00;  // PORTWAKECAP
> +    pci_conf[0x64] = 0x00;
> +    pci_conf[0x65] = 0x00;
> +    pci_conf[0x66] = 0x00;
> +    pci_conf[0x67] = 0x00;
> +    pci_conf[0x68] = 0x01;
> +    pci_conf[0x69] = 0x00;
> +    pci_conf[0x6a] = 0x00;
> +    pci_conf[0x6b] = 0x00;  // USBLEGSUP
> +    pci_conf[0x6c] = 0x00;
> +    pci_conf[0x6d] = 0x00;
> +    pci_conf[0x6e] = 0x00;
> +    pci_conf[0x6f] = 0xc0;  // USBLEFCTLSTS

[...]

> Index: hw/pci.h
> ===================================================================
> --- hw/pci.h  (revision 5571)
> +++ hw/pci.h  (working copy)
> @@ -112,6 +112,10 @@
>  /* usb-ohci.c */
>  void usb_ohci_init_pci(struct PCIBus *bus, int num_ports, int devfn);
>  
> +/* usb-ehci.c */
> +void usb_ehci_init_pci(struct PCIBus *bus, int devfn);

See above: usb_ehci_init_pci() or usb_ehci_init()?

[...]

> Index: hw/pc.c
> ===================================================================
> --- hw/pc.c   (revision 5571)
> +++ hw/pc.c   (working copy)
> @@ -1028,8 +1028,11 @@
>  
>      cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device, hd);
>  
> +    //  TODO the piix3 does not have EHCI - looks like ICH4
> +    //  was first southbridge to support it.
>      if (pci_enabled && usb_enabled) {
> -        usb_uhci_piix3_init(pci_bus, piix3_devfn + 2);
> +        usb_uhci_piix3_init(pci_bus, piix3_devfn + 3);
> +        usb_ehci_init(pci_bus, piix3_devfn + 2);

See above: usb_ehci_init_pci() or usb_ehci_init()?

>      }
>  
>      if (pci_enabled && acpi_enabled) {
> @@ -1037,7 +1040,7 @@
>          i2c_bus *smbus;
>  
>          /* TODO: Populate SPD eeprom data.  */
> -        smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
> i8259[9]);
> +        smbus = piix4_pm_init(pci_bus, piix3_devfn + 4, 0xb100, i8259[9]);
>          for (i = 0; i < 8; i++) {
>              smbus_eeprom_device_init(smbus, 0x50 + i, eeprom_buf + (i
> * 256));
>          }