[Qemu-devel] [PATCH v1 0/8] Sysbus EHCI + Zynq USB.

[Qemu-devel] [PATCH v1 0/8] Sysbus EHCI + Zynq USB.

[Peter Crosthwaite]

Added Sysbus variant of EHCI and attached it to Xilinx Zynq. Apparently the EHCI stuff is going to useful for Tegra too. (Andreas?)

See http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg04100.html thread for the inital RFC discussion on this development.

Regession tested using i386 Debian (dd of=/dev/sdx ...)  to get some test coverage outside of Zynq. More testing welcome.

Few other bits and pieces of cleanup in there too, around the EHCI_DEBUG printfery.

Peter Crosthwaite (8):
  usb/ehci: parameterise the register region offsets
  usb/ehci: Abstract away PCI DMA API
  usb/ehci: seperate out PCIisms
  usb/ehci: Add usb-ehci-sysbus
  xilinx_zynq: add USB controllers
  usb/ehci: Guard definition of EHCI_DEBUG
  usb/ehci: Debug mode compile fixes
  usb/ehci: Put RAM in undefined MMIO regions

 hw/usb/hcd-ehci.c |  285 +++++++++++++++++++++++++++++++++++------------------
 hw/xilinx_zynq.c  |   16 +++
 2 files changed, 204 insertions(+), 97 deletions(-)

[Qemu-devel] [PATCH v1 1/8] usb/ehci: parameterise the register region offsets

[Peter Crosthwaite]

The capabilities register and operational register offsets can vary from one
EHCI implementation to the next. Parameterise accordingly.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/usb/hcd-ehci.c |   68 ++++++++++++++++++++++++++++-------------------------
 1 files changed, 36 insertions(+), 32 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 6c65a73..ddf63d6 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -50,18 +50,15 @@
 #define MMIO_SIZE        0x1000
 
 /* Capability Registers Base Address - section 2.2 */
-#define CAPREGBASE       0x0000
-#define CAPLENGTH        CAPREGBASE + 0x0000  // 1-byte, 0x0001 reserved
-#define HCIVERSION       CAPREGBASE + 0x0002  // 2-bytes, i/f version #
-#define HCSPARAMS        CAPREGBASE + 0x0004  // 4-bytes, structural params
-#define HCCPARAMS        CAPREGBASE + 0x0008  // 4-bytes, capability params
+#define CAPLENGTH        0x0000  // 1-byte, 0x0001 reserved
+#define HCIVERSION       0x0002  // 2-bytes, i/f version #
+#define HCSPARAMS        0x0004  // 4-bytes, structural params
+#define HCCPARAMS        0x0008  // 4-bytes, capability params
 #define EECP             HCCPARAMS + 1
-#define HCSPPORTROUTE1   CAPREGBASE + 0x000c
-#define HCSPPORTROUTE2   CAPREGBASE + 0x0010
+#define HCSPPORTROUTE1   0x000c
+#define HCSPPORTROUTE2   0x0010
 
-#define OPREGBASE        0x0020        // Operational Registers Base Address
-
-#define USBCMD           OPREGBASE + 0x0000
+#define USBCMD           0x0000
 #define USBCMD_RUNSTOP   (1 << 0)      // run / Stop
 #define USBCMD_HCRESET   (1 << 1)      // HC Reset
 #define USBCMD_FLS       (3 << 2)      // Frame List Size
@@ -75,7 +72,7 @@
 #define USBCMD_ITC       (0x7f << 16)  // Int Threshold Control
 #define USBCMD_ITC_SH    16            // Int Threshold Control Shift
 
-#define USBSTS           OPREGBASE + 0x0004
+#define USBSTS           0x0004
 #define USBSTS_RO_MASK   0x0000003f
 #define USBSTS_INT       (1 << 0)      // USB Interrupt
 #define USBSTS_ERRINT    (1 << 1)      // Error Interrupt
@@ -92,18 +89,18 @@
  *  Interrupt enable bits correspond to the interrupt active bits in USBSTS
  *  so no need to redefine here.
  */
-#define USBINTR              OPREGBASE + 0x0008
+#define USBINTR              0x0008
 #define USBINTR_MASK         0x0000003f
 
-#define FRINDEX              OPREGBASE + 0x000c
-#define CTRLDSSEGMENT        OPREGBASE + 0x0010
-#define PERIODICLISTBASE     OPREGBASE + 0x0014
-#define ASYNCLISTADDR        OPREGBASE + 0x0018
+#define FRINDEX              0x000c
+#define CTRLDSSEGMENT        0x0010
+#define PERIODICLISTBASE     0x0014
+#define ASYNCLISTADDR        0x0018
 #define ASYNCLISTADDR_MASK   0xffffffe0
 
-#define CONFIGFLAG           OPREGBASE + 0x0040
+#define CONFIGFLAG           0x0040
 
-#define PORTSC               (OPREGBASE + 0x0044)
+#define PORTSC               0x0044
 #define PORTSC_BEGIN         PORTSC
 #define PORTSC_END           (PORTSC + 4 * NB_PORTS)
 /*
@@ -399,14 +396,16 @@ struct EHCIState {
 
     /* properties */
     uint32_t maxframes;
+    uint16_t opregbase;
+    uint16_t capabase;
 
     /*
      *  EHCI spec version 1.0 Section 2.3
      *  Host Controller Operational Registers
      */
-    uint8_t caps[OPREGBASE];
+    uint8_t *caps;
     union {
-        uint32_t opreg[(PORTSC_BEGIN-OPREGBASE)/sizeof(uint32_t)];
+        uint32_t opreg[PORTSC_BEGIN/sizeof(uint32_t)];
         struct {
             uint32_t usbcmd;
             uint32_t usbsts;
@@ -505,8 +504,7 @@ static const char *state2str(uint32_t state)
 
 static const char *addr2str(hwaddr addr)
 {
-    return nr2str(ehci_mmio_names, ARRAY_SIZE(ehci_mmio_names),
-                  addr + OPREGBASE);
+    return nr2str(ehci_mmio_names, ARRAY_SIZE(ehci_mmio_names), addr);
 }
 
 static void ehci_trace_usbsts(uint32_t mask, int state)
@@ -1114,7 +1112,7 @@ static uint64_t ehci_opreg_read(void *ptr, hwaddr addr,
     uint32_t val;
 
     val = s->opreg[addr >> 2];
-    trace_usb_ehci_opreg_read(addr + OPREGBASE, addr2str(addr), val);
+    trace_usb_ehci_opreg_read(addr + s->opregbase, addr2str(addr), val);
     return val;
 }
 
@@ -1210,9 +1208,9 @@ static void ehci_opreg_write(void *ptr, hwaddr addr,
     uint32_t old = *mmio;
     int i;
 
-    trace_usb_ehci_opreg_write(addr + OPREGBASE, addr2str(addr), val);
+    trace_usb_ehci_opreg_write(addr + s->opregbase, addr2str(addr), val);
 
-    switch (addr + OPREGBASE) {
+    switch (addr) {
     case USBCMD:
         if (val & USBCMD_HCRESET) {
             ehci_reset(s);
@@ -1290,7 +1288,8 @@ static void ehci_opreg_write(void *ptr, hwaddr addr,
     }
 
     *mmio = val;
-    trace_usb_ehci_opreg_change(addr + OPREGBASE, addr2str(addr), *mmio, old);
+    trace_usb_ehci_opreg_change(addr + s->opregbase, addr2str(addr),
+                                *mmio, old);
 }
 
 
@@ -2638,6 +2637,8 @@ static const VMStateDescription vmstate_ehci = {
 
 static Property ehci_properties[] = {
     DEFINE_PROP_UINT32("maxframes", EHCIState, maxframes, 128),
+    DEFINE_PROP_UINT16("capabase", EHCIState, capabase, 0),
+    DEFINE_PROP_UINT16("opregbase", EHCIState, opregbase, 0x20),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2718,8 +2719,10 @@ static int usb_ehci_initfn(PCIDevice *dev)
     pci_conf[0x6e] = 0x00;
     pci_conf[0x6f] = 0xc0;  // USBLEFCTLSTS
 
+    s->caps = g_malloc0(s->opregbase);
+
     /* 2.2 host controller interface version */
-    s->caps[0x00] = (uint8_t) OPREGBASE;
+    s->caps[0x00] = (uint8_t)(s->opregbase - s->capabase);
     s->caps[0x01] = 0x00;
     s->caps[0x02] = 0x00;
     s->caps[0x03] = 0x01;        /* HC version */
@@ -2752,15 +2755,16 @@ static int usb_ehci_initfn(PCIDevice *dev)
 
     memory_region_init(&s->mem, "ehci", MMIO_SIZE);
     memory_region_init_io(&s->mem_caps, &ehci_mmio_caps_ops, s,
-                          "capabilities", OPREGBASE);
+                          "capabilities", s->opregbase);
     memory_region_init_io(&s->mem_opreg, &ehci_mmio_opreg_ops, s,
-                          "operational", PORTSC_BEGIN - OPREGBASE);
+                          "operational", PORTSC_BEGIN);
     memory_region_init_io(&s->mem_ports, &ehci_mmio_port_ops, s,
                           "ports", PORTSC_END - PORTSC_BEGIN);
 
-    memory_region_add_subregion(&s->mem, 0,            &s->mem_caps);
-    memory_region_add_subregion(&s->mem, OPREGBASE,    &s->mem_opreg);
-    memory_region_add_subregion(&s->mem, PORTSC_BEGIN, &s->mem_ports);
+    memory_region_add_subregion(&s->mem, s->capabase, &s->mem_caps);
+    memory_region_add_subregion(&s->mem, s->opregbase, &s->mem_opreg);
+    memory_region_add_subregion(&s->mem, s->opregbase + PORTSC_BEGIN,
+                                &s->mem_ports);
 
     pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mem);
 
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH v1 1/8] usb/ehci: parameterise the register region offsets

[Gerd Hoffmann]

On 10/25/12 11:47, Peter Crosthwaite wrote:
> The capabilities register and operational register offsets can vary from one
> EHCI implementation to the next. Parameterise accordingly.

>  static Property ehci_properties[] = {
>      DEFINE_PROP_UINT32("maxframes", EHCIState, maxframes, 128),
> +    DEFINE_PROP_UINT16("capabase", EHCIState, capabase, 0),
> +    DEFINE_PROP_UINT16("opregbase", EHCIState, opregbase, 0x20),
>      DEFINE_PROP_END_OF_LIST(),
>  };

I don't think we want have this as properties.  Just set them in initfn().

> @@ -2718,8 +2719,10 @@ static int usb_ehci_initfn(PCIDevice *dev)
>      pci_conf[0x6e] = 0x00;
>      pci_conf[0x6f] = 0xc0;  // USBLEFCTLSTS
>  
> +    s->caps = g_malloc0(s->opregbase);

I don't think we need to make that dynamic.

opregbase just happened equal caps_size for pci-ehci due to packing
these regions without space inbetween.

>      memory_region_init_io(&s->mem_caps, &ehci_mmio_caps_ops, s,
> -                          "capabilities", OPREGBASE);
> +                          "capabilities", s->opregbase);

caps_size

cheers,
  Gerd

[Qemu-devel] [PATCH v1 2/8] usb/ehci: Abstract away PCI DMA API

[Peter Crosthwaite]

Pull the DMAContext for the PCI DMA out at device init time and put it into
the device state. Use dma_memory_read/write() instead of pci specific versions.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/usb/hcd-ehci.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index ddf63d6..53bc4e5 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -389,6 +389,7 @@ struct EHCIState {
     USBBus bus;
     qemu_irq irq;
     MemoryRegion mem;
+    DMAContext *dma;
     MemoryRegion mem_caps;
     MemoryRegion mem_opreg;
     MemoryRegion mem_ports;
@@ -1302,7 +1303,7 @@ static inline int get_dwords(EHCIState *ehci, uint32_t addr,
     int i;
 
     for(i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
-        pci_dma_read(&ehci->dev, addr, buf, sizeof(*buf));
+        dma_memory_read(ehci->dma, addr, buf, sizeof(*buf));
         *buf = le32_to_cpu(*buf);
     }
 
@@ -1317,7 +1318,7 @@ static inline int put_dwords(EHCIState *ehci, uint32_t addr,
 
     for(i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
         uint32_t tmp = cpu_to_le32(*buf);
-        pci_dma_write(&ehci->dev, addr, &tmp, sizeof(tmp));
+        dma_memory_write(ehci->dma, addr, &tmp, sizeof(tmp));
     }
 
     return 1;
@@ -1400,7 +1401,7 @@ static int ehci_init_transfer(EHCIPacket *p)
     cpage  = get_field(p->qtd.token, QTD_TOKEN_CPAGE);
     bytes  = get_field(p->qtd.token, QTD_TOKEN_TBYTES);
     offset = p->qtd.bufptr[0] & ~QTD_BUFPTR_MASK;
-    pci_dma_sglist_init(&p->sgl, &p->queue->ehci->dev, 5);
+    qemu_sglist_init(&p->sgl, 5, p->queue->ehci->dma);
 
     while (bytes > 0) {
         if (cpage > 4) {
@@ -1629,7 +1630,7 @@ static int ehci_process_itd(EHCIState *ehci,
                 return USB_RET_PROCERR;
             }
 
-            pci_dma_sglist_init(&ehci->isgl, &ehci->dev, 2);
+            qemu_sglist_init(&ehci->isgl, 2, ehci->dma);
             if (off + len > 4096) {
                 /* transfer crosses page border */
                 uint32_t len2 = off + len - 4096;
@@ -2389,7 +2390,7 @@ static void ehci_advance_periodic_state(EHCIState *ehci)
         }
         list |= ((ehci->frindex & 0x1ff8) >> 1);
 
-        pci_dma_read(&ehci->dev, list, &entry, sizeof entry);
+        dma_memory_read(ehci->dma, list, &entry, sizeof entry);
         entry = le32_to_cpu(entry);
 
         DPRINTF("PERIODIC state adv fr=%d.  [%08X] -> %08X\n",
@@ -2737,6 +2738,8 @@ static int usb_ehci_initfn(PCIDevice *dev)
 
     s->irq = s->dev.irq[3];
 
+    s->dma = pci_dma_context(dev);
+
     usb_bus_new(&s->bus, &ehci_bus_ops, &s->dev.qdev);
     for(i = 0; i < NB_PORTS; i++) {
         usb_register_port(&s->bus, &s->ports[i], s, i, &ehci_port_ops,
-- 
1.7.0.4

[Qemu-devel] [PATCH v1 3/8] usb/ehci: seperate out PCIisms

[Peter Crosthwaite]

Seperate the PCI stuff from the EHCI components. Extracted the PCIDevice
out into a new wrapper struct to make EHCIState non-PCI-specific. Seperated
tho non PCI init component out into a seperate "common" init function.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/usb/hcd-ehci.c |  132 ++++++++++++++++++++++++++++++----------------------
 1 files changed, 76 insertions(+), 56 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 53bc4e5..862564c 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -385,7 +385,6 @@ struct EHCIQueue {
 typedef QTAILQ_HEAD(EHCIQueueHead, EHCIQueue) EHCIQueueHead;
 
 struct EHCIState {
-    PCIDevice dev;
     USBBus bus;
     qemu_irq irq;
     MemoryRegion mem;
@@ -446,6 +445,11 @@ struct EHCIState {
     uint32_t async_stepdown;
 };
 
+typedef struct EHCIItfState {
+    PCIDevice pcidev;
+    struct EHCIState ehci;
+} EHCIItfState;
+
 #define SET_LAST_RUN_CLOCK(s) \
     (s)->last_run_ns = qemu_get_clock_ns(vm_clock);
 
@@ -2539,7 +2543,7 @@ static const MemoryRegionOps ehci_mmio_port_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-static int usb_ehci_initfn(PCIDevice *dev);
+static int usb_ehci_pci_initfn(PCIDevice *dev);
 
 static USBPortOps ehci_port_ops = {
     .attach = ehci_attach,
@@ -2601,11 +2605,10 @@ static void usb_ehci_vm_state_change(void *opaque, int running, RunState state)
 
 static const VMStateDescription vmstate_ehci = {
     .name        = "ehci",
-    .version_id  = 2,
-    .minimum_version_id  = 1,
+    .version_id  = 3,
+    .minimum_version_id  = 2,
     .post_load   = usb_ehci_post_load,
     .fields      = (VMStateField[]) {
-        VMSTATE_PCI_DEVICE(dev, EHCIState),
         /* mmio registers */
         VMSTATE_UINT32(usbcmd, EHCIState),
         VMSTATE_UINT32(usbsts, EHCIState),
@@ -2637,31 +2640,42 @@ static const VMStateDescription vmstate_ehci = {
 };
 
 static Property ehci_properties[] = {
-    DEFINE_PROP_UINT32("maxframes", EHCIState, maxframes, 128),
-    DEFINE_PROP_UINT16("capabase", EHCIState, capabase, 0),
-    DEFINE_PROP_UINT16("opregbase", EHCIState, opregbase, 0x20),
+    DEFINE_PROP_UINT32("maxframes", EHCIItfState, ehci.maxframes, 128),
+    DEFINE_PROP_UINT16("capabase", EHCIItfState, ehci.capabase, 0),
+    DEFINE_PROP_UINT16("opregbase", EHCIItfState, ehci.opregbase, 0x20),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-static void ehci_class_init(ObjectClass *klass, void *data)
+static const VMStateDescription vmstate_ehci_pci = {
+    .name        = "ehci-pci",
+    .version_id  = 3,
+    .minimum_version_id  = 2,
+    .post_load   = usb_ehci_post_load,
+    .fields      = (VMStateField[]) {
+        VMSTATE_PCI_DEVICE(pcidev, EHCIItfState),
+        VMSTATE_STRUCT(ehci, EHCIItfState, 3, vmstate_ehci, EHCIState),
+    }
+};
+
+static void ehci_pci_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->init = usb_ehci_initfn;
+    k->init = usb_ehci_pci_initfn;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_82801D; /* ich4 */
     k->revision = 0x10;
     k->class_id = PCI_CLASS_SERIAL_USB;
-    dc->vmsd = &vmstate_ehci;
+    dc->vmsd = &vmstate_ehci_pci;
     dc->props = ehci_properties;
 }
 
-static TypeInfo ehci_info = {
+static TypeInfo ehci_pci_info = {
     .name          = "usb-ehci",
     .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(EHCIState),
-    .class_init    = ehci_class_init,
+    .instance_size = sizeof(EHCIItfState),
+    .class_init    = ehci_pci_class_init,
 };
 
 static void ich9_ehci_class_init(ObjectClass *klass, void *data)
@@ -2669,57 +2683,27 @@ static void ich9_ehci_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->init = usb_ehci_initfn;
+    k->init = usb_ehci_pci_initfn;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1;
     k->revision = 0x03;
     k->class_id = PCI_CLASS_SERIAL_USB;
-    dc->vmsd = &vmstate_ehci;
+    dc->vmsd = &vmstate_ehci_pci;
     dc->props = ehci_properties;
 }
 
 static TypeInfo ich9_ehci_info = {
     .name          = "ich9-usb-ehci1",
     .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(EHCIState),
+    .instance_size = sizeof(EHCIItfState),
     .class_init    = ich9_ehci_class_init,
 };
 
-static int usb_ehci_initfn(PCIDevice *dev)
+
+static void usb_ehci_initfn(EHCIState *s, DeviceState *dev)
 {
-    EHCIState *s = DO_UPCAST(EHCIState, dev, dev);
-    uint8_t *pci_conf = s->dev.config;
     int i;
 
-    pci_set_byte(&pci_conf[PCI_CLASS_PROG], 0x20);
-
-    /* capabilities pointer */
-    pci_set_byte(&pci_conf[PCI_CAPABILITY_LIST], 0x00);
-    //pci_set_byte(&pci_conf[PCI_CAPABILITY_LIST], 0x50);
-
-    pci_set_byte(&pci_conf[PCI_INTERRUPT_PIN], 4); /* interrupt pin D */
-    pci_set_byte(&pci_conf[PCI_MIN_GNT], 0);
-    pci_set_byte(&pci_conf[PCI_MAX_LAT], 0);
-
-    // pci_conf[0x50] = 0x01; // power management caps
-
-    pci_set_byte(&pci_conf[USB_SBRN], USB_RELEASE_2); // release number (2.1.4)
-    pci_set_byte(&pci_conf[0x61], 0x20);  // frame length adjustment (2.1.5)
-    pci_set_word(&pci_conf[0x62], 0x00);  // port wake up capability (2.1.6)
-
-    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
-
     s->caps = g_malloc0(s->opregbase);
 
     /* 2.2 host controller interface version */
@@ -2736,11 +2720,7 @@ static int usb_ehci_initfn(PCIDevice *dev)
     s->caps[0x0a] = 0x00;
     s->caps[0x0b] = 0x00;
 
-    s->irq = s->dev.irq[3];
-
-    s->dma = pci_dma_context(dev);
-
-    usb_bus_new(&s->bus, &ehci_bus_ops, &s->dev.qdev);
+    usb_bus_new(&s->bus, &ehci_bus_ops, dev);
     for(i = 0; i < NB_PORTS; i++) {
         usb_register_port(&s->bus, &s->ports[i], s, i, &ehci_port_ops,
                           USB_SPEED_MASK_HIGH);
@@ -2768,15 +2748,55 @@ static int usb_ehci_initfn(PCIDevice *dev)
     memory_region_add_subregion(&s->mem, s->opregbase, &s->mem_opreg);
     memory_region_add_subregion(&s->mem, s->opregbase + PORTSC_BEGIN,
                                 &s->mem_ports);
+}
+
+static int usb_ehci_pci_initfn(PCIDevice *dev)
+{
+    EHCIItfState *i = DO_UPCAST(EHCIItfState, pcidev, dev);
+    EHCIState *s = &i->ehci;
+    uint8_t *pci_conf = dev->config;
+
+    pci_set_byte(&pci_conf[PCI_CLASS_PROG], 0x20);
+
+    /* capabilities pointer */
+    pci_set_byte(&pci_conf[PCI_CAPABILITY_LIST], 0x00);
+    /* pci_set_byte(&pci_conf[PCI_CAPABILITY_LIST], 0x50); */
+
+    pci_set_byte(&pci_conf[PCI_INTERRUPT_PIN], 4); /* interrupt pin D */
+    pci_set_byte(&pci_conf[PCI_MIN_GNT], 0);
+    pci_set_byte(&pci_conf[PCI_MAX_LAT], 0);
+
+    /* pci_conf[0x50] = 0x01; *//* power management caps */
+
+    pci_set_byte(&pci_conf[USB_SBRN], USB_RELEASE_2); /* release # (2.1.4) */
+    pci_set_byte(&pci_conf[0x61], 0x20);  /* frame length adjustment (2.1.5) */
+    pci_set_word(&pci_conf[0x62], 0x00);  /* port wake up capability (2.1.6) */
+
+    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 */
+
+    s->irq = dev->irq[3];
+    s->dma = pci_dma_context(dev);
 
-    pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mem);
+    usb_ehci_initfn(s, DEVICE(dev));
+    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mem);
 
     return 0;
 }
 
 static void ehci_register_types(void)
 {
-    type_register_static(&ehci_info);
+    type_register_static(&ehci_pci_info);
     type_register_static(&ich9_ehci_info);
 }
 
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH v1 3/8] usb/ehci: seperate out PCIisms

[Gerd Hoffmann]

> +typedef struct EHCIItfState {
> +    PCIDevice pcidev;
> +    struct EHCIState ehci;
> +} EHCIItfState;

EHCIPCIState ?

>  static const VMStateDescription vmstate_ehci = {
>      .name        = "ehci",
> -    .version_id  = 2,
> -    .minimum_version_id  = 1,
> +    .version_id  = 3,
> +    .minimum_version_id  = 2,

Pick a new name for this ...

> -static void ehci_class_init(ObjectClass *klass, void *data)
> +static const VMStateDescription vmstate_ehci_pci = {
> +    .name        = "ehci-pci",

... and keep using "ehci" here ...

> +    .version_id  = 3,
> +    .minimum_version_id  = 2,

... then you don't need to fiddle with the versions as the vmstate wire
format doesn't change then.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v1 3/8] usb/ehci: seperate out PCIisms

[Peter Crosthwaite]

On Thu, Oct 25, 2012 at 10:08 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> +typedef struct EHCIItfState {
>> +    PCIDevice pcidev;
>> +    struct EHCIState ehci;
>> +} EHCIItfState;
>
> EHCIPCIState ?
>
>>  static const VMStateDescription vmstate_ehci = {
>>      .name        = "ehci",
>> -    .version_id  = 2,
>> -    .minimum_version_id  = 1,
>> +    .version_id  = 3,
>> +    .minimum_version_id  = 2,
>
> Pick a new name for this ...
>
>> -static void ehci_class_init(ObjectClass *klass, void *data)
>> +static const VMStateDescription vmstate_ehci_pci = {
>> +    .name        = "ehci-pci",
>
> ... and keep using "ehci" here ...
>
>> +    .version_id  = 3,
>> +    .minimum_version_id  = 2,
>
> ... then you don't need to fiddle with the versions as the vmstate wire
> format doesn't change then.
>

Does that work considering you have turned one layer of VMSD into two?
Can it equivocate machines saved with the old all-in-one vmsd with
this new one that is structured in two layers?

Regards,
Peter

> cheers,
>   Gerd
>

Re: [Qemu-devel] [PATCH v1 3/8] usb/ehci: seperate out PCIisms

[Gerd Hoffmann]

  Hi,

>> ... then you don't need to fiddle with the versions as the vmstate wire
>> format doesn't change then.
>>
> 
> Does that work considering you have turned one layer of VMSD into two?
> Can it equivocate machines saved with the old all-in-one vmsd with
> this new one that is structured in two layers?

I'm pretty sure it does as only for top-level vmsd name and version are
stuffed into the wire bytestream.  For sub-vmsds only the actual fields
are written.  So moving stuff from the toplevel vmsd into another one
which then gets referenced via VMSTATE_STRUCT doesn't change the format.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v1 3/8] usb/ehci: seperate out PCIisms

[Peter Crosthwaite]

On Thu, Oct 25, 2012 at 10:57 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
>>> ... then you don't need to fiddle with the versions as the vmstate wire
>>> format doesn't change then.
>>>
>>
>> Does that work considering you have turned one layer of VMSD into two?
>> Can it equivocate machines saved with the old all-in-one vmsd with
>> this new one that is structured in two layers?
>
> I'm pretty sure it does as only for top-level vmsd name and version are
> stuffed into the wire bytestream.  For sub-vmsds only the actual fields
> are written.  So moving stuff from the toplevel vmsd into another one
> which then gets referenced via VMSTATE_STRUCT doesn't change the format.

Nice,

Will refactor as you proposed v2 and keep backwards compat.

Regards,
Peter

>
> cheers,
>   Gerd
>
>

[Qemu-devel] [PATCH v1 4/8] usb/ehci: Add usb-ehci-sysbus

[Peter Crosthwaite]

Add QOM device definition for sysbus attached EHCI.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/usb/hcd-ehci.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 52 insertions(+), 1 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 862564c..17482f7 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -35,6 +35,8 @@
 #include "trace.h"
 #include "dma.h"
 #include "sysemu.h"
+#include "hw/sysbus.h"
+#include "exec-memory.h"
 
 #define EHCI_DEBUG   0
 
@@ -446,7 +448,13 @@ struct EHCIState {
 };
 
 typedef struct EHCIItfState {
-    PCIDevice pcidev;
+    /* FIXME: Figure out a better way to share one Property[] array between two
+     * QOM types with different parents
+     */
+    union {
+        PCIDevice pcidev;
+        SysBusDevice busdev;
+    };
     struct EHCIState ehci;
 } EHCIItfState;
 
@@ -2544,6 +2552,7 @@ static const MemoryRegionOps ehci_mmio_port_ops = {
 };
 
 static int usb_ehci_pci_initfn(PCIDevice *dev);
+static int usb_ehci_sysbus_initfn(SysBusDevice *dev);
 
 static USBPortOps ehci_port_ops = {
     .attach = ehci_attach,
@@ -2699,6 +2708,32 @@ static TypeInfo ich9_ehci_info = {
     .class_init    = ich9_ehci_class_init,
 };
 
+static const VMStateDescription vmstate_ehci_sysbus = {
+    .name        = "ehci-sysbus",
+    .version_id  = 3,
+    .minimum_version_id  = 2,
+    .fields      = (VMStateField[]) {
+        VMSTATE_STRUCT(ehci, EHCIItfState, 3, vmstate_ehci, EHCIState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void ehci_sysbus_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+
+    k->init = usb_ehci_sysbus_initfn;
+    dc->vmsd = &vmstate_ehci_sysbus;
+    dc->props = ehci_properties;
+}
+
+static TypeInfo ehci_sysbus_info = {
+    .name          = "ehci-sysbus",
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(EHCIItfState),
+    .class_init    = ehci_sysbus_class_init,
+};
 
 static void usb_ehci_initfn(EHCIState *s, DeviceState *dev)
 {
@@ -2750,6 +2785,21 @@ static void usb_ehci_initfn(EHCIState *s, DeviceState *dev)
                                 &s->mem_ports);
 }
 
+static int usb_ehci_sysbus_initfn(SysBusDevice *dev)
+{
+    EHCIItfState *i = FROM_SYSBUS(EHCIItfState, dev);
+    EHCIState *s = &i->ehci;
+
+    s->dma = g_new(DMAContext, 1);
+    dma_context_init(s->dma, &address_space_memory, NULL, NULL, NULL);
+
+    usb_ehci_initfn(s, DEVICE(dev));
+    sysbus_init_irq(dev, &s->irq);
+    sysbus_init_mmio(dev, &s->mem);
+
+    return 0;
+}
+
 static int usb_ehci_pci_initfn(PCIDevice *dev)
 {
     EHCIItfState *i = DO_UPCAST(EHCIItfState, pcidev, dev);
@@ -2798,6 +2848,7 @@ static void ehci_register_types(void)
 {
     type_register_static(&ehci_pci_info);
     type_register_static(&ich9_ehci_info);
+    type_register_static(&ehci_sysbus_info);
 }
 
 type_init(ehci_register_types)
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH v1 4/8] usb/ehci: Add usb-ehci-sysbus

[Peter Maydell]

On 25 October 2012 10:47, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> +static int usb_ehci_sysbus_initfn(SysBusDevice *dev)
> +{
> +    EHCIItfState *i = FROM_SYSBUS(EHCIItfState, dev);
> +    EHCIState *s = &i->ehci;
> +
> +    s->dma = g_new(DMAContext, 1);
> +    dma_context_init(s->dma, &address_space_memory, NULL, NULL, NULL);

Assuming Avi doesn't reject it, better to use the dma_context_memory
I introduced in this patch (not yet applied to master):
  http://patchwork.ozlabs.org/patch/193550/

-- PMM

Re: [Qemu-devel] [PATCH v1 4/8] usb/ehci: Add usb-ehci-sysbus

[Avi Kivity]

On 10/25/2012 11:55 AM, Peter Maydell wrote:
> On 25 October 2012 10:47, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> +static int usb_ehci_sysbus_initfn(SysBusDevice *dev)
>> +{
>> +    EHCIItfState *i = FROM_SYSBUS(EHCIItfState, dev);
>> +    EHCIState *s = &i->ehci;
>> +
>> +    s->dma = g_new(DMAContext, 1);
>> +    dma_context_init(s->dma, &address_space_memory, NULL, NULL, NULL);
> 
> Assuming Avi doesn't reject it, better to use the dma_context_memory
> I introduced in this patch (not yet applied to master):
>   http://patchwork.ozlabs.org/patch/193550/
> 

Patch is good, sorry if you were expecting an explicit ack.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v1 4/8] usb/ehci: Add usb-ehci-sysbus

[Gerd Hoffmann]

>  typedef struct EHCIItfState {
> -    PCIDevice pcidev;
> +    /* FIXME: Figure out a better way to share one Property[] array between two
> +     * QOM types with different parents
> +     */
> +    union {
> +        PCIDevice pcidev;
> +        SysBusDevice busdev;
> +    };

Ah, I see where this hack comes from.  I don't think they should share
the properties in the first place ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v1 4/8] usb/ehci: Add usb-ehci-sysbus

[Peter Crosthwaite]

On Thu, Oct 25, 2012 at 10:10 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>  typedef struct EHCIItfState {
>> -    PCIDevice pcidev;
>> +    /* FIXME: Figure out a better way to share one Property[] array between two
>> +     * QOM types with different parents
>> +     */
>> +    union {
>> +        PCIDevice pcidev;
>> +        SysBusDevice busdev;
>> +    };
>
> Ah, I see where this hack comes from.  I don't think they should share
> the properties in the first place ...
>

Kind of annoying to not share them as it really is a copy paste job
with the exception of the wrapping struct type.

> cheers,
>   Gerd
>

[Qemu-devel] [PATCH v1 5/8] xilinx_zynq: add USB controllers

[Peter Crosthwaite]

Add the two usb controllers in Zynq.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/xilinx_zynq.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/hw/xilinx_zynq.c b/hw/xilinx_zynq.c
index c55dafb..ed6934f 100644
--- a/hw/xilinx_zynq.c
+++ b/hw/xilinx_zynq.c
@@ -77,6 +77,19 @@ static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq)
 
 }
 
+static inline void zynq_init_usb(uint32_t base_addr, qemu_irq irq)
+{
+    DeviceState *dev = qdev_create(NULL, "ehci-sysbus");
+    SysBusDevice *busdev;
+
+    qdev_prop_set_uint16(dev, "capabase", 0x100);
+    qdev_prop_set_uint32(dev, "opregbase", 0x140);
+    qdev_init_nofail(dev);
+    busdev = sysbus_from_qdev(dev);
+    sysbus_mmio_map(busdev, 0, base_addr);
+    sysbus_connect_irq(busdev, 0, irq);
+}
+
 static void zynq_init(QEMUMachineInitArgs *args)
 {
     ram_addr_t ram_size = args->ram_size;
@@ -150,6 +163,9 @@ static void zynq_init(QEMUMachineInitArgs *args)
     zynq_init_spi_flashes(0xE0006000, pic[58-IRQ_OFFSET]);
     zynq_init_spi_flashes(0xE0007000, pic[81-IRQ_OFFSET]);
 
+    zynq_init_usb(0xE0002000, pic[53-IRQ_OFFSET]);
+    zynq_init_usb(0xE0003000, pic[75-IRQ_OFFSET]);
+
     sysbus_create_simple("cadence_uart", 0xE0000000, pic[59-IRQ_OFFSET]);
     sysbus_create_simple("cadence_uart", 0xE0001000, pic[82-IRQ_OFFSET]);
 
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH v1 5/8] xilinx_zynq: add USB controllers

[Gerd Hoffmann]

> +static inline void zynq_init_usb(uint32_t base_addr, qemu_irq irq)
> +{
> +    DeviceState *dev = qdev_create(NULL, "ehci-sysbus");

I'd suggest to have a "ehci-sysbus-zynq" device instead which sets
capsbase & opregbase in ->init() ...

> +    qdev_prop_set_uint16(dev, "capabase", 0x100);
> +    qdev_prop_set_uint32(dev, "opregbase", 0x140);

... then drop these lines.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v1 5/8] xilinx_zynq: add USB controllers

[Peter Maydell]

On 25 October 2012 13:12, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> +static inline void zynq_init_usb(uint32_t base_addr, qemu_irq irq)
>> +{
>> +    DeviceState *dev = qdev_create(NULL, "ehci-sysbus");
>
> I'd suggest to have a "ehci-sysbus-zynq" device instead which sets
> capsbase & opregbase in ->init() ...
>
>> +    qdev_prop_set_uint16(dev, "capabase", 0x100);
>> +    qdev_prop_set_uint32(dev, "opregbase", 0x140);
>
> ... then drop these lines.

That sounds weird to me -- properties are exactly the mechanism
for having a device which is configurable. Why have two differently
named devices which only differ in the value of a configurable
property?

[I haven't looked at whether these specific properties make
sense or if there's some other higher-level-of-abstraction
knob that would be better to expose.)

-- PMM

Re: [Qemu-devel] [PATCH v1 5/8] xilinx_zynq: add USB controllers

[Peter Crosthwaite]

On Thu, Oct 25, 2012 at 10:16 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 25 October 2012 13:12, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>> +static inline void zynq_init_usb(uint32_t base_addr, qemu_irq irq)
>>> +{
>>> +    DeviceState *dev = qdev_create(NULL, "ehci-sysbus");
>>
>> I'd suggest to have a "ehci-sysbus-zynq" device instead which sets
>> capsbase & opregbase in ->init() ...
>>
>>> +    qdev_prop_set_uint16(dev, "capabase", 0x100);
>>> +    qdev_prop_set_uint32(dev, "opregbase", 0x140);
>>
>> ... then drop these lines.
>
> That sounds weird to me -- properties are exactly the mechanism
> for having a device which is configurable. Why have two differently
> named devices which only differ in the value of a configurable
> property?
>

Yes I agree. Creating a now QOM definition for every variant of a
device is tedious. EHCI provides a nice abstraction which should not
have awareness of its particular implementations.

The way I have done it here also maps to how it is handled in the
linux kernel as well. capabase and opregbase are left as parameters
and EHCI implementations wrap around and set them as needed.
hcd-ehci.c in the kernel has no awareness of zynq and I think the same
hold for hcd-ehci.c in QEMU.

> [I haven't looked at whether these specific properties make
> sense or if there's some other higher-level-of-abstraction
> knob that would be better to expose.)
>

Im interested to see what up with Tegra here. Might have a look
through the kernel drivers to see what kinda diffs there are from one
EHCI variant to the next tmrw.

Regards,
Peter

> -- PMM
>

Re: [Qemu-devel] [PATCH v1 5/8] xilinx_zynq: add USB controllers

[Gerd Hoffmann]

On 10/25/12 14:56, Peter Crosthwaite wrote:
> On Thu, Oct 25, 2012 at 10:16 PM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> On 25 October 2012 13:12, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>>> +static inline void zynq_init_usb(uint32_t base_addr, qemu_irq irq)
>>>> +{
>>>> +    DeviceState *dev = qdev_create(NULL, "ehci-sysbus");
>>>
>>> I'd suggest to have a "ehci-sysbus-zynq" device instead which sets
>>> capsbase & opregbase in ->init() ...
>>>
>>>> +    qdev_prop_set_uint16(dev, "capabase", 0x100);
>>>> +    qdev_prop_set_uint32(dev, "opregbase", 0x140);
>>>
>>> ... then drop these lines.
>>
>> That sounds weird to me -- properties are exactly the mechanism
>> for having a device which is configurable. Why have two differently
>> named devices which only differ in the value of a configurable
>> property?

> Yes I agree. Creating a now QOM definition for every variant of a
> device is tedious. EHCI provides a nice abstraction which should not
> have awareness of its particular implementations.

Maybe "zynq" is the wrong abstraction and this should be named by the
actual ehci chip implementation (which could be the same for a bunch of
sysbus boards).

But, yes, different chips should have different QOM definitions.  Like
we have a bunch of different uhci variants with a QOM definition for
each of them.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v1 5/8] xilinx_zynq: add USB controllers

[Peter Crosthwaite]

On Thu, Oct 25, 2012 at 11:14 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On 10/25/12 14:56, Peter Crosthwaite wrote:
>> On Thu, Oct 25, 2012 at 10:16 PM, Peter Maydell
>> <peter.maydell@linaro.org> wrote:
>>> On 25 October 2012 13:12, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>>>> +static inline void zynq_init_usb(uint32_t base_addr, qemu_irq irq)
>>>>> +{
>>>>> +    DeviceState *dev = qdev_create(NULL, "ehci-sysbus");
>>>>
>>>> I'd suggest to have a "ehci-sysbus-zynq" device instead which sets
>>>> capsbase & opregbase in ->init() ...
>>>>
>>>>> +    qdev_prop_set_uint16(dev, "capabase", 0x100);
>>>>> +    qdev_prop_set_uint32(dev, "opregbase", 0x140);
>>>>
>>>> ... then drop these lines.
>>>
>>> That sounds weird to me -- properties are exactly the mechanism
>>> for having a device which is configurable. Why have two differently
>>> named devices which only differ in the value of a configurable
>>> property?
>
>> Yes I agree. Creating a now QOM definition for every variant of a
>> device is tedious. EHCI provides a nice abstraction which should not
>> have awareness of its particular implementations.
>
> Maybe "zynq" is the wrong abstraction and this should be named by the
> actual ehci chip implementation (which could be the same for a bunch of
> sysbus boards).
>
> But, yes, different chips should have different QOM definitions.  Like
> we have a bunch of different uhci variants with a QOM definition for
> each of them.
>

Can we at least take a data driven approach to this? Get the device
names and their varying parameters into a big table up top and they
just create N device defs from it? hw/m25p80 is one example of a
device that does something similar although the variation is selected
using a single qdev prop rather than the QOM names.

> cheers,
>   Gerd
>
>

Re: [Qemu-devel] [PATCH v1 5/8] xilinx_zynq: add USB controllers

[Gerd Hoffmann]

  Hi,

>>> Yes I agree. Creating a now QOM definition for every variant of a
>>> device is tedious. EHCI provides a nice abstraction which should not
>>> have awareness of its particular implementations.
>>
>> Maybe "zynq" is the wrong abstraction and this should be named by the
>> actual ehci chip implementation (which could be the same for a bunch of
>> sysbus boards).
>>
>> But, yes, different chips should have different QOM definitions.  Like
>> we have a bunch of different uhci variants with a QOM definition for
>> each of them.
> 
> Can we at least take a data driven approach to this?

Ahem.  Well.  I'd love to.  It even used to be that way, before QOM.
uhci had a simple PCIDeviceInfo table, with an entry for each variant.
QOM turned that a bunch of class_init functions ...

Looking a bit closer (at include/qemu/object.h): seems we can pass
additional data to TypeInfo->class_init via TypeInfo->class_data.  That
should help here.

I'll go try that to simplify uhci ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v1 5/8] xilinx_zynq: add USB controllers

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 84 bytes --]

  Hi,

> I'll go try that to simplify uhci ...

Seems to work ...

cheers,
  Gerd



[-- Attachment #2: fix --]
[-- Type: text/plain, Size: 2663 bytes --]

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index b6b972f..64442a4 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -88,6 +88,13 @@ enum {
 typedef struct UHCIState UHCIState;
 typedef struct UHCIAsync UHCIAsync;
 typedef struct UHCIQueue UHCIQueue;
+typedef struct UHCIInfo UHCIInfo;
+
+struct UHCIInfo {
+    uint16_t vendor_id;
+    uint16_t device_id;
+    uint8_t  revision;
+};
 
 /* 
  * Pending async transaction.
@@ -1293,17 +1300,18 @@ static Property uhci_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
-static void piix3_uhci_class_init(ObjectClass *klass, void *data)
+static void uhci_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+    UHCIInfo *info = data;
 
     k->init = usb_uhci_common_initfn;
     k->exit = usb_uhci_exit;
-    k->vendor_id = PCI_VENDOR_ID_INTEL;
-    k->device_id = PCI_DEVICE_ID_INTEL_82371SB_2;
-    k->revision = 0x01;
-    k->class_id = PCI_CLASS_SERIAL_USB;
+    k->vendor_id = info->vendor_id;
+    k->device_id = info->device_id;
+    k->revision  = info->revision;
+    k->class_id  = PCI_CLASS_SERIAL_USB;
     dc->vmsd = &vmstate_uhci;
     dc->props = uhci_properties;
 }
@@ -1312,29 +1320,24 @@ static TypeInfo piix3_uhci_info = {
     .name          = "piix3-usb-uhci",
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(UHCIState),
-    .class_init    = piix3_uhci_class_init,
+    .class_init    = uhci_class_init,
+    .class_data    = (UHCIInfo[]) { {
+            .vendor_id = PCI_VENDOR_ID_INTEL,
+            .device_id = PCI_DEVICE_ID_INTEL_82371SB_2,
+            .revision  = 0x01,
+     } },
 };
 
-static void piix4_uhci_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-
-    k->init = usb_uhci_common_initfn;
-    k->exit = usb_uhci_exit;
-    k->vendor_id = PCI_VENDOR_ID_INTEL;
-    k->device_id = PCI_DEVICE_ID_INTEL_82371AB_2;
-    k->revision = 0x01;
-    k->class_id = PCI_CLASS_SERIAL_USB;
-    dc->vmsd = &vmstate_uhci;
-    dc->props = uhci_properties;
-}
-
 static TypeInfo piix4_uhci_info = {
     .name          = "piix4-usb-uhci",
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(UHCIState),
-    .class_init    = piix4_uhci_class_init,
+    .class_init    = uhci_class_init,
+    .class_data    = (UHCIInfo[]) { {
+            .vendor_id = PCI_VENDOR_ID_INTEL,
+            .device_id = PCI_DEVICE_ID_INTEL_82371AB_2,
+            .revision  = 0x01,
+     } },
 };
 
 static void vt82c686b_uhci_class_init(ObjectClass *klass, void *data)

Re: [Qemu-devel] [PATCH v1 5/8] xilinx_zynq: add USB controllers

[Peter Crosthwaite]

Copying patch inline to make some comments on it

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index b6b972f..64442a4 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -88,6 +88,13 @@ enum {
 typedef struct UHCIState UHCIState;
 typedef struct UHCIAsync UHCIAsync;
 typedef struct UHCIQueue UHCIQueue;
+typedef struct UHCIInfo UHCIInfo;
+
+struct UHCIInfo {
+    uint16_t vendor_id;
+    uint16_t device_id;
+    uint8_t  revision;
+};

 /*
  * Pending async transaction.
@@ -1293,17 +1300,18 @@ static Property uhci_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };

-static void piix3_uhci_class_init(ObjectClass *klass, void *data)
+static void uhci_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+    UHCIInfo *info = data;

     k->init = usb_uhci_common_initfn;
     k->exit = usb_uhci_exit;
-    k->vendor_id = PCI_VENDOR_ID_INTEL;
-    k->device_id = PCI_DEVICE_ID_INTEL_82371SB_2;
-    k->revision = 0x01;
-    k->class_id = PCI_CLASS_SERIAL_USB;
+    k->vendor_id = info->vendor_id;
+    k->device_id = info->device_id;
+    k->revision  = info->revision;
+    k->class_id  = PCI_CLASS_SERIAL_USB;
     dc->vmsd = &vmstate_uhci;
     dc->props = uhci_properties;
 }
@@ -1312,29 +1320,24 @@ static TypeInfo piix3_uhci_info = {
     .name          = "piix3-usb-uhci",
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(UHCIState),
-    .class_init    = piix3_uhci_class_init,
+    .class_init    = uhci_class_init,
+    .class_data    = (UHCIInfo[]) { {
+            .vendor_id = PCI_VENDOR_ID_INTEL,
+            .device_id = PCI_DEVICE_ID_INTEL_82371SB_2,
+            .revision  = 0x01,
+     } },
 };

-static void piix4_uhci_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-
-    k->init = usb_uhci_common_initfn;
-    k->exit = usb_uhci_exit;
-    k->vendor_id = PCI_VENDOR_ID_INTEL;
-    k->device_id = PCI_DEVICE_ID_INTEL_82371AB_2;
-    k->revision = 0x01;
-    k->class_id = PCI_CLASS_SERIAL_USB;
-    dc->vmsd = &vmstate_uhci;
-    dc->props = uhci_properties;
-}
-
 static TypeInfo piix4_uhci_info = {
     .name          = "piix4-usb-uhci",
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(UHCIState),
-    .class_init    = piix4_uhci_class_init,
+    .class_init    = uhci_class_init,
+    .class_data    = (UHCIInfo[]) { {
+            .vendor_id = PCI_VENDOR_ID_INTEL,
+            .device_id = PCI_DEVICE_ID_INTEL_82371AB_2,
+            .revision  = 0x01,
+     } },
 };

 static void vt82c686b_uhci_class_init(ObjectClass *klass, void *data)

On Fri, Oct 26, 2012 at 12:10 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
>> I'll go try that to simplify uhci ...
>
> Seems to work ...
>
> cheers,
>   Gerd
>
>

Re: [Qemu-devel] [PATCH v1 5/8] xilinx_zynq: add USB controllers

[Peter Crosthwaite]

On Fri, Oct 26, 2012 at 9:54 AM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Copying patch inline to make some comments on it
>
> diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
> index b6b972f..64442a4 100644
> --- a/hw/usb/hcd-uhci.c
> +++ b/hw/usb/hcd-uhci.c
> @@ -88,6 +88,13 @@ enum {
>  typedef struct UHCIState UHCIState;
>  typedef struct UHCIAsync UHCIAsync;
>  typedef struct UHCIQueue UHCIQueue;
> +typedef struct UHCIInfo UHCIInfo;
> +
> +struct UHCIInfo {
> +    uint16_t vendor_id;
> +    uint16_t device_id;
> +    uint8_t  revision;
> +};
>

Theres nothing PCI specific here. Should this be defined somewhere in
the PCI layers so PCI classes. First thing I had to do for EHCI
equivalent was copy paste this s/UHCI/EHCI with no other change.

>  /*
>   * Pending async transaction.
> @@ -1293,17 +1300,18 @@ static Property uhci_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> -static void piix3_uhci_class_init(ObjectClass *klass, void *data)
> +static void uhci_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    UHCIInfo *info = data;
>
>      k->init = usb_uhci_common_initfn;
>      k->exit = usb_uhci_exit;
> -    k->vendor_id = PCI_VENDOR_ID_INTEL;
> -    k->device_id = PCI_DEVICE_ID_INTEL_82371SB_2;
> -    k->revision = 0x01;
> -    k->class_id = PCI_CLASS_SERIAL_USB;
> +    k->vendor_id = info->vendor_id;
> +    k->device_id = info->device_id;
> +    k->revision  = info->revision;
> +    k->class_id  = PCI_CLASS_SERIAL_USB;
>      dc->vmsd = &vmstate_uhci;
>      dc->props = uhci_properties;
>  }
> @@ -1312,29 +1320,24 @@ static TypeInfo piix3_uhci_info = {
>      .name          = "piix3-usb-uhci",
>      .parent        = TYPE_PCI_DEVICE,
>      .instance_size = sizeof(UHCIState),
> -    .class_init    = piix3_uhci_class_init,
> +    .class_init    = uhci_class_init,

Therese three elements (parent, instance_size, class_init) are
repeated from one definition to the next. This will get tedious when
we eventually come to make the same fix for some of the other devices
that have large numbers of variants (pflash_cfi0x and m25p80 being
some of the angrier ones).

Regards,
Peter

> +    .class_data    = (UHCIInfo[]) { {
> +            .vendor_id = PCI_VENDOR_ID_INTEL,
> +            .device_id = PCI_DEVICE_ID_INTEL_82371SB_2,
> +            .revision  = 0x01,
> +     } },
>  };
>
> -static void piix4_uhci_class_init(ObjectClass *klass, void *data)
> -{
> -    DeviceClass *dc = DEVICE_CLASS(klass);
> -    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> -
> -    k->init = usb_uhci_common_initfn;
> -    k->exit = usb_uhci_exit;
> -    k->vendor_id = PCI_VENDOR_ID_INTEL;
> -    k->device_id = PCI_DEVICE_ID_INTEL_82371AB_2;
> -    k->revision = 0x01;
> -    k->class_id = PCI_CLASS_SERIAL_USB;
> -    dc->vmsd = &vmstate_uhci;
> -    dc->props = uhci_properties;
> -}
> -
>  static TypeInfo piix4_uhci_info = {
>      .name          = "piix4-usb-uhci",
>      .parent        = TYPE_PCI_DEVICE,
>      .instance_size = sizeof(UHCIState),
> -    .class_init    = piix4_uhci_class_init,
> +    .class_init    = uhci_class_init,
> +    .class_data    = (UHCIInfo[]) { {
> +            .vendor_id = PCI_VENDOR_ID_INTEL,
> +            .device_id = PCI_DEVICE_ID_INTEL_82371AB_2,
> +            .revision  = 0x01,
> +     } },
>  };
>
>  static void vt82c686b_uhci_class_init(ObjectClass *klass, void *data)
>
> On Fri, Oct 26, 2012 at 12:10 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>   Hi,
>>
>>> I'll go try that to simplify uhci ...
>>
>> Seems to work ...
>>
>> cheers,
>>   Gerd
>>
>>

Re: [Qemu-devel] [PATCH v1 5/8] xilinx_zynq: add USB controllers

[Gerd Hoffmann]

  Hi,

>> @@ -1312,29 +1320,24 @@ static TypeInfo piix3_uhci_info = {
>>      .name          = "piix3-usb-uhci",
>>      .parent        = TYPE_PCI_DEVICE,
>>      .instance_size = sizeof(UHCIState),
>> -    .class_init    = piix3_uhci_class_init,
>> +    .class_init    = uhci_class_init,
> 
> Therese three elements (parent, instance_size, class_init) are
> repeated from one definition to the next. This will get tedious when
> we eventually come to make the same fix for some of the other devices
> that have large numbers of variants (pflash_cfi0x and m25p80 being
> some of the angrier ones).

I think we can also create TypeInfo at runtime (uhci_register_types in
the uhci case).  We'll just have to stick the name into UHCIInfo, build
a static uhciinfo array, then generate & register TypeInfo from that.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v1 5/8] xilinx_zynq: add USB controllers

[Peter Crosthwaite]

Next version up on list, Ive changed the approach a bit.

On Fri, Oct 26, 2012 at 4:49 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
>>> @@ -1312,29 +1320,24 @@ static TypeInfo piix3_uhci_info = {
>>>      .name          = "piix3-usb-uhci",
>>>      .parent        = TYPE_PCI_DEVICE,
>>>      .instance_size = sizeof(UHCIState),
>>> -    .class_init    = piix3_uhci_class_init,
>>> +    .class_init    = uhci_class_init,
>>
>> Therese three elements (parent, instance_size, class_init) are
>> repeated from one definition to the next. This will get tedious when
>> we eventually come to make the same fix for some of the other devices
>> that have large numbers of variants (pflash_cfi0x and m25p80 being
>> some of the angrier ones).
>
> I think we can also create TypeInfo at runtime (uhci_register_types in
> the uhci case).
Im comming I think this approach is ok for a small number of variants,
but for the large ones creating type_infos dynamically may be the way
to go.

> We'll just have to stick the name into UHCIInfo, build
> a static uhciinfo array, then generate & register TypeInfo from that.
>
Instead of create a new struct definition (e.g. UHCIInfo), I just used
the parent class definition (you would use PCIDeviceClass in the UHCI
case). EHCI was a little more complex cos of the multiple parents, but
UHCI you should just be able to pass a partially populated
PCIDeviceClass to class_data that class_init can pick fields out of.

> cheers,
>   Gerd
>
>

[Qemu-devel] [PATCH v1 6/8] usb/ehci: Guard definition of EHCI_DEBUG

[Peter Crosthwaite]

Guard against re-definition of EHCI_DEBUG. Allows for turning on of debug info
from configure (using --qemu-extra-cflags="-DEHCI_DEBUG=1") rather than source
code hacking.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/usb/hcd-ehci.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 17482f7..394feca 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -38,7 +38,9 @@
 #include "hw/sysbus.h"
 #include "exec-memory.h"
 
+#ifndef EHCI_DEBUG
 #define EHCI_DEBUG   0
+#endif
 
 #if EHCI_DEBUG
 #define DPRINTF printf
-- 
1.7.0.4

[Qemu-devel] [PATCH v1 7/8] usb/ehci: Debug mode compile fixes

[Peter Crosthwaite]

A few debug messages in EHCI must have missed out on updates during incremental
developments. Fixed.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/usb/hcd-ehci.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 394feca..78f9dfd 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1489,8 +1489,8 @@ static void ehci_execute_complete(EHCIQueue *q)
     assert(p->async == EHCI_ASYNC_INITIALIZED ||
            p->async == EHCI_ASYNC_FINISHED);
 
-    DPRINTF("execute_complete: qhaddr 0x%x, next %x, qtdaddr 0x%x, status %d\n",
-            q->qhaddr, q->qh.next, q->qtdaddr, q->usb_status);
+    DPRINTF("execute_complete: qhaddr 0x%x, next %x, qtdaddr 0x%x\n",
+            q->qhaddr, q->qh.next, q->qtdaddr);
 
     if (p->usb_status < 0) {
         switch (p->usb_status) {
@@ -1549,6 +1549,7 @@ static int ehci_execute(EHCIPacket *p, const char *action)
     USBEndpoint *ep;
     int ret;
     int endp;
+    EHCIQueue *q = p->queue;
 
     assert(p->async == EHCI_ASYNC_NONE ||
            p->async == EHCI_ASYNC_INITIALIZED);
@@ -1560,7 +1561,7 @@ static int ehci_execute(EHCIPacket *p, const char *action)
 
     p->tbytes = (p->qtd.token & QTD_TOKEN_TBYTES_MASK) >> QTD_TOKEN_TBYTES_SH;
     if (p->tbytes > BUFF_SIZE) {
-        ehci_trace_guest_bug(p->queue->ehci,
+        ehci_trace_guest_bug(q->ehci,
                              "guest requested more bytes than allowed");
         return USB_RET_PROCERR;
     }
@@ -1581,8 +1582,8 @@ static int ehci_execute(EHCIPacket *p, const char *action)
         break;
     }
 
-    endp = get_field(p->queue->qh.epchar, QH_EPCHAR_EP);
-    ep = usb_ep_get(p->queue->dev, p->pid, endp);
+    endp = get_field(q->qh.epchar, QH_EPCHAR_EP);
+    ep = usb_ep_get(q->dev, p->pid, endp);
 
     if (p->async == EHCI_ASYNC_NONE) {
         if (ehci_init_transfer(p) != 0) {
@@ -1594,12 +1595,10 @@ static int ehci_execute(EHCIPacket *p, const char *action)
         p->async = EHCI_ASYNC_INITIALIZED;
     }
 
-    trace_usb_ehci_packet_action(p->queue, p, action);
-    ret = usb_handle_packet(p->queue->dev, &p->packet);
-    DPRINTF("submit: qh %x next %x qtd %x pid %x len %zd "
-            "(total %d) endp %x ret %d\n",
-            q->qhaddr, q->qh.next, q->qtdaddr, q->pid,
-            q->packet.iov.size, q->tbytes, endp, ret);
+    trace_usb_ehci_packet_action(q, p, action);
+    ret = usb_handle_packet(q->dev, &p->packet);
+    DPRINTF("submit: qh %x next %x qtd %x pid %x (total %d) endp %x ret %d\n",
+            q->qhaddr, q->qh.next, q->qtdaddr, p->pid, p->tbytes, endp, ret);
 
     if (ret > BUFF_SIZE) {
         fprintf(stderr, "ret from usb_handle_packet > BUFF_SIZE\n");
-- 
1.7.0.4

[Qemu-devel] [PATCH v1 8/8] usb/ehci: Put RAM in undefined MMIO regions

[Peter Crosthwaite]

Just put RAM regions in the unimplemented spaces in the MMIO region. These
regions have undefined behaviour, but this at least stops QEMU from segfaulting
when the guest bangs on these registers (and sucessfully fakes reading and
writing the registers with no side effects).

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/usb/hcd-ehci.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 78f9dfd..b6418bc 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -396,6 +396,8 @@ struct EHCIState {
     MemoryRegion mem_caps;
     MemoryRegion mem_opreg;
     MemoryRegion mem_ports;
+    MemoryRegion mem_other_low;
+    MemoryRegion mem_other_high;
     int companion_count;
 
     /* properties */
@@ -2773,17 +2775,27 @@ static void usb_ehci_initfn(EHCIState *s, DeviceState *dev)
     qemu_add_vm_change_state_handler(usb_ehci_vm_state_change, s);
 
     memory_region_init(&s->mem, "ehci", MMIO_SIZE);
+    if (s->capabase) {
+        memory_region_init_ram(&s->mem_other_low, "other-low", s->capabase);
+    }
     memory_region_init_io(&s->mem_caps, &ehci_mmio_caps_ops, s,
                           "capabilities", s->opregbase);
     memory_region_init_io(&s->mem_opreg, &ehci_mmio_opreg_ops, s,
                           "operational", PORTSC_BEGIN);
     memory_region_init_io(&s->mem_ports, &ehci_mmio_port_ops, s,
                           "ports", PORTSC_END - PORTSC_BEGIN);
+    memory_region_init_ram(&s->mem_other_high, "other-high", MMIO_SIZE -
+                           s->opregbase - (PORTSC_END - PORTSC_BEGIN));
 
+    if (s->capabase) {
+        memory_region_add_subregion(&s->mem, 0, &s->mem_other_low);
+    }
     memory_region_add_subregion(&s->mem, s->capabase, &s->mem_caps);
     memory_region_add_subregion(&s->mem, s->opregbase, &s->mem_opreg);
     memory_region_add_subregion(&s->mem, s->opregbase + PORTSC_BEGIN,
                                 &s->mem_ports);
+    memory_region_add_subregion(&s->mem, s->opregbase + PORTSC_END,
+                                &s->mem_other_high);
 }
 
 static int usb_ehci_sysbus_initfn(SysBusDevice *dev)
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH v1 8/8] usb/ehci: Put RAM in undefined MMIO regions

[Gerd Hoffmann]

On 10/25/12 11:47, Peter Crosthwaite wrote:
> Just put RAM regions in the unimplemented spaces in the MMIO region. These
> regions have undefined behaviour, but this at least stops QEMU from segfaulting
> when the guest bangs on these registers (and sucessfully fakes reading and
> writing the registers with no side effects).

Should not be needed, memory api should deal with that properly.
Something is fishy somewhere.  Maybe the dmacontext thing Peter Maydell
noted for patch 5.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v1 8/8] usb/ehci: Put RAM in undefined MMIO regions

[Peter Crosthwaite]

On Thu, Oct 25, 2012 at 10:19 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On 10/25/12 11:47, Peter Crosthwaite wrote:
>> Just put RAM regions in the unimplemented spaces in the MMIO region. These
>> regions have undefined behaviour, but this at least stops QEMU from segfaulting
>> when the guest bangs on these registers (and sucessfully fakes reading and
>> writing the registers with no side effects).
>
> Should not be needed, memory api should deal with that properly.

CC Avi,

Whats going on here is there is a container of size 0x1000 created
with memory_region_init() and a handful of small subregions are
populated. the container is then mapped to a 0x1000 size region of the
system memory. What is supposed to happen when the guest access a
region in the container for which no subregion has been added? For me
it was a segfault, so i needed this patch for guest to proceed past
accesses these undefined regions.

> Something is fishy somewhere.  Maybe the dmacontext thing Peter Maydell
> noted for patch 5.
>

I think thats a separate issue. This is about the guest accessing the
EHCI MMIO region not DMA.

My implementation of P5 is functionality equivalent to Peters
proposal. Just Peters idea will save me two lines of code and a memory
leak :)

Regards,
Peter

> cheers,
>   Gerd
>
>

Re: [Qemu-devel] [PATCH v1 8/8] usb/ehci: Put RAM in undefined MMIO regions

[Peter Maydell]

On 25 October 2012 14:03, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Thu, Oct 25, 2012 at 10:19 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> On 10/25/12 11:47, Peter Crosthwaite wrote:
>>> Just put RAM regions in the unimplemented spaces in the MMIO region. These
>>> regions have undefined behaviour, but this at least stops QEMU from segfaulting
>>> when the guest bangs on these registers (and sucessfully fakes reading and
>>> writing the registers with no side effects).
>>
>> Should not be needed, memory api should deal with that properly.
>
> CC Avi,
>
> Whats going on here is there is a container of size 0x1000 created
> with memory_region_init() and a handful of small subregions are
> populated. the container is then mapped to a 0x1000 size region of the
> system memory. What is supposed to happen when the guest access a
> region in the container for which no subregion has been added? For me
> it was a segfault, so i needed this patch for guest to proceed past
> accesses these undefined regions.

 (1) what does the hardware do? If the hardware device has (the
 equivalent of) an N bit address bus and treats accesses to unused
 addresses within the memory range defined by that bus width as RAZ/WI
 then that's what we should be modelling.
 If the hardware has real registers in this range we should also be
 modelling them (possibly as RAZ/WI with a LOG_UNIMP diagnostic).

 (2) what should the memory system do for accesses where there is
 no memory region? This is really system specific as it depends
 what the bus fabric does. For ARM the usual thing would be to
 generate a decode error response which will result in the guest
 CPU taking a data abort or prefetch abort. I don't think our
 memory system currently has any way of saying "for this access
 generate an exception"...

 (3) I don't think "qemu segfaults" is a good response to bad
 guest behaviour so that needs fixing somehow even if we can't
 model what the h/w does :-)

-- PMM

Re: [Qemu-devel] [PATCH v1 8/8] usb/ehci: Put RAM in undefined MMIO regions

[Avi Kivity]

On 10/25/2012 03:12 PM, Peter Maydell wrote:
>  (2) what should the memory system do for accesses where there is
>  no memory region? This is really system specific as it depends
>  what the bus fabric does. For ARM the usual thing would be to
>  generate a decode error response which will result in the guest
>  CPU taking a data abort or prefetch abort. I don't think our
>  memory system currently has any way of saying "for this access
>  generate an exception"...
> 

You could easily have the top-level container have ->ops that generate
an exception.


>  (3) I don't think "qemu segfaults" is a good response to bad
>  guest behaviour so that needs fixing somehow even if we can't
>  model what the h/w does :-)

Right, a stack trace (or a patch) would be appreciated.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v1 8/8] usb/ehci: Put RAM in undefined MMIO regions

[Peter Maydell]

On 25 October 2012 14:21, Avi Kivity <avi@redhat.com> wrote:
> On 10/25/2012 03:12 PM, Peter Maydell wrote:
>>  (2) what should the memory system do for accesses where there is
>>  no memory region? This is really system specific as it depends
>>  what the bus fabric does. For ARM the usual thing would be to
>>  generate a decode error response which will result in the guest
>>  CPU taking a data abort or prefetch abort. I don't think our
>>  memory system currently has any way of saying "for this access
>>  generate an exception"...
>>
>
> You could easily have the top-level container have ->ops that generate
> an exception.

Ah, yes, there's an 'accepts' callback. (That's kind of awkward
as an API because it means your decode logic gets spread between
read, write and accept: there are some devices where it would be
nice to have the 'default:' case of the address switch say "unknown
offset, raise decode error". If the read callback took a uint64_t*
rather than returning the read data, we could make both read and
write return a success/decode-error type of status result.)

-- PMM

Re: [Qemu-devel] [PATCH v1 8/8] usb/ehci: Put RAM in undefined MMIO regions

[Avi Kivity]

On 10/25/2012 03:28 PM, Peter Maydell wrote:
> On 25 October 2012 14:21, Avi Kivity <avi@redhat.com> wrote:
>> On 10/25/2012 03:12 PM, Peter Maydell wrote:
>>>  (2) what should the memory system do for accesses where there is
>>>  no memory region? This is really system specific as it depends
>>>  what the bus fabric does. For ARM the usual thing would be to
>>>  generate a decode error response which will result in the guest
>>>  CPU taking a data abort or prefetch abort. I don't think our
>>>  memory system currently has any way of saying "for this access
>>>  generate an exception"...
>>>
>>
>> You could easily have the top-level container have ->ops that generate
>> an exception.
> 
> Ah, yes, there's an 'accepts' callback. (That's kind of awkward
> as an API because it means your decode logic gets spread between
> read, write and accept: there are some devices where it would be
> nice to have the 'default:' case of the address switch say "unknown
> offset, raise decode error". If the read callback took a uint64_t*
> rather than returning the read data, we could make both read and
> write return a success/decode-error type of status result.)

I actually forgot about ->accepts().  But it isn't needed for this use
case, just have the lowest priority region (the container) implement
->read/write that generate the exception.  If some other region decodes,
the container region will be ignored, if nothing decodes, you get your
exception.

wrt decode duplication, I've been thinking of a single ->service()
callback that accepts a Transaction argument, including all the details
(offset, data, and direction).  We may also do something like
MemoryRegionPortio, but not hacky, that lets you have a MemoryRegion for
each register.  That means that instead of writing two large functions
that duplicates the decode, you write one function per register, and
switch on transfer direction.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v1 8/8] usb/ehci: Put RAM in undefined MMIO regions

[Peter Maydell]

On 25 October 2012 14:41, Avi Kivity <avi@redhat.com> wrote:
> On 10/25/2012 03:28 PM, Peter Maydell wrote:
>> On 25 October 2012 14:21, Avi Kivity <avi@redhat.com> wrote:
>>> You could easily have the top-level container have ->ops that generate
>>> an exception.
>>
>> Ah, yes, there's an 'accepts' callback. (That's kind of awkward
>> as an API because it means your decode logic gets spread between
>> read, write and accept: there are some devices where it would be
>> nice to have the 'default:' case of the address switch say "unknown
>> offset, raise decode error". If the read callback took a uint64_t*
>> rather than returning the read data, we could make both read and
>> write return a success/decode-error type of status result.)
>
> I actually forgot about ->accepts().  But it isn't needed for this use
> case, just have the lowest priority region (the container) implement
> ->read/write that generate the exception

I don't understand this -- read/write don't have any way of saying
"please generate an exception". The only thing I can see in the
API that does that is returning false from accepts().

> wrt decode duplication, I've been thinking of a single ->service()
> callback that accepts a Transaction argument, including all the details
> (offset, data, and direction).

If we do this we should make sure that the Transaction allows us to
include CPU-architecture dependent info -- for ARM we would want to
model transaction attributes like 'secure/nonsecure', 'privileged/nonpriv',
'instruction/data', etc. You also want to include in the transaction
attributes who the master end of this transaction is (so a slave
can distinguish accesses from a particular CPU core in a cluster,
for instance). This would allow us to remove some of the current
nasty hacks where devices reach into the CPUArchState to  retrieve
info that should ideally be modelled as part of the bus transaction.

-- PMM

Re: [Qemu-devel] [PATCH v1 8/8] usb/ehci: Put RAM in undefined MMIO regions

[Avi Kivity]

On 10/25/2012 03:50 PM, Peter Maydell wrote:
> On 25 October 2012 14:41, Avi Kivity <avi@redhat.com> wrote:
>> On 10/25/2012 03:28 PM, Peter Maydell wrote:
>>> On 25 October 2012 14:21, Avi Kivity <avi@redhat.com> wrote:
>>>> You could easily have the top-level container have ->ops that generate
>>>> an exception.
>>>
>>> Ah, yes, there's an 'accepts' callback. (That's kind of awkward
>>> as an API because it means your decode logic gets spread between
>>> read, write and accept: there are some devices where it would be
>>> nice to have the 'default:' case of the address switch say "unknown
>>> offset, raise decode error". If the read callback took a uint64_t*
>>> rather than returning the read data, we could make both read and
>>> write return a success/decode-error type of status result.)
>>
>> I actually forgot about ->accepts().  But it isn't needed for this use
>> case, just have the lowest priority region (the container) implement
>> ->read/write that generate the exception
> 
> I don't understand this -- read/write don't have any way of saying
> "please generate an exception". The only thing I can see in the
> API that does that is returning false from accepts().

read/write can call anything.  So if the SoC code installs the lowest
region, it has access to whatever mechanisms generate the exceptions.

(it may not have access to CPUState though).

> 
>> wrt decode duplication, I've been thinking of a single ->service()
>> callback that accepts a Transaction argument, including all the details
>> (offset, data, and direction).
> 
> If we do this we should make sure that the Transaction allows us to
> include CPU-architecture dependent info -- for ARM we would want to
> model transaction attributes like 'secure/nonsecure', 'privileged/nonpriv',
> 'instruction/data', etc. You also want to include in the transaction
> attributes who the master end of this transaction is (so a slave
> can distinguish accesses from a particular CPU core in a cluster,
> for instance). This would allow us to remove some of the current
> nasty hacks where devices reach into the CPUArchState to  retrieve
> info that should ideally be modelled as part of the bus transaction.

Sounds like good arguments for another sweep.



-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v1 8/8] usb/ehci: Put RAM in undefined MMIO regions

[Peter Crosthwaite]

On Thu, Oct 25, 2012 at 11:50 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 25 October 2012 14:41, Avi Kivity <avi@redhat.com> wrote:
>> On 10/25/2012 03:28 PM, Peter Maydell wrote:
>>> On 25 October 2012 14:21, Avi Kivity <avi@redhat.com> wrote:
>>>> You could easily have the top-level container have ->ops that generate
>>>> an exception.
>>>
>>> Ah, yes, there's an 'accepts' callback. (That's kind of awkward
>>> as an API because it means your decode logic gets spread between
>>> read, write and accept: there are some devices where it would be
>>> nice to have the 'default:' case of the address switch say "unknown
>>> offset, raise decode error". If the read callback took a uint64_t*
>>> rather than returning the read data, we could make both read and
>>> write return a success/decode-error type of status result.)
>>
>> I actually forgot about ->accepts().  But it isn't needed for this use
>> case, just have the lowest priority region (the container) implement
>> ->read/write that generate the exception
>
> I don't understand this -- read/write don't have any way of saying
> "please generate an exception". The only thing I can see in the
> API that does that is returning false from accepts().
>
>> wrt decode duplication, I've been thinking of a single ->service()
>> callback that accepts a Transaction argument, including all the details
>> (offset, data, and direction).
>
> If we do this we should make sure that the Transaction allows us to
> include CPU-architecture dependent info -- for ARM we would want to
> model transaction attributes like 'secure/nonsecure', 'privileged/nonpriv',
> 'instruction/data', etc.

If we want these features shouldnt we just bite the bullet and
impelement AXI with its own set of QOM definitions? Axi slaves then
inherit from TYPE_AXI_SLAVE. If we want ARM specific features then we
should replace SYSBUS with an ARM bus that does these things rather
than push them up to the memory API.

Add to that ARM bus feature list exclusive/non-exclusive as well.

You also want to include in the transaction
> attributes who the master end of this transaction is (so a slave
> can distinguish accesses from a particular CPU core in a cluster,
> for instance). This would allow us to remove some of the current
> nasty hacks where devices reach into the CPUArchState to  retrieve
> info that should ideally be modelled as part of the bus transaction.
>
> -- PMM
>

Re: [Qemu-devel] [PATCH v1 8/8] usb/ehci: Put RAM in undefined MMIO regions

[Peter Maydell]

On 25 October 2012 14:59, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Thu, Oct 25, 2012 at 11:50 PM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> On 25 October 2012 14:41, Avi Kivity <avi@redhat.com> wrote:
>>> wrt decode duplication, I've been thinking of a single ->service()
>>> callback that accepts a Transaction argument, including all the details
>>> (offset, data, and direction).
>>
>> If we do this we should make sure that the Transaction allows us to
>> include CPU-architecture dependent info -- for ARM we would want to
>> model transaction attributes like 'secure/nonsecure', 'privileged/nonpriv',
>> 'instruction/data', etc.
>
> If we want these features shouldnt we just bite the bullet and
> impelement AXI with its own set of QOM definitions? Axi slaves then
> inherit from TYPE_AXI_SLAVE. If we want ARM specific features then we
> should replace SYSBUS with an ARM bus that does these things rather
> than push them up to the memory API.

The memory API should provide the basic feature set that you can
use to implement system specific buses and transaction signals with.
We don't want to have reimplement basic support for reading and
writing all over again.

Also, we don't necesasrily want to implement AXI specifically;
we need to implement the programmer-visible parts of it, but many
of those carry over to AXI, APB, AHB...

> Add to that ARM bus feature list exclusive/non-exclusive as well.

Ah yes. We don't really model ARM exclusive transactions at all well
anywhere in QEMU ; I think it all works more by luck than anything
else at the moment...

-- PMM

Re: [Qemu-devel] [PATCH v1 8/8] usb/ehci: Put RAM in undefined MMIO regions

[Avi Kivity]

On 10/25/2012 03:03 PM, Peter Crosthwaite wrote:
> On Thu, Oct 25, 2012 at 10:19 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> On 10/25/12 11:47, Peter Crosthwaite wrote:
>>> Just put RAM regions in the unimplemented spaces in the MMIO region. These
>>> regions have undefined behaviour, but this at least stops QEMU from segfaulting
>>> when the guest bangs on these registers (and sucessfully fakes reading and
>>> writing the registers with no side effects).
>>
>> Should not be needed, memory api should deal with that properly.
> 
> CC Avi,
> 
> Whats going on here is there is a container of size 0x1000 created
> with memory_region_init() and a handful of small subregions are
> populated. the container is then mapped to a 0x1000 size region of the
> system memory. What is supposed to happen when the guest access a
> region in the container for which no subregion has been added? 

It falls back to the parent container.  If there isn't one, something
system-specific happens.  You can override that by initializing your
container with memory_region_init_io(); the callbacks will then receive
any accesses which are not caught by any subregion.



-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v1 0/8] Sysbus EHCI + Zynq USB.

[Peter Crosthwaite]

Ping!

This is the first version of the EHCI sysbus series which takes a
property based approach rather than the dynamic class approach.

No refactoring of the PCI stuff is done here (introduced v2) but
following on from the discussion on IRC about how to do and the
suggestion we take a property based approach, could we get a review of
this and mix and match between this and V3 for the solution?

Regards,
Peter

On Thu, Oct 25, 2012 at 7:47 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Added Sysbus variant of EHCI and attached it to Xilinx Zynq. Apparently the EHCI stuff is going to useful for Tegra too. (Andreas?)
>
> See http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg04100.html thread for the inital RFC discussion on this development.
>
> Regession tested using i386 Debian (dd of=/dev/sdx ...)  to get some test coverage outside of Zynq. More testing welcome.
>
> Few other bits and pieces of cleanup in there too, around the EHCI_DEBUG printfery.
>
> Peter Crosthwaite (8):
>   usb/ehci: parameterise the register region offsets
>   usb/ehci: Abstract away PCI DMA API
>   usb/ehci: seperate out PCIisms
>   usb/ehci: Add usb-ehci-sysbus
>   xilinx_zynq: add USB controllers
>   usb/ehci: Guard definition of EHCI_DEBUG
>   usb/ehci: Debug mode compile fixes
>   usb/ehci: Put RAM in undefined MMIO regions
>
>  hw/usb/hcd-ehci.c |  285 +++++++++++++++++++++++++++++++++++------------------
>  hw/xilinx_zynq.c  |   16 +++
>  2 files changed, 204 insertions(+), 97 deletions(-)
>

Re: [Qemu-devel] [PATCH v1 0/8] Sysbus EHCI + Zynq USB.

[Gerd Hoffmann]

On 10/29/12 15:08, Peter Crosthwaite wrote:
> Ping!
> 
> This is the first version of the EHCI sysbus series which takes a
> property based approach rather than the dynamic class approach.
> 
> No refactoring of the PCI stuff is done here (introduced v2) but
> following on from the discussion on IRC about how to do and the
> suggestion we take a property based approach, could we get a review of
> this and mix and match between this and V3 for the solution?

I like the EHCI{Pci,Sysbus}State approach in the v3 series, also the
sysbus restruction so sysbus_create_simple() can be used in v2+.

I'd suggest to drop all the controversial stuff:

 - v3 patch #1 (go with NULL like ohci does for the time being,
   when we finally figured+agreed on how to fix it we can change
   both ehci+ohci).
 - v3 patch #2 (class_data for pci).

With patch #2 being gone patch #6 needs to be changed too of course.
Just do it the classic way for now and lets worry later about how to
dynamically generate variants.

Have you tested the patch for the unmapped-register access I've sent?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v1 0/8] Sysbus EHCI + Zynq USB.

[Peter Crosthwaite]

On Tue, Oct 30, 2012 at 5:20 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On 10/29/12 15:08, Peter Crosthwaite wrote:
>> Ping!
>>
>> This is the first version of the EHCI sysbus series which takes a
>> property based approach rather than the dynamic class approach.
>>
>> No refactoring of the PCI stuff is done here (introduced v2) but
>> following on from the discussion on IRC about how to do and the
>> suggestion we take a property based approach, could we get a review of
>> this and mix and match between this and V3 for the solution?
>
> I like the EHCI{Pci,Sysbus}State approach in the v3 series, also the
> sysbus restruction so sysbus_create_simple() can be used in v2+.
>
> I'd suggest to drop all the controversial stuff:
>
>  - v3 patch #1 (go with NULL like ohci does for the time being,
>    when we finally figured+agreed on how to fix it we can change
>    both ehci+ohci).
>  - v3 patch #2 (class_data for pci).
>

Hi Gerd,

Its difficult to drop this patch, as it defines struct EHCIPCIClass
which is needed to hold the capabase and opregbase properties
introduced later. If the only objection is the class_data then I can
revert to the old code driven approach with separate class_init fns
for each, but if this inheritance heirachy I have set up and the way
the properties are handled is under debate (which after IRC discussion
last night they were) then we are blocked. The key distinction from
UHCI is that there are EHCI specific props that we want to pass
through which means the definition of a new class EHCIPCIClass, which
is the point debate. There was disagreement on that. I think the
class_data was the secondary issue in the end.

Andreas, Anthony,

Can we get a decisive action plan here even if its just "do It
Andreas' way" - I dont mind fixing it, its just there were multiple
solutions kicked around and no agreement reached, so at the moment
whatever I do from here it appears one maintainer or the other is
going to block. Last I read Anthony was pro device properties, which
is close to v1 of the series, Andreas was against it, with proposed
modifications to the Class heirachy set up by v3 of the series -
mostly organsiational nothing fundamental.

Regards,
Peter

> With patch #2 being gone patch #6 needs to be changed too of course.
> Just do it the classic way for now and lets worry later about how to
> dynamically generate variants.
>
> Have you tested the patch for the unmapped-register access I've sent?
>
> cheers,
>   Gerd
>
>

Re: [Qemu-devel] [PATCH v1 0/8] Sysbus EHCI + Zynq USB.

[Gerd Hoffmann]

On 10/30/12 09:24, Peter Crosthwaite wrote:
> On Tue, Oct 30, 2012 at 5:20 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> On 10/29/12 15:08, Peter Crosthwaite wrote:
>>> Ping!
>>>
>>> This is the first version of the EHCI sysbus series which takes a
>>> property based approach rather than the dynamic class approach.
>>>
>>> No refactoring of the PCI stuff is done here (introduced v2) but
>>> following on from the discussion on IRC about how to do and the
>>> suggestion we take a property based approach, could we get a review of
>>> this and mix and match between this and V3 for the solution?
>>
>> I like the EHCI{Pci,Sysbus}State approach in the v3 series, also the
>> sysbus restruction so sysbus_create_simple() can be used in v2+.
>>
>> I'd suggest to drop all the controversial stuff:
>>
>>  - v3 patch #1 (go with NULL like ohci does for the time being,
>>    when we finally figured+agreed on how to fix it we can change
>>    both ehci+ohci).
>>  - v3 patch #2 (class_data for pci).
>>
> 
> Hi Gerd,
> 
> Its difficult to drop this patch, as it defines struct EHCIPCIClass
> which is needed to hold the capabase and opregbase properties
> introduced later.

Just set them hard-coded in usb_ehci_pci_initfn (before calling into the
common usb_ehci_initfn function obviously), they are identical in all
pci variants.

The same works (for now) for sysbus, although that will probably change
in the future when we add more variants with other base values.

> If the only objection is the class_data then I can
> revert to the old code driven approach with separate class_init fns
> for each,

Yes, just keep the separate class_init fns (+drop EHCIPCIClass) for now.

For a quick merge I'd suggest to go with established practices here,
i.e. either just have the properties twice (once for pci, once for
sysfs) or use a #define like nic/block properties do.

Exploring ways to do this in a better way using QOM is fine too, but I
wouldn't attempt this for the initial merge.  QOM is at least partly
still in the design phase, thus causing lots of discussions when it
comes to hashing out the details as you've seen.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v1 0/8] Sysbus EHCI + Zynq USB.

[Andreas Färber]

Am 30.10.2012 09:24, schrieb Peter Crosthwaite:
> On Tue, Oct 30, 2012 at 5:20 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> On 10/29/12 15:08, Peter Crosthwaite wrote:
>>> Ping!
>>>
>>> This is the first version of the EHCI sysbus series which takes a
>>> property based approach rather than the dynamic class approach.
>>>
>>> No refactoring of the PCI stuff is done here (introduced v2) but
>>> following on from the discussion on IRC about how to do and the
>>> suggestion we take a property based approach, could we get a review of
>>> this and mix and match between this and V3 for the solution?
>>
>> I like the EHCI{Pci,Sysbus}State approach in the v3 series, also the
>> sysbus restruction so sysbus_create_simple() can be used in v2+.
>>
>> I'd suggest to drop all the controversial stuff:
>>
>>  - v3 patch #1 (go with NULL like ohci does for the time being,
>>    when we finally figured+agreed on how to fix it we can change
>>    both ehci+ohci).
>>  - v3 patch #2 (class_data for pci).
>>
> 
> Hi Gerd,
> 
> Its difficult to drop this patch, as it defines struct EHCIPCIClass
> which is needed to hold the capabase and opregbase properties
> introduced later. If the only objection is the class_data then I can
> revert to the old code driven approach with separate class_init fns
> for each, but if this inheritance heirachy I have set up and the way
> the properties are handled is under debate (which after IRC discussion
> last night they were) then we are blocked. The key distinction from
> UHCI is that there are EHCI specific props that we want to pass
> through which means the definition of a new class EHCIPCIClass, which
> is the point debate. There was disagreement on that. I think the
> class_data was the secondary issue in the end.
> 
> Andreas, Anthony,
> 
> Can we get a decisive action plan here even if its just "do It
> Andreas' way" - I dont mind fixing it, its just there were multiple
> solutions kicked around and no agreement reached, so at the moment
> whatever I do from here it appears one maintainer or the other is
> going to block. Last I read Anthony was pro device properties, which
> is close to v1 of the series, Andreas was against it, with proposed
> modifications to the Class heirachy set up by v3 of the series -
> mostly organsiational nothing fundamental.

Erm, I'm not against properties, I just doubted it would work in the
described scenario, but Anthony said there were some magic that would
make it work. If it works overridably, then fine with me!

What I am strongly against was the union parent approach, and in the
last iteration I requested some formal cleanups / patch minimizations
(not yet fully reviewed, /me cooking up a CPU pull).

Cheers,
Andreas

P.S. Thanks for digging out the SDHCI series; I rebased that on my end
and was about to ask.

> 
> Regards,
> Peter
> 
>> With patch #2 being gone patch #6 needs to be changed too of course.
>> Just do it the classic way for now and lets worry later about how to
>> dynamically generate variants.
>>
>> Have you tested the patch for the unmapped-register access I've sent?
>>
>> cheers,
>>   Gerd
>>
>>


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg