| Message ID | 1340087992-2399-5-git-send-email-benh@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
On 06/19/2012 01:39 AM, Benjamin Herrenschmidt wrote: > From: David Gibson<david@gibson.dropbear.id.au> > > The OHCI device emulation can provide both PCI and SysBus OHCI > implementations. Because of this, it was not previously converted to > use the PCI DMA helper functions. > > This patch converts it to use the new universal DMA helper functions. > In the PCI case, it obtains its DMAContext from pci_dma_context(), in > the SysBus case, it uses NULL - i.e. assumes for now that there will > be no IOMMU translation for a SysBus OHCI. > > Cc: Gerd Hoffmann<kraxel@redhat.com> > Cc: Michael S. Tsirkin<mst@redhat.com> > > Signed-off-by: David Gibson<david@gibson.dropbear.id.au> > Signed-off-by: Benjamin Herrenschmidt<benh@kernel.crashing.org> So... the DMA api is designed to allow for partial result returns which I presume an implementation would use as a simplification. But none of these callers actually check the return code? Either errors are important and we need to adjust callees to check them or errors aren't important and we should return void. Why leave pci accessors and not implement usb_memory_rw() wrappers? Regards, Anthony Liguori > --- > hw/usb/hcd-ohci.c | 93 +++++++++++++++++++++++++++++------------------------ > 1 file changed, 51 insertions(+), 42 deletions(-) > > diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c > index 1a1cc88..844e7ed 100644 > --- a/hw/usb/hcd-ohci.c > +++ b/hw/usb/hcd-ohci.c > @@ -31,7 +31,7 @@ > #include "hw/usb.h" > #include "hw/pci.h" > #include "hw/sysbus.h" > -#include "hw/qdev-addr.h" > +#include "hw/qdev-dma.h" > > //#define DEBUG_OHCI > /* Dump packet contents. */ > @@ -62,6 +62,7 @@ typedef struct { > USBBus bus; > qemu_irq irq; > MemoryRegion mem; > + DMAContext *dma; > int num_ports; > const char *name; > > @@ -104,7 +105,7 @@ typedef struct { > uint32_t htest; > > /* SM501 local memory offset */ > - target_phys_addr_t localmem_base; > + dma_addr_t localmem_base; > > /* Active packets. */ > uint32_t old_ctl; > @@ -482,14 +483,14 @@ static void ohci_reset(void *opaque) > > /* Get an array of dwords from main memory */ > static inline int get_dwords(OHCIState *ohci, > - uint32_t addr, uint32_t *buf, int num) > + dma_addr_t addr, uint32_t *buf, int num) > { > int i; > > addr += ohci->localmem_base; > > for (i = 0; i< num; i++, buf++, addr += sizeof(*buf)) { > - cpu_physical_memory_read(addr, buf, sizeof(*buf)); > + dma_memory_read(ohci->dma, addr, buf, sizeof(*buf)); > *buf = le32_to_cpu(*buf); > } > > @@ -498,7 +499,7 @@ static inline int get_dwords(OHCIState *ohci, > > /* Put an array of dwords in to main memory */ > static inline int put_dwords(OHCIState *ohci, > - uint32_t addr, uint32_t *buf, int num) > + dma_addr_t addr, uint32_t *buf, int num) > { > int i; > > @@ -506,7 +507,7 @@ static inline int put_dwords(OHCIState *ohci, > > for (i = 0; i< num; i++, buf++, addr += sizeof(*buf)) { > uint32_t tmp = cpu_to_le32(*buf); > - cpu_physical_memory_write(addr,&tmp, sizeof(tmp)); > + dma_memory_write(ohci->dma, addr,&tmp, sizeof(tmp)); > } > > return 1; > @@ -514,14 +515,14 @@ static inline int put_dwords(OHCIState *ohci, > > /* Get an array of words from main memory */ > static inline int get_words(OHCIState *ohci, > - uint32_t addr, uint16_t *buf, int num) > + dma_addr_t addr, uint16_t *buf, int num) > { > int i; > > addr += ohci->localmem_base; > > for (i = 0; i< num; i++, buf++, addr += sizeof(*buf)) { > - cpu_physical_memory_read(addr, buf, sizeof(*buf)); > + dma_memory_read(ohci->dma, addr, buf, sizeof(*buf)); > *buf = le16_to_cpu(*buf); > } > > @@ -530,7 +531,7 @@ static inline int get_words(OHCIState *ohci, > > /* Put an array of words in to main memory */ > static inline int put_words(OHCIState *ohci, > - uint32_t addr, uint16_t *buf, int num) > + dma_addr_t addr, uint16_t *buf, int num) > { > int i; > > @@ -538,40 +539,40 @@ static inline int put_words(OHCIState *ohci, > > for (i = 0; i< num; i++, buf++, addr += sizeof(*buf)) { > uint16_t tmp = cpu_to_le16(*buf); > - cpu_physical_memory_write(addr,&tmp, sizeof(tmp)); > + dma_memory_write(ohci->dma, addr,&tmp, sizeof(tmp)); > } > > return 1; > } > > static inline int ohci_read_ed(OHCIState *ohci, > - uint32_t addr, struct ohci_ed *ed) > + dma_addr_t addr, struct ohci_ed *ed) > { > return get_dwords(ohci, addr, (uint32_t *)ed, sizeof(*ed)>> 2); > } > > static inline int ohci_read_td(OHCIState *ohci, > - uint32_t addr, struct ohci_td *td) > + dma_addr_t addr, struct ohci_td *td) > { > return get_dwords(ohci, addr, (uint32_t *)td, sizeof(*td)>> 2); > } > > static inline int ohci_read_iso_td(OHCIState *ohci, > - uint32_t addr, struct ohci_iso_td *td) > + dma_addr_t addr, struct ohci_iso_td *td) > { > return (get_dwords(ohci, addr, (uint32_t *)td, 4)&& > get_words(ohci, addr + 16, td->offset, 8)); > } > > static inline int ohci_read_hcca(OHCIState *ohci, > - uint32_t addr, struct ohci_hcca *hcca) > + dma_addr_t addr, struct ohci_hcca *hcca) > { > - cpu_physical_memory_read(addr + ohci->localmem_base, hcca, sizeof(*hcca)); > + dma_memory_read(ohci->dma, addr + ohci->localmem_base, hcca, sizeof(*hcca)); > return 1; > } > > static inline int ohci_put_ed(OHCIState *ohci, > - uint32_t addr, struct ohci_ed *ed) > + dma_addr_t addr, struct ohci_ed *ed) > { > /* ed->tail is under control of the HCD. > * Since just ed->head is changed by HC, just write back this > @@ -583,64 +584,63 @@ static inline int ohci_put_ed(OHCIState *ohci, > } > > static inline int ohci_put_td(OHCIState *ohci, > - uint32_t addr, struct ohci_td *td) > + dma_addr_t addr, struct ohci_td *td) > { > return put_dwords(ohci, addr, (uint32_t *)td, sizeof(*td)>> 2); > } > > static inline int ohci_put_iso_td(OHCIState *ohci, > - uint32_t addr, struct ohci_iso_td *td) > + dma_addr_t addr, struct ohci_iso_td *td) > { > return (put_dwords(ohci, addr, (uint32_t *)td, 4)&& > put_words(ohci, addr + 16, td->offset, 8)); > } > > static inline int ohci_put_hcca(OHCIState *ohci, > - uint32_t addr, struct ohci_hcca *hcca) > + dma_addr_t addr, struct ohci_hcca *hcca) > { > - cpu_physical_memory_write(addr + ohci->localmem_base + HCCA_WRITEBACK_OFFSET, > - (char *)hcca + HCCA_WRITEBACK_OFFSET, > - HCCA_WRITEBACK_SIZE); > + dma_memory_write(ohci->dma, > + addr + ohci->localmem_base + HCCA_WRITEBACK_OFFSET, > + (char *)hcca + HCCA_WRITEBACK_OFFSET, > + HCCA_WRITEBACK_SIZE); > return 1; > } > > /* Read/Write the contents of a TD from/to main memory. */ > static void ohci_copy_td(OHCIState *ohci, struct ohci_td *td, > - uint8_t *buf, int len, int write) > + uint8_t *buf, int len, DMADirection dir) > { > - uint32_t ptr; > - uint32_t n; > + dma_addr_t ptr, n; > > ptr = td->cbp; > n = 0x1000 - (ptr& 0xfff); > if (n> len) > n = len; > - cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, n, write); > + dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, n, dir); > if (n == len) > return; > ptr = td->be& ~0xfffu; > buf += n; > - cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, len - n, write); > + dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, len - n, dir); > } > > /* Read/Write the contents of an ISO TD from/to main memory. */ > static void ohci_copy_iso_td(OHCIState *ohci, > uint32_t start_addr, uint32_t end_addr, > - uint8_t *buf, int len, int write) > + uint8_t *buf, int len, DMADirection dir) > { > - uint32_t ptr; > - uint32_t n; > + dma_addr_t ptr, n; > > ptr = start_addr; > n = 0x1000 - (ptr& 0xfff); > if (n> len) > n = len; > - cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, n, write); > + dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, n, dir); > if (n == len) > return; > ptr = end_addr& ~0xfffu; > buf += n; > - cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, len - n, write); > + dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, len - n, dir); > } > > static void ohci_process_lists(OHCIState *ohci, int completion); > @@ -803,7 +803,8 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed, > } > > if (len&& dir != OHCI_TD_DIR_IN) { > - ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len, 0); > + ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len, > + DMA_DIRECTION_TO_DEVICE); > } > > if (completion) { > @@ -827,7 +828,8 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed, > /* Writeback */ > if (dir == OHCI_TD_DIR_IN&& ret>= 0&& ret<= len) { > /* IN transfer succeeded */ > - ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, ret, 1); > + ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, ret, > + DMA_DIRECTION_FROM_DEVICE); > OHCI_SET_BM(iso_td.offset[relative_frame_number], TD_PSW_CC, > OHCI_CC_NOERROR); > OHCI_SET_BM(iso_td.offset[relative_frame_number], TD_PSW_SIZE, ret); > @@ -971,7 +973,8 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed) > pktlen = len; > } > if (!completion) { > - ohci_copy_td(ohci,&td, ohci->usb_buf, pktlen, 0); > + ohci_copy_td(ohci,&td, ohci->usb_buf, pktlen, > + DMA_DIRECTION_TO_DEVICE); > } > } > } > @@ -1021,7 +1024,8 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed) > } > if (ret>= 0) { > if (dir == OHCI_TD_DIR_IN) { > - ohci_copy_td(ohci,&td, ohci->usb_buf, ret, 1); > + ohci_copy_td(ohci,&td, ohci->usb_buf, ret, > + DMA_DIRECTION_FROM_DEVICE); > #ifdef DEBUG_PACKET > DPRINTF(" data:"); > for (i = 0; i< ret; i++) > @@ -1748,11 +1752,14 @@ static USBBusOps ohci_bus_ops = { > }; > > static int usb_ohci_init(OHCIState *ohci, DeviceState *dev, > - int num_ports, uint32_t localmem_base, > - char *masterbus, uint32_t firstport) > + int num_ports, dma_addr_t localmem_base, > + char *masterbus, uint32_t firstport, > + DMAContext *dma) > { > int i; > > + ohci->dma = dma; > + > if (usb_frame_time == 0) { > #ifdef OHCI_TIME_WARP > usb_frame_time = get_ticks_per_sec(); > @@ -1817,7 +1824,8 @@ static int usb_ohci_initfn_pci(struct PCIDevice *dev) > ohci->pci_dev.config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */ > > if (usb_ohci_init(&ohci->state,&dev->qdev, ohci->num_ports, 0, > - ohci->masterbus, ohci->firstport) != 0) { > + ohci->masterbus, ohci->firstport, > + pci_dma_context(dev)) != 0) { > return -1; > } > ohci->state.irq = ohci->pci_dev.irq[0]; > @@ -1831,7 +1839,7 @@ typedef struct { > SysBusDevice busdev; > OHCIState ohci; > uint32_t num_ports; > - target_phys_addr_t dma_offset; > + dma_addr_t dma_offset; > } OHCISysBusState; > > static int ohci_init_pxa(SysBusDevice *dev) > @@ -1839,7 +1847,8 @@ static int ohci_init_pxa(SysBusDevice *dev) > OHCISysBusState *s = FROM_SYSBUS(OHCISysBusState, dev); > > /* Cannot fail as we pass NULL for masterbus */ > - usb_ohci_init(&s->ohci,&dev->qdev, s->num_ports, s->dma_offset, NULL, 0); > + usb_ohci_init(&s->ohci,&dev->qdev, s->num_ports, s->dma_offset, NULL, 0, > + NULL); > sysbus_init_irq(dev,&s->ohci.irq); > sysbus_init_mmio(dev,&s->ohci.mem); > > @@ -1875,7 +1884,7 @@ static TypeInfo ohci_pci_info = { > > static Property ohci_sysbus_properties[] = { > DEFINE_PROP_UINT32("num-ports", OHCISysBusState, num_ports, 3), > - DEFINE_PROP_TADDR("dma-offset", OHCISysBusState, dma_offset, 3), > + DEFINE_PROP_DMAADDR("dma-offset", OHCISysBusState, dma_offset, 3), > DEFINE_PROP_END_OF_LIST(), > }; >
> > Cc: Gerd Hoffmann<kraxel@redhat.com> > > Cc: Michael S. Tsirkin<mst@redhat.com> > > > > Signed-off-by: David Gibson<david@gibson.dropbear.id.au> > > Signed-off-by: Benjamin Herrenschmidt<benh@kernel.crashing.org> > > > So... the DMA api is designed to allow for partial result returns which I > presume an implementation would use as a simplification. > > But none of these callers actually check the return code? > > Either errors are important and we need to adjust callees to check them or > errors aren't important and we should return void. Errors should matter and I agree callers should check them, this is the series I inherited and I believe it does need improvements in those areas (though it would be easier if the driver authors were the one to do those improvements). > Why leave pci accessors and not implement usb_memory_rw() wrappers? Well, "usb" is a bit too generic, ehci and ohci would each need to have their own sets of wrappers. But yes, that's possible... is it really worth it ? There's nothing fundamentally wrong with using the dma_* accessors. Cheers, Ben. > Regards, > > Anthony Liguori > > > --- > > hw/usb/hcd-ohci.c | 93 +++++++++++++++++++++++++++++------------------------ > > 1 file changed, 51 insertions(+), 42 deletions(-) > > > > diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c > > index 1a1cc88..844e7ed 100644 > > --- a/hw/usb/hcd-ohci.c > > +++ b/hw/usb/hcd-ohci.c > > @@ -31,7 +31,7 @@ > > #include "hw/usb.h" > > #include "hw/pci.h" > > #include "hw/sysbus.h" > > -#include "hw/qdev-addr.h" > > +#include "hw/qdev-dma.h" > > > > //#define DEBUG_OHCI > > /* Dump packet contents. */ > > @@ -62,6 +62,7 @@ typedef struct { > > USBBus bus; > > qemu_irq irq; > > MemoryRegion mem; > > + DMAContext *dma; > > int num_ports; > > const char *name; > > > > @@ -104,7 +105,7 @@ typedef struct { > > uint32_t htest; > > > > /* SM501 local memory offset */ > > - target_phys_addr_t localmem_base; > > + dma_addr_t localmem_base; > > > > /* Active packets. */ > > uint32_t old_ctl; > > @@ -482,14 +483,14 @@ static void ohci_reset(void *opaque) > > > > /* Get an array of dwords from main memory */ > > static inline int get_dwords(OHCIState *ohci, > > - uint32_t addr, uint32_t *buf, int num) > > + dma_addr_t addr, uint32_t *buf, int num) > > { > > int i; > > > > addr += ohci->localmem_base; > > > > for (i = 0; i< num; i++, buf++, addr += sizeof(*buf)) { > > - cpu_physical_memory_read(addr, buf, sizeof(*buf)); > > + dma_memory_read(ohci->dma, addr, buf, sizeof(*buf)); > > *buf = le32_to_cpu(*buf); > > } > > > > @@ -498,7 +499,7 @@ static inline int get_dwords(OHCIState *ohci, > > > > /* Put an array of dwords in to main memory */ > > static inline int put_dwords(OHCIState *ohci, > > - uint32_t addr, uint32_t *buf, int num) > > + dma_addr_t addr, uint32_t *buf, int num) > > { > > int i; > > > > @@ -506,7 +507,7 @@ static inline int put_dwords(OHCIState *ohci, > > > > for (i = 0; i< num; i++, buf++, addr += sizeof(*buf)) { > > uint32_t tmp = cpu_to_le32(*buf); > > - cpu_physical_memory_write(addr,&tmp, sizeof(tmp)); > > + dma_memory_write(ohci->dma, addr,&tmp, sizeof(tmp)); > > } > > > > return 1; > > @@ -514,14 +515,14 @@ static inline int put_dwords(OHCIState *ohci, > > > > /* Get an array of words from main memory */ > > static inline int get_words(OHCIState *ohci, > > - uint32_t addr, uint16_t *buf, int num) > > + dma_addr_t addr, uint16_t *buf, int num) > > { > > int i; > > > > addr += ohci->localmem_base; > > > > for (i = 0; i< num; i++, buf++, addr += sizeof(*buf)) { > > - cpu_physical_memory_read(addr, buf, sizeof(*buf)); > > + dma_memory_read(ohci->dma, addr, buf, sizeof(*buf)); > > *buf = le16_to_cpu(*buf); > > } > > > > @@ -530,7 +531,7 @@ static inline int get_words(OHCIState *ohci, > > > > /* Put an array of words in to main memory */ > > static inline int put_words(OHCIState *ohci, > > - uint32_t addr, uint16_t *buf, int num) > > + dma_addr_t addr, uint16_t *buf, int num) > > { > > int i; > > > > @@ -538,40 +539,40 @@ static inline int put_words(OHCIState *ohci, > > > > for (i = 0; i< num; i++, buf++, addr += sizeof(*buf)) { > > uint16_t tmp = cpu_to_le16(*buf); > > - cpu_physical_memory_write(addr,&tmp, sizeof(tmp)); > > + dma_memory_write(ohci->dma, addr,&tmp, sizeof(tmp)); > > } > > > > return 1; > > } > > > > static inline int ohci_read_ed(OHCIState *ohci, > > - uint32_t addr, struct ohci_ed *ed) > > + dma_addr_t addr, struct ohci_ed *ed) > > { > > return get_dwords(ohci, addr, (uint32_t *)ed, sizeof(*ed)>> 2); > > } > > > > static inline int ohci_read_td(OHCIState *ohci, > > - uint32_t addr, struct ohci_td *td) > > + dma_addr_t addr, struct ohci_td *td) > > { > > return get_dwords(ohci, addr, (uint32_t *)td, sizeof(*td)>> 2); > > } > > > > static inline int ohci_read_iso_td(OHCIState *ohci, > > - uint32_t addr, struct ohci_iso_td *td) > > + dma_addr_t addr, struct ohci_iso_td *td) > > { > > return (get_dwords(ohci, addr, (uint32_t *)td, 4)&& > > get_words(ohci, addr + 16, td->offset, 8)); > > } > > > > static inline int ohci_read_hcca(OHCIState *ohci, > > - uint32_t addr, struct ohci_hcca *hcca) > > + dma_addr_t addr, struct ohci_hcca *hcca) > > { > > - cpu_physical_memory_read(addr + ohci->localmem_base, hcca, sizeof(*hcca)); > > + dma_memory_read(ohci->dma, addr + ohci->localmem_base, hcca, sizeof(*hcca)); > > return 1; > > } > > > > static inline int ohci_put_ed(OHCIState *ohci, > > - uint32_t addr, struct ohci_ed *ed) > > + dma_addr_t addr, struct ohci_ed *ed) > > { > > /* ed->tail is under control of the HCD. > > * Since just ed->head is changed by HC, just write back this > > @@ -583,64 +584,63 @@ static inline int ohci_put_ed(OHCIState *ohci, > > } > > > > static inline int ohci_put_td(OHCIState *ohci, > > - uint32_t addr, struct ohci_td *td) > > + dma_addr_t addr, struct ohci_td *td) > > { > > return put_dwords(ohci, addr, (uint32_t *)td, sizeof(*td)>> 2); > > } > > > > static inline int ohci_put_iso_td(OHCIState *ohci, > > - uint32_t addr, struct ohci_iso_td *td) > > + dma_addr_t addr, struct ohci_iso_td *td) > > { > > return (put_dwords(ohci, addr, (uint32_t *)td, 4)&& > > put_words(ohci, addr + 16, td->offset, 8)); > > } > > > > static inline int ohci_put_hcca(OHCIState *ohci, > > - uint32_t addr, struct ohci_hcca *hcca) > > + dma_addr_t addr, struct ohci_hcca *hcca) > > { > > - cpu_physical_memory_write(addr + ohci->localmem_base + HCCA_WRITEBACK_OFFSET, > > - (char *)hcca + HCCA_WRITEBACK_OFFSET, > > - HCCA_WRITEBACK_SIZE); > > + dma_memory_write(ohci->dma, > > + addr + ohci->localmem_base + HCCA_WRITEBACK_OFFSET, > > + (char *)hcca + HCCA_WRITEBACK_OFFSET, > > + HCCA_WRITEBACK_SIZE); > > return 1; > > } > > > > /* Read/Write the contents of a TD from/to main memory. */ > > static void ohci_copy_td(OHCIState *ohci, struct ohci_td *td, > > - uint8_t *buf, int len, int write) > > + uint8_t *buf, int len, DMADirection dir) > > { > > - uint32_t ptr; > > - uint32_t n; > > + dma_addr_t ptr, n; > > > > ptr = td->cbp; > > n = 0x1000 - (ptr& 0xfff); > > if (n> len) > > n = len; > > - cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, n, write); > > + dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, n, dir); > > if (n == len) > > return; > > ptr = td->be& ~0xfffu; > > buf += n; > > - cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, len - n, write); > > + dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, len - n, dir); > > } > > > > /* Read/Write the contents of an ISO TD from/to main memory. */ > > static void ohci_copy_iso_td(OHCIState *ohci, > > uint32_t start_addr, uint32_t end_addr, > > - uint8_t *buf, int len, int write) > > + uint8_t *buf, int len, DMADirection dir) > > { > > - uint32_t ptr; > > - uint32_t n; > > + dma_addr_t ptr, n; > > > > ptr = start_addr; > > n = 0x1000 - (ptr& 0xfff); > > if (n> len) > > n = len; > > - cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, n, write); > > + dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, n, dir); > > if (n == len) > > return; > > ptr = end_addr& ~0xfffu; > > buf += n; > > - cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, len - n, write); > > + dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, len - n, dir); > > } > > > > static void ohci_process_lists(OHCIState *ohci, int completion); > > @@ -803,7 +803,8 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed, > > } > > > > if (len&& dir != OHCI_TD_DIR_IN) { > > - ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len, 0); > > + ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len, > > + DMA_DIRECTION_TO_DEVICE); > > } > > > > if (completion) { > > @@ -827,7 +828,8 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed, > > /* Writeback */ > > if (dir == OHCI_TD_DIR_IN&& ret>= 0&& ret<= len) { > > /* IN transfer succeeded */ > > - ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, ret, 1); > > + ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, ret, > > + DMA_DIRECTION_FROM_DEVICE); > > OHCI_SET_BM(iso_td.offset[relative_frame_number], TD_PSW_CC, > > OHCI_CC_NOERROR); > > OHCI_SET_BM(iso_td.offset[relative_frame_number], TD_PSW_SIZE, ret); > > @@ -971,7 +973,8 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed) > > pktlen = len; > > } > > if (!completion) { > > - ohci_copy_td(ohci,&td, ohci->usb_buf, pktlen, 0); > > + ohci_copy_td(ohci,&td, ohci->usb_buf, pktlen, > > + DMA_DIRECTION_TO_DEVICE); > > } > > } > > } > > @@ -1021,7 +1024,8 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed) > > } > > if (ret>= 0) { > > if (dir == OHCI_TD_DIR_IN) { > > - ohci_copy_td(ohci,&td, ohci->usb_buf, ret, 1); > > + ohci_copy_td(ohci,&td, ohci->usb_buf, ret, > > + DMA_DIRECTION_FROM_DEVICE); > > #ifdef DEBUG_PACKET > > DPRINTF(" data:"); > > for (i = 0; i< ret; i++) > > @@ -1748,11 +1752,14 @@ static USBBusOps ohci_bus_ops = { > > }; > > > > static int usb_ohci_init(OHCIState *ohci, DeviceState *dev, > > - int num_ports, uint32_t localmem_base, > > - char *masterbus, uint32_t firstport) > > + int num_ports, dma_addr_t localmem_base, > > + char *masterbus, uint32_t firstport, > > + DMAContext *dma) > > { > > int i; > > > > + ohci->dma = dma; > > + > > if (usb_frame_time == 0) { > > #ifdef OHCI_TIME_WARP > > usb_frame_time = get_ticks_per_sec(); > > @@ -1817,7 +1824,8 @@ static int usb_ohci_initfn_pci(struct PCIDevice *dev) > > ohci->pci_dev.config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */ > > > > if (usb_ohci_init(&ohci->state,&dev->qdev, ohci->num_ports, 0, > > - ohci->masterbus, ohci->firstport) != 0) { > > + ohci->masterbus, ohci->firstport, > > + pci_dma_context(dev)) != 0) { > > return -1; > > } > > ohci->state.irq = ohci->pci_dev.irq[0]; > > @@ -1831,7 +1839,7 @@ typedef struct { > > SysBusDevice busdev; > > OHCIState ohci; > > uint32_t num_ports; > > - target_phys_addr_t dma_offset; > > + dma_addr_t dma_offset; > > } OHCISysBusState; > > > > static int ohci_init_pxa(SysBusDevice *dev) > > @@ -1839,7 +1847,8 @@ static int ohci_init_pxa(SysBusDevice *dev) > > OHCISysBusState *s = FROM_SYSBUS(OHCISysBusState, dev); > > > > /* Cannot fail as we pass NULL for masterbus */ > > - usb_ohci_init(&s->ohci,&dev->qdev, s->num_ports, s->dma_offset, NULL, 0); > > + usb_ohci_init(&s->ohci,&dev->qdev, s->num_ports, s->dma_offset, NULL, 0, > > + NULL); > > sysbus_init_irq(dev,&s->ohci.irq); > > sysbus_init_mmio(dev,&s->ohci.mem); > > > > @@ -1875,7 +1884,7 @@ static TypeInfo ohci_pci_info = { > > > > static Property ohci_sysbus_properties[] = { > > DEFINE_PROP_UINT32("num-ports", OHCISysBusState, num_ports, 3), > > - DEFINE_PROP_TADDR("dma-offset", OHCISysBusState, dma_offset, 3), > > + DEFINE_PROP_DMAADDR("dma-offset", OHCISysBusState, dma_offset, 3), > > DEFINE_PROP_END_OF_LIST(), > > }; > >
On 06/20/2012 04:36 PM, Benjamin Herrenschmidt wrote: >>> Cc: Gerd Hoffmann<kraxel@redhat.com> >>> Cc: Michael S. Tsirkin<mst@redhat.com> >>> >>> Signed-off-by: David Gibson<david@gibson.dropbear.id.au> >>> Signed-off-by: Benjamin Herrenschmidt<benh@kernel.crashing.org> >> >> >> So... the DMA api is designed to allow for partial result returns which I >> presume an implementation would use as a simplification. >> >> But none of these callers actually check the return code? >> >> Either errors are important and we need to adjust callees to check them or >> errors aren't important and we should return void. > > Errors should matter and I agree callers should check them, this is the > series I inherited and I believe it does need improvements in those > areas (though it would be easier if the driver authors were the one to > do those improvements). Well let's return void in the DMA methods and let the IOMMUs assert on error. At least that will avoid surprises until someone decides they care enough about errors to touch all callers. I think silently failing a memcpy() can potentially lead to a vulnerability so I'd rather avoid that. > >> Why leave pci accessors and not implement usb_memory_rw() wrappers? > > Well, "usb" is a bit too generic, ehci and ohci would each need to have > their own sets of wrappers. But yes, that's possible... is it really > worth it ? There's nothing fundamentally wrong with using the dma_* > accessors. So is using the pci accessors wrong? I'm not saying you should go and convert every caller of the pci_ functions, I just want a clear policy on what interface devices should use. Regards, Anthony Liguori > > Cheers, > Ben. > >> Regards, >> >> Anthony Liguori >> >>> --- >>> hw/usb/hcd-ohci.c | 93 +++++++++++++++++++++++++++++------------------------ >>> 1 file changed, 51 insertions(+), 42 deletions(-) >>> >>> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c >>> index 1a1cc88..844e7ed 100644 >>> --- a/hw/usb/hcd-ohci.c >>> +++ b/hw/usb/hcd-ohci.c >>> @@ -31,7 +31,7 @@ >>> #include "hw/usb.h" >>> #include "hw/pci.h" >>> #include "hw/sysbus.h" >>> -#include "hw/qdev-addr.h" >>> +#include "hw/qdev-dma.h" >>> >>> //#define DEBUG_OHCI >>> /* Dump packet contents. */ >>> @@ -62,6 +62,7 @@ typedef struct { >>> USBBus bus; >>> qemu_irq irq; >>> MemoryRegion mem; >>> + DMAContext *dma; >>> int num_ports; >>> const char *name; >>> >>> @@ -104,7 +105,7 @@ typedef struct { >>> uint32_t htest; >>> >>> /* SM501 local memory offset */ >>> - target_phys_addr_t localmem_base; >>> + dma_addr_t localmem_base; >>> >>> /* Active packets. */ >>> uint32_t old_ctl; >>> @@ -482,14 +483,14 @@ static void ohci_reset(void *opaque) >>> >>> /* Get an array of dwords from main memory */ >>> static inline int get_dwords(OHCIState *ohci, >>> - uint32_t addr, uint32_t *buf, int num) >>> + dma_addr_t addr, uint32_t *buf, int num) >>> { >>> int i; >>> >>> addr += ohci->localmem_base; >>> >>> for (i = 0; i< num; i++, buf++, addr += sizeof(*buf)) { >>> - cpu_physical_memory_read(addr, buf, sizeof(*buf)); >>> + dma_memory_read(ohci->dma, addr, buf, sizeof(*buf)); >>> *buf = le32_to_cpu(*buf); >>> } >>> >>> @@ -498,7 +499,7 @@ static inline int get_dwords(OHCIState *ohci, >>> >>> /* Put an array of dwords in to main memory */ >>> static inline int put_dwords(OHCIState *ohci, >>> - uint32_t addr, uint32_t *buf, int num) >>> + dma_addr_t addr, uint32_t *buf, int num) >>> { >>> int i; >>> >>> @@ -506,7 +507,7 @@ static inline int put_dwords(OHCIState *ohci, >>> >>> for (i = 0; i< num; i++, buf++, addr += sizeof(*buf)) { >>> uint32_t tmp = cpu_to_le32(*buf); >>> - cpu_physical_memory_write(addr,&tmp, sizeof(tmp)); >>> + dma_memory_write(ohci->dma, addr,&tmp, sizeof(tmp)); >>> } >>> >>> return 1; >>> @@ -514,14 +515,14 @@ static inline int put_dwords(OHCIState *ohci, >>> >>> /* Get an array of words from main memory */ >>> static inline int get_words(OHCIState *ohci, >>> - uint32_t addr, uint16_t *buf, int num) >>> + dma_addr_t addr, uint16_t *buf, int num) >>> { >>> int i; >>> >>> addr += ohci->localmem_base; >>> >>> for (i = 0; i< num; i++, buf++, addr += sizeof(*buf)) { >>> - cpu_physical_memory_read(addr, buf, sizeof(*buf)); >>> + dma_memory_read(ohci->dma, addr, buf, sizeof(*buf)); >>> *buf = le16_to_cpu(*buf); >>> } >>> >>> @@ -530,7 +531,7 @@ static inline int get_words(OHCIState *ohci, >>> >>> /* Put an array of words in to main memory */ >>> static inline int put_words(OHCIState *ohci, >>> - uint32_t addr, uint16_t *buf, int num) >>> + dma_addr_t addr, uint16_t *buf, int num) >>> { >>> int i; >>> >>> @@ -538,40 +539,40 @@ static inline int put_words(OHCIState *ohci, >>> >>> for (i = 0; i< num; i++, buf++, addr += sizeof(*buf)) { >>> uint16_t tmp = cpu_to_le16(*buf); >>> - cpu_physical_memory_write(addr,&tmp, sizeof(tmp)); >>> + dma_memory_write(ohci->dma, addr,&tmp, sizeof(tmp)); >>> } >>> >>> return 1; >>> } >>> >>> static inline int ohci_read_ed(OHCIState *ohci, >>> - uint32_t addr, struct ohci_ed *ed) >>> + dma_addr_t addr, struct ohci_ed *ed) >>> { >>> return get_dwords(ohci, addr, (uint32_t *)ed, sizeof(*ed)>> 2); >>> } >>> >>> static inline int ohci_read_td(OHCIState *ohci, >>> - uint32_t addr, struct ohci_td *td) >>> + dma_addr_t addr, struct ohci_td *td) >>> { >>> return get_dwords(ohci, addr, (uint32_t *)td, sizeof(*td)>> 2); >>> } >>> >>> static inline int ohci_read_iso_td(OHCIState *ohci, >>> - uint32_t addr, struct ohci_iso_td *td) >>> + dma_addr_t addr, struct ohci_iso_td *td) >>> { >>> return (get_dwords(ohci, addr, (uint32_t *)td, 4)&& >>> get_words(ohci, addr + 16, td->offset, 8)); >>> } >>> >>> static inline int ohci_read_hcca(OHCIState *ohci, >>> - uint32_t addr, struct ohci_hcca *hcca) >>> + dma_addr_t addr, struct ohci_hcca *hcca) >>> { >>> - cpu_physical_memory_read(addr + ohci->localmem_base, hcca, sizeof(*hcca)); >>> + dma_memory_read(ohci->dma, addr + ohci->localmem_base, hcca, sizeof(*hcca)); >>> return 1; >>> } >>> >>> static inline int ohci_put_ed(OHCIState *ohci, >>> - uint32_t addr, struct ohci_ed *ed) >>> + dma_addr_t addr, struct ohci_ed *ed) >>> { >>> /* ed->tail is under control of the HCD. >>> * Since just ed->head is changed by HC, just write back this >>> @@ -583,64 +584,63 @@ static inline int ohci_put_ed(OHCIState *ohci, >>> } >>> >>> static inline int ohci_put_td(OHCIState *ohci, >>> - uint32_t addr, struct ohci_td *td) >>> + dma_addr_t addr, struct ohci_td *td) >>> { >>> return put_dwords(ohci, addr, (uint32_t *)td, sizeof(*td)>> 2); >>> } >>> >>> static inline int ohci_put_iso_td(OHCIState *ohci, >>> - uint32_t addr, struct ohci_iso_td *td) >>> + dma_addr_t addr, struct ohci_iso_td *td) >>> { >>> return (put_dwords(ohci, addr, (uint32_t *)td, 4)&& >>> put_words(ohci, addr + 16, td->offset, 8)); >>> } >>> >>> static inline int ohci_put_hcca(OHCIState *ohci, >>> - uint32_t addr, struct ohci_hcca *hcca) >>> + dma_addr_t addr, struct ohci_hcca *hcca) >>> { >>> - cpu_physical_memory_write(addr + ohci->localmem_base + HCCA_WRITEBACK_OFFSET, >>> - (char *)hcca + HCCA_WRITEBACK_OFFSET, >>> - HCCA_WRITEBACK_SIZE); >>> + dma_memory_write(ohci->dma, >>> + addr + ohci->localmem_base + HCCA_WRITEBACK_OFFSET, >>> + (char *)hcca + HCCA_WRITEBACK_OFFSET, >>> + HCCA_WRITEBACK_SIZE); >>> return 1; >>> } >>> >>> /* Read/Write the contents of a TD from/to main memory. */ >>> static void ohci_copy_td(OHCIState *ohci, struct ohci_td *td, >>> - uint8_t *buf, int len, int write) >>> + uint8_t *buf, int len, DMADirection dir) >>> { >>> - uint32_t ptr; >>> - uint32_t n; >>> + dma_addr_t ptr, n; >>> >>> ptr = td->cbp; >>> n = 0x1000 - (ptr& 0xfff); >>> if (n> len) >>> n = len; >>> - cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, n, write); >>> + dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, n, dir); >>> if (n == len) >>> return; >>> ptr = td->be& ~0xfffu; >>> buf += n; >>> - cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, len - n, write); >>> + dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, len - n, dir); >>> } >>> >>> /* Read/Write the contents of an ISO TD from/to main memory. */ >>> static void ohci_copy_iso_td(OHCIState *ohci, >>> uint32_t start_addr, uint32_t end_addr, >>> - uint8_t *buf, int len, int write) >>> + uint8_t *buf, int len, DMADirection dir) >>> { >>> - uint32_t ptr; >>> - uint32_t n; >>> + dma_addr_t ptr, n; >>> >>> ptr = start_addr; >>> n = 0x1000 - (ptr& 0xfff); >>> if (n> len) >>> n = len; >>> - cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, n, write); >>> + dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, n, dir); >>> if (n == len) >>> return; >>> ptr = end_addr& ~0xfffu; >>> buf += n; >>> - cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, len - n, write); >>> + dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, len - n, dir); >>> } >>> >>> static void ohci_process_lists(OHCIState *ohci, int completion); >>> @@ -803,7 +803,8 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed, >>> } >>> >>> if (len&& dir != OHCI_TD_DIR_IN) { >>> - ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len, 0); >>> + ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len, >>> + DMA_DIRECTION_TO_DEVICE); >>> } >>> >>> if (completion) { >>> @@ -827,7 +828,8 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed, >>> /* Writeback */ >>> if (dir == OHCI_TD_DIR_IN&& ret>= 0&& ret<= len) { >>> /* IN transfer succeeded */ >>> - ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, ret, 1); >>> + ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, ret, >>> + DMA_DIRECTION_FROM_DEVICE); >>> OHCI_SET_BM(iso_td.offset[relative_frame_number], TD_PSW_CC, >>> OHCI_CC_NOERROR); >>> OHCI_SET_BM(iso_td.offset[relative_frame_number], TD_PSW_SIZE, ret); >>> @@ -971,7 +973,8 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed) >>> pktlen = len; >>> } >>> if (!completion) { >>> - ohci_copy_td(ohci,&td, ohci->usb_buf, pktlen, 0); >>> + ohci_copy_td(ohci,&td, ohci->usb_buf, pktlen, >>> + DMA_DIRECTION_TO_DEVICE); >>> } >>> } >>> } >>> @@ -1021,7 +1024,8 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed) >>> } >>> if (ret>= 0) { >>> if (dir == OHCI_TD_DIR_IN) { >>> - ohci_copy_td(ohci,&td, ohci->usb_buf, ret, 1); >>> + ohci_copy_td(ohci,&td, ohci->usb_buf, ret, >>> + DMA_DIRECTION_FROM_DEVICE); >>> #ifdef DEBUG_PACKET >>> DPRINTF(" data:"); >>> for (i = 0; i< ret; i++) >>> @@ -1748,11 +1752,14 @@ static USBBusOps ohci_bus_ops = { >>> }; >>> >>> static int usb_ohci_init(OHCIState *ohci, DeviceState *dev, >>> - int num_ports, uint32_t localmem_base, >>> - char *masterbus, uint32_t firstport) >>> + int num_ports, dma_addr_t localmem_base, >>> + char *masterbus, uint32_t firstport, >>> + DMAContext *dma) >>> { >>> int i; >>> >>> + ohci->dma = dma; >>> + >>> if (usb_frame_time == 0) { >>> #ifdef OHCI_TIME_WARP >>> usb_frame_time = get_ticks_per_sec(); >>> @@ -1817,7 +1824,8 @@ static int usb_ohci_initfn_pci(struct PCIDevice *dev) >>> ohci->pci_dev.config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */ >>> >>> if (usb_ohci_init(&ohci->state,&dev->qdev, ohci->num_ports, 0, >>> - ohci->masterbus, ohci->firstport) != 0) { >>> + ohci->masterbus, ohci->firstport, >>> + pci_dma_context(dev)) != 0) { >>> return -1; >>> } >>> ohci->state.irq = ohci->pci_dev.irq[0]; >>> @@ -1831,7 +1839,7 @@ typedef struct { >>> SysBusDevice busdev; >>> OHCIState ohci; >>> uint32_t num_ports; >>> - target_phys_addr_t dma_offset; >>> + dma_addr_t dma_offset; >>> } OHCISysBusState; >>> >>> static int ohci_init_pxa(SysBusDevice *dev) >>> @@ -1839,7 +1847,8 @@ static int ohci_init_pxa(SysBusDevice *dev) >>> OHCISysBusState *s = FROM_SYSBUS(OHCISysBusState, dev); >>> >>> /* Cannot fail as we pass NULL for masterbus */ >>> - usb_ohci_init(&s->ohci,&dev->qdev, s->num_ports, s->dma_offset, NULL, 0); >>> + usb_ohci_init(&s->ohci,&dev->qdev, s->num_ports, s->dma_offset, NULL, 0, >>> + NULL); >>> sysbus_init_irq(dev,&s->ohci.irq); >>> sysbus_init_mmio(dev,&s->ohci.mem); >>> >>> @@ -1875,7 +1884,7 @@ static TypeInfo ohci_pci_info = { >>> >>> static Property ohci_sysbus_properties[] = { >>> DEFINE_PROP_UINT32("num-ports", OHCISysBusState, num_ports, 3), >>> - DEFINE_PROP_TADDR("dma-offset", OHCISysBusState, dma_offset, 3), >>> + DEFINE_PROP_DMAADDR("dma-offset", OHCISysBusState, dma_offset, 3), >>> DEFINE_PROP_END_OF_LIST(), >>> }; >>> > >
On Wed, 2012-06-20 at 16:40 -0500, Anthony Liguori wrote: > Well let's return void in the DMA methods and let the IOMMUs assert on error. > At least that will avoid surprises until someone decides they care enough about > errors to touch all callers. > > I think silently failing a memcpy() can potentially lead to a vulnerability so > I'd rather avoid that. No I'd rather keep the error returns, really, even if that means fixing a few devices. I can look at making sure we don't pass random qemu data, on error that's reasonably easy. assert on error means guest code can assert qemu ... not a great idea but maybe we can add a warning. > >> Why leave pci accessors and not implement usb_memory_rw() wrappers? > > > > Well, "usb" is a bit too generic, ehci and ohci would each need to have > > their own sets of wrappers. But yes, that's possible... is it really > > worth it ? There's nothing fundamentally wrong with using the dma_* > > accessors. > > So is using the pci accessors wrong? Not really either, I don't think it matters :-) > I'm not saying you should go and convert every caller of the pci_ functions, I > just want a clear policy on what interface devices should use. Ideally the bus interface for the bus they sit on so they don't have to bother digging the DMAContext and are immune to change we would do in that area. Devices that mix multiple bus types however are a bit more tricky, but so far are few, and those can use dma_* and know where to get the DMAContext from. If we ever replace DMAContext with something else we can probably just change the field to that "something else" with a very simple search/replace on those devices (at least that's the best case :-) I think anything else is just no worth bothering. Cheers, Ben. > Regards, > > Anthony Liguori > > > > > Cheers, > > Ben. > > > >> Regards, > >> > >> Anthony Liguori > >> > >>> --- > >>> hw/usb/hcd-ohci.c | 93 +++++++++++++++++++++++++++++------------------------ > >>> 1 file changed, 51 insertions(+), 42 deletions(-) > >>> > >>> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c > >>> index 1a1cc88..844e7ed 100644 > >>> --- a/hw/usb/hcd-ohci.c > >>> +++ b/hw/usb/hcd-ohci.c > >>> @@ -31,7 +31,7 @@ > >>> #include "hw/usb.h" > >>> #include "hw/pci.h" > >>> #include "hw/sysbus.h" > >>> -#include "hw/qdev-addr.h" > >>> +#include "hw/qdev-dma.h" > >>> > >>> //#define DEBUG_OHCI > >>> /* Dump packet contents. */ > >>> @@ -62,6 +62,7 @@ typedef struct { > >>> USBBus bus; > >>> qemu_irq irq; > >>> MemoryRegion mem; > >>> + DMAContext *dma; > >>> int num_ports; > >>> const char *name; > >>> > >>> @@ -104,7 +105,7 @@ typedef struct { > >>> uint32_t htest; > >>> > >>> /* SM501 local memory offset */ > >>> - target_phys_addr_t localmem_base; > >>> + dma_addr_t localmem_base; > >>> > >>> /* Active packets. */ > >>> uint32_t old_ctl; > >>> @@ -482,14 +483,14 @@ static void ohci_reset(void *opaque) > >>> > >>> /* Get an array of dwords from main memory */ > >>> static inline int get_dwords(OHCIState *ohci, > >>> - uint32_t addr, uint32_t *buf, int num) > >>> + dma_addr_t addr, uint32_t *buf, int num) > >>> { > >>> int i; > >>> > >>> addr += ohci->localmem_base; > >>> > >>> for (i = 0; i< num; i++, buf++, addr += sizeof(*buf)) { > >>> - cpu_physical_memory_read(addr, buf, sizeof(*buf)); > >>> + dma_memory_read(ohci->dma, addr, buf, sizeof(*buf)); > >>> *buf = le32_to_cpu(*buf); > >>> } > >>> > >>> @@ -498,7 +499,7 @@ static inline int get_dwords(OHCIState *ohci, > >>> > >>> /* Put an array of dwords in to main memory */ > >>> static inline int put_dwords(OHCIState *ohci, > >>> - uint32_t addr, uint32_t *buf, int num) > >>> + dma_addr_t addr, uint32_t *buf, int num) > >>> { > >>> int i; > >>> > >>> @@ -506,7 +507,7 @@ static inline int put_dwords(OHCIState *ohci, > >>> > >>> for (i = 0; i< num; i++, buf++, addr += sizeof(*buf)) { > >>> uint32_t tmp = cpu_to_le32(*buf); > >>> - cpu_physical_memory_write(addr,&tmp, sizeof(tmp)); > >>> + dma_memory_write(ohci->dma, addr,&tmp, sizeof(tmp)); > >>> } > >>> > >>> return 1; > >>> @@ -514,14 +515,14 @@ static inline int put_dwords(OHCIState *ohci, > >>> > >>> /* Get an array of words from main memory */ > >>> static inline int get_words(OHCIState *ohci, > >>> - uint32_t addr, uint16_t *buf, int num) > >>> + dma_addr_t addr, uint16_t *buf, int num) > >>> { > >>> int i; > >>> > >>> addr += ohci->localmem_base; > >>> > >>> for (i = 0; i< num; i++, buf++, addr += sizeof(*buf)) { > >>> - cpu_physical_memory_read(addr, buf, sizeof(*buf)); > >>> + dma_memory_read(ohci->dma, addr, buf, sizeof(*buf)); > >>> *buf = le16_to_cpu(*buf); > >>> } > >>> > >>> @@ -530,7 +531,7 @@ static inline int get_words(OHCIState *ohci, > >>> > >>> /* Put an array of words in to main memory */ > >>> static inline int put_words(OHCIState *ohci, > >>> - uint32_t addr, uint16_t *buf, int num) > >>> + dma_addr_t addr, uint16_t *buf, int num) > >>> { > >>> int i; > >>> > >>> @@ -538,40 +539,40 @@ static inline int put_words(OHCIState *ohci, > >>> > >>> for (i = 0; i< num; i++, buf++, addr += sizeof(*buf)) { > >>> uint16_t tmp = cpu_to_le16(*buf); > >>> - cpu_physical_memory_write(addr,&tmp, sizeof(tmp)); > >>> + dma_memory_write(ohci->dma, addr,&tmp, sizeof(tmp)); > >>> } > >>> > >>> return 1; > >>> } > >>> > >>> static inline int ohci_read_ed(OHCIState *ohci, > >>> - uint32_t addr, struct ohci_ed *ed) > >>> + dma_addr_t addr, struct ohci_ed *ed) > >>> { > >>> return get_dwords(ohci, addr, (uint32_t *)ed, sizeof(*ed)>> 2); > >>> } > >>> > >>> static inline int ohci_read_td(OHCIState *ohci, > >>> - uint32_t addr, struct ohci_td *td) > >>> + dma_addr_t addr, struct ohci_td *td) > >>> { > >>> return get_dwords(ohci, addr, (uint32_t *)td, sizeof(*td)>> 2); > >>> } > >>> > >>> static inline int ohci_read_iso_td(OHCIState *ohci, > >>> - uint32_t addr, struct ohci_iso_td *td) > >>> + dma_addr_t addr, struct ohci_iso_td *td) > >>> { > >>> return (get_dwords(ohci, addr, (uint32_t *)td, 4)&& > >>> get_words(ohci, addr + 16, td->offset, 8)); > >>> } > >>> > >>> static inline int ohci_read_hcca(OHCIState *ohci, > >>> - uint32_t addr, struct ohci_hcca *hcca) > >>> + dma_addr_t addr, struct ohci_hcca *hcca) > >>> { > >>> - cpu_physical_memory_read(addr + ohci->localmem_base, hcca, sizeof(*hcca)); > >>> + dma_memory_read(ohci->dma, addr + ohci->localmem_base, hcca, sizeof(*hcca)); > >>> return 1; > >>> } > >>> > >>> static inline int ohci_put_ed(OHCIState *ohci, > >>> - uint32_t addr, struct ohci_ed *ed) > >>> + dma_addr_t addr, struct ohci_ed *ed) > >>> { > >>> /* ed->tail is under control of the HCD. > >>> * Since just ed->head is changed by HC, just write back this > >>> @@ -583,64 +584,63 @@ static inline int ohci_put_ed(OHCIState *ohci, > >>> } > >>> > >>> static inline int ohci_put_td(OHCIState *ohci, > >>> - uint32_t addr, struct ohci_td *td) > >>> + dma_addr_t addr, struct ohci_td *td) > >>> { > >>> return put_dwords(ohci, addr, (uint32_t *)td, sizeof(*td)>> 2); > >>> } > >>> > >>> static inline int ohci_put_iso_td(OHCIState *ohci, > >>> - uint32_t addr, struct ohci_iso_td *td) > >>> + dma_addr_t addr, struct ohci_iso_td *td) > >>> { > >>> return (put_dwords(ohci, addr, (uint32_t *)td, 4)&& > >>> put_words(ohci, addr + 16, td->offset, 8)); > >>> } > >>> > >>> static inline int ohci_put_hcca(OHCIState *ohci, > >>> - uint32_t addr, struct ohci_hcca *hcca) > >>> + dma_addr_t addr, struct ohci_hcca *hcca) > >>> { > >>> - cpu_physical_memory_write(addr + ohci->localmem_base + HCCA_WRITEBACK_OFFSET, > >>> - (char *)hcca + HCCA_WRITEBACK_OFFSET, > >>> - HCCA_WRITEBACK_SIZE); > >>> + dma_memory_write(ohci->dma, > >>> + addr + ohci->localmem_base + HCCA_WRITEBACK_OFFSET, > >>> + (char *)hcca + HCCA_WRITEBACK_OFFSET, > >>> + HCCA_WRITEBACK_SIZE); > >>> return 1; > >>> } > >>> > >>> /* Read/Write the contents of a TD from/to main memory. */ > >>> static void ohci_copy_td(OHCIState *ohci, struct ohci_td *td, > >>> - uint8_t *buf, int len, int write) > >>> + uint8_t *buf, int len, DMADirection dir) > >>> { > >>> - uint32_t ptr; > >>> - uint32_t n; > >>> + dma_addr_t ptr, n; > >>> > >>> ptr = td->cbp; > >>> n = 0x1000 - (ptr& 0xfff); > >>> if (n> len) > >>> n = len; > >>> - cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, n, write); > >>> + dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, n, dir); > >>> if (n == len) > >>> return; > >>> ptr = td->be& ~0xfffu; > >>> buf += n; > >>> - cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, len - n, write); > >>> + dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, len - n, dir); > >>> } > >>> > >>> /* Read/Write the contents of an ISO TD from/to main memory. */ > >>> static void ohci_copy_iso_td(OHCIState *ohci, > >>> uint32_t start_addr, uint32_t end_addr, > >>> - uint8_t *buf, int len, int write) > >>> + uint8_t *buf, int len, DMADirection dir) > >>> { > >>> - uint32_t ptr; > >>> - uint32_t n; > >>> + dma_addr_t ptr, n; > >>> > >>> ptr = start_addr; > >>> n = 0x1000 - (ptr& 0xfff); > >>> if (n> len) > >>> n = len; > >>> - cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, n, write); > >>> + dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, n, dir); > >>> if (n == len) > >>> return; > >>> ptr = end_addr& ~0xfffu; > >>> buf += n; > >>> - cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, len - n, write); > >>> + dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, len - n, dir); > >>> } > >>> > >>> static void ohci_process_lists(OHCIState *ohci, int completion); > >>> @@ -803,7 +803,8 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed, > >>> } > >>> > >>> if (len&& dir != OHCI_TD_DIR_IN) { > >>> - ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len, 0); > >>> + ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len, > >>> + DMA_DIRECTION_TO_DEVICE); > >>> } > >>> > >>> if (completion) { > >>> @@ -827,7 +828,8 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed, > >>> /* Writeback */ > >>> if (dir == OHCI_TD_DIR_IN&& ret>= 0&& ret<= len) { > >>> /* IN transfer succeeded */ > >>> - ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, ret, 1); > >>> + ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, ret, > >>> + DMA_DIRECTION_FROM_DEVICE); > >>> OHCI_SET_BM(iso_td.offset[relative_frame_number], TD_PSW_CC, > >>> OHCI_CC_NOERROR); > >>> OHCI_SET_BM(iso_td.offset[relative_frame_number], TD_PSW_SIZE, ret); > >>> @@ -971,7 +973,8 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed) > >>> pktlen = len; > >>> } > >>> if (!completion) { > >>> - ohci_copy_td(ohci,&td, ohci->usb_buf, pktlen, 0); > >>> + ohci_copy_td(ohci,&td, ohci->usb_buf, pktlen, > >>> + DMA_DIRECTION_TO_DEVICE); > >>> } > >>> } > >>> } > >>> @@ -1021,7 +1024,8 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed) > >>> } > >>> if (ret>= 0) { > >>> if (dir == OHCI_TD_DIR_IN) { > >>> - ohci_copy_td(ohci,&td, ohci->usb_buf, ret, 1); > >>> + ohci_copy_td(ohci,&td, ohci->usb_buf, ret, > >>> + DMA_DIRECTION_FROM_DEVICE); > >>> #ifdef DEBUG_PACKET > >>> DPRINTF(" data:"); > >>> for (i = 0; i< ret; i++) > >>> @@ -1748,11 +1752,14 @@ static USBBusOps ohci_bus_ops = { > >>> }; > >>> > >>> static int usb_ohci_init(OHCIState *ohci, DeviceState *dev, > >>> - int num_ports, uint32_t localmem_base, > >>> - char *masterbus, uint32_t firstport) > >>> + int num_ports, dma_addr_t localmem_base, > >>> + char *masterbus, uint32_t firstport, > >>> + DMAContext *dma) > >>> { > >>> int i; > >>> > >>> + ohci->dma = dma; > >>> + > >>> if (usb_frame_time == 0) { > >>> #ifdef OHCI_TIME_WARP > >>> usb_frame_time = get_ticks_per_sec(); > >>> @@ -1817,7 +1824,8 @@ static int usb_ohci_initfn_pci(struct PCIDevice *dev) > >>> ohci->pci_dev.config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */ > >>> > >>> if (usb_ohci_init(&ohci->state,&dev->qdev, ohci->num_ports, 0, > >>> - ohci->masterbus, ohci->firstport) != 0) { > >>> + ohci->masterbus, ohci->firstport, > >>> + pci_dma_context(dev)) != 0) { > >>> return -1; > >>> } > >>> ohci->state.irq = ohci->pci_dev.irq[0]; > >>> @@ -1831,7 +1839,7 @@ typedef struct { > >>> SysBusDevice busdev; > >>> OHCIState ohci; > >>> uint32_t num_ports; > >>> - target_phys_addr_t dma_offset; > >>> + dma_addr_t dma_offset; > >>> } OHCISysBusState; > >>> > >>> static int ohci_init_pxa(SysBusDevice *dev) > >>> @@ -1839,7 +1847,8 @@ static int ohci_init_pxa(SysBusDevice *dev) > >>> OHCISysBusState *s = FROM_SYSBUS(OHCISysBusState, dev); > >>> > >>> /* Cannot fail as we pass NULL for masterbus */ > >>> - usb_ohci_init(&s->ohci,&dev->qdev, s->num_ports, s->dma_offset, NULL, 0); > >>> + usb_ohci_init(&s->ohci,&dev->qdev, s->num_ports, s->dma_offset, NULL, 0, > >>> + NULL); > >>> sysbus_init_irq(dev,&s->ohci.irq); > >>> sysbus_init_mmio(dev,&s->ohci.mem); > >>> > >>> @@ -1875,7 +1884,7 @@ static TypeInfo ohci_pci_info = { > >>> > >>> static Property ohci_sysbus_properties[] = { > >>> DEFINE_PROP_UINT32("num-ports", OHCISysBusState, num_ports, 3), > >>> - DEFINE_PROP_TADDR("dma-offset", OHCISysBusState, dma_offset, 3), > >>> + DEFINE_PROP_DMAADDR("dma-offset", OHCISysBusState, dma_offset, 3), > >>> DEFINE_PROP_END_OF_LIST(), > >>> }; > >>> > > > >
Hi, >>> Why leave pci accessors and not implement usb_memory_rw() wrappers? >> >> Well, "usb" is a bit too generic, ehci and ohci would each need to have >> their own sets of wrappers. But yes, that's possible... is it really >> worth it ? There's nothing fundamentally wrong with using the dma_* >> accessors. > > So is using the pci accessors wrong? > > I'm not saying you should go and convert every caller of the pci_ > functions, I just want a clear policy on what interface devices should use. usb device emulations should use usb_packet_copy() usb host adapters emulations should use either usb_packet_map() + usb_packet_unmap(), or use usb_packet_addbuf(), then copy from/to the buffer using whatever is approximate. For pci host controller that is pci_memory_rw(). For ohci which exists in both pci and non-pci variants it looks reasonable to me to get a iommu handle in bus-specific code, then use dma_memory_rw with that handle directly in the common code paths. cheers, Gerd
On Thu, Jun 21, 2012 at 08:02:06AM +1000, Benjamin Herrenschmidt wrote: > On Wed, 2012-06-20 at 16:40 -0500, Anthony Liguori wrote: > > > Well let's return void in the DMA methods and let the IOMMUs assert on error. > > At least that will avoid surprises until someone decides they care enough about > > errors to touch all callers. > > > > I think silently failing a memcpy() can potentially lead to a vulnerability so > > I'd rather avoid that. > > No I'd rather keep the error returns, really, even if that means fixing > a few devices. I can look at making sure we don't pass random qemu data, > on error that's reasonably easy. > > assert on error means guest code can assert qemu ... not a great idea > but maybe we can add a warning. Why not? Guest can always just halt if it wants to anyway. On the other hand, warnings can fill up host logs so represent a security problem. > > >> Why leave pci accessors and not implement usb_memory_rw() wrappers? > > > > > > Well, "usb" is a bit too generic, ehci and ohci would each need to have > > > their own sets of wrappers. But yes, that's possible... is it really > > > worth it ? There's nothing fundamentally wrong with using the dma_* > > > accessors. > > > > So is using the pci accessors wrong? > > Not really either, I don't think it matters :-) > > > I'm not saying you should go and convert every caller of the pci_ functions, I > > just want a clear policy on what interface devices should use. > > Ideally the bus interface for the bus they sit on so they don't have to > bother digging the DMAContext and are immune to change we would do in > that area. > > Devices that mix multiple bus types however are a bit more tricky, but > so far are few, and those can use dma_* and know where to get the > DMAContext from. > > If we ever replace DMAContext with something else we can probably just > change the field to that "something else" with a very simple > search/replace on those devices (at least that's the best case :-) > > I think anything else is just no worth bothering. > > Cheers, > Ben.
On 06/21/2012 02:33 AM, Michael S. Tsirkin wrote: > On Thu, Jun 21, 2012 at 08:02:06AM +1000, Benjamin Herrenschmidt wrote: >> On Wed, 2012-06-20 at 16:40 -0500, Anthony Liguori wrote: >> >>> Well let's return void in the DMA methods and let the IOMMUs assert on error. >>> At least that will avoid surprises until someone decides they care enough about >>> errors to touch all callers. >>> >>> I think silently failing a memcpy() can potentially lead to a vulnerability so >>> I'd rather avoid that. >> >> No I'd rather keep the error returns, really, even if that means fixing >> a few devices. I can look at making sure we don't pass random qemu data, >> on error that's reasonably easy. >> >> assert on error means guest code can assert qemu ... not a great idea >> but maybe we can add a warning. > > Why not? Guest can always just halt if it wants to anyway. > On the other hand, warnings can fill up host logs so > represent a security problem. As long as we scrub the buffers, returning an unhandled error seems okay to me. I've long thought we should have some sort of generic way to throw an error and effectively pause a single device. I'm not sure how it would work in practice though. Regards, Anthony Liguori
On Thu, Jun 21, 2012 at 07:55:58AM -0500, Anthony Liguori wrote: > On 06/21/2012 02:33 AM, Michael S. Tsirkin wrote: > >On Thu, Jun 21, 2012 at 08:02:06AM +1000, Benjamin Herrenschmidt wrote: > >>On Wed, 2012-06-20 at 16:40 -0500, Anthony Liguori wrote: > >> > >>>Well let's return void in the DMA methods and let the IOMMUs assert on error. > >>>At least that will avoid surprises until someone decides they care enough about > >>>errors to touch all callers. > >>> > >>>I think silently failing a memcpy() can potentially lead to a vulnerability so > >>>I'd rather avoid that. > >> > >>No I'd rather keep the error returns, really, even if that means fixing > >>a few devices. I can look at making sure we don't pass random qemu data, > >>on error that's reasonably easy. > >> > >>assert on error means guest code can assert qemu ... not a great idea > >>but maybe we can add a warning. > > > >Why not? Guest can always just halt if it wants to anyway. > >On the other hand, warnings can fill up host logs so > >represent a security problem. > > As long as we scrub the buffers, returning an unhandled error seems okay to me. > > I've long thought we should have some sort of generic way to throw > an error and effectively pause a single device. I'm not sure how it > would work in practice though. > > Regards, > > Anthony Liguori I think we should add an API to log a message and pause the VM. Later admin can resume the VM, save it to file for debugging etc.
On Thu, 2012-06-21 at 07:55 -0500, Anthony Liguori wrote: > As long as we scrub the buffers, returning an unhandled error seems okay to me. > > I've long thought we should have some sort of generic way to throw an error and > effectively pause a single device. I'm not sure how it would work in practice > though. I'll scrub the buffer in dma_memory_rw when reading from the guest in all failure cases to ensure that we don't use a buffer of stale qemu data. In addition I'm doing a separate patch that I'll post later (needs a bit more time) that improves OHCI's error handling. Cheers, Ben.
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c index 1a1cc88..844e7ed 100644 --- a/hw/usb/hcd-ohci.c +++ b/hw/usb/hcd-ohci.c @@ -31,7 +31,7 @@ #include "hw/usb.h" #include "hw/pci.h" #include "hw/sysbus.h" -#include "hw/qdev-addr.h" +#include "hw/qdev-dma.h" //#define DEBUG_OHCI /* Dump packet contents. */ @@ -62,6 +62,7 @@ typedef struct { USBBus bus; qemu_irq irq; MemoryRegion mem; + DMAContext *dma; int num_ports; const char *name; @@ -104,7 +105,7 @@ typedef struct { uint32_t htest; /* SM501 local memory offset */ - target_phys_addr_t localmem_base; + dma_addr_t localmem_base; /* Active packets. */ uint32_t old_ctl; @@ -482,14 +483,14 @@ static void ohci_reset(void *opaque) /* Get an array of dwords from main memory */ static inline int get_dwords(OHCIState *ohci, - uint32_t addr, uint32_t *buf, int num) + dma_addr_t addr, uint32_t *buf, int num) { int i; addr += ohci->localmem_base; for (i = 0; i < num; i++, buf++, addr += sizeof(*buf)) { - cpu_physical_memory_read(addr, buf, sizeof(*buf)); + dma_memory_read(ohci->dma, addr, buf, sizeof(*buf)); *buf = le32_to_cpu(*buf); } @@ -498,7 +499,7 @@ static inline int get_dwords(OHCIState *ohci, /* Put an array of dwords in to main memory */ static inline int put_dwords(OHCIState *ohci, - uint32_t addr, uint32_t *buf, int num) + dma_addr_t addr, uint32_t *buf, int num) { int i; @@ -506,7 +507,7 @@ static inline int put_dwords(OHCIState *ohci, for (i = 0; i < num; i++, buf++, addr += sizeof(*buf)) { uint32_t tmp = cpu_to_le32(*buf); - cpu_physical_memory_write(addr, &tmp, sizeof(tmp)); + dma_memory_write(ohci->dma, addr, &tmp, sizeof(tmp)); } return 1; @@ -514,14 +515,14 @@ static inline int put_dwords(OHCIState *ohci, /* Get an array of words from main memory */ static inline int get_words(OHCIState *ohci, - uint32_t addr, uint16_t *buf, int num) + dma_addr_t addr, uint16_t *buf, int num) { int i; addr += ohci->localmem_base; for (i = 0; i < num; i++, buf++, addr += sizeof(*buf)) { - cpu_physical_memory_read(addr, buf, sizeof(*buf)); + dma_memory_read(ohci->dma, addr, buf, sizeof(*buf)); *buf = le16_to_cpu(*buf); } @@ -530,7 +531,7 @@ static inline int get_words(OHCIState *ohci, /* Put an array of words in to main memory */ static inline int put_words(OHCIState *ohci, - uint32_t addr, uint16_t *buf, int num) + dma_addr_t addr, uint16_t *buf, int num) { int i; @@ -538,40 +539,40 @@ static inline int put_words(OHCIState *ohci, for (i = 0; i < num; i++, buf++, addr += sizeof(*buf)) { uint16_t tmp = cpu_to_le16(*buf); - cpu_physical_memory_write(addr, &tmp, sizeof(tmp)); + dma_memory_write(ohci->dma, addr, &tmp, sizeof(tmp)); } return 1; } static inline int ohci_read_ed(OHCIState *ohci, - uint32_t addr, struct ohci_ed *ed) + dma_addr_t addr, struct ohci_ed *ed) { return get_dwords(ohci, addr, (uint32_t *)ed, sizeof(*ed) >> 2); } static inline int ohci_read_td(OHCIState *ohci, - uint32_t addr, struct ohci_td *td) + dma_addr_t addr, struct ohci_td *td) { return get_dwords(ohci, addr, (uint32_t *)td, sizeof(*td) >> 2); } static inline int ohci_read_iso_td(OHCIState *ohci, - uint32_t addr, struct ohci_iso_td *td) + dma_addr_t addr, struct ohci_iso_td *td) { return (get_dwords(ohci, addr, (uint32_t *)td, 4) && get_words(ohci, addr + 16, td->offset, 8)); } static inline int ohci_read_hcca(OHCIState *ohci, - uint32_t addr, struct ohci_hcca *hcca) + dma_addr_t addr, struct ohci_hcca *hcca) { - cpu_physical_memory_read(addr + ohci->localmem_base, hcca, sizeof(*hcca)); + dma_memory_read(ohci->dma, addr + ohci->localmem_base, hcca, sizeof(*hcca)); return 1; } static inline int ohci_put_ed(OHCIState *ohci, - uint32_t addr, struct ohci_ed *ed) + dma_addr_t addr, struct ohci_ed *ed) { /* ed->tail is under control of the HCD. * Since just ed->head is changed by HC, just write back this @@ -583,64 +584,63 @@ static inline int ohci_put_ed(OHCIState *ohci, } static inline int ohci_put_td(OHCIState *ohci, - uint32_t addr, struct ohci_td *td) + dma_addr_t addr, struct ohci_td *td) { return put_dwords(ohci, addr, (uint32_t *)td, sizeof(*td) >> 2); } static inline int ohci_put_iso_td(OHCIState *ohci, - uint32_t addr, struct ohci_iso_td *td) + dma_addr_t addr, struct ohci_iso_td *td) { return (put_dwords(ohci, addr, (uint32_t *)td, 4) && put_words(ohci, addr + 16, td->offset, 8)); } static inline int ohci_put_hcca(OHCIState *ohci, - uint32_t addr, struct ohci_hcca *hcca) + dma_addr_t addr, struct ohci_hcca *hcca) { - cpu_physical_memory_write(addr + ohci->localmem_base + HCCA_WRITEBACK_OFFSET, - (char *)hcca + HCCA_WRITEBACK_OFFSET, - HCCA_WRITEBACK_SIZE); + dma_memory_write(ohci->dma, + addr + ohci->localmem_base + HCCA_WRITEBACK_OFFSET, + (char *)hcca + HCCA_WRITEBACK_OFFSET, + HCCA_WRITEBACK_SIZE); return 1; } /* Read/Write the contents of a TD from/to main memory. */ static void ohci_copy_td(OHCIState *ohci, struct ohci_td *td, - uint8_t *buf, int len, int write) + uint8_t *buf, int len, DMADirection dir) { - uint32_t ptr; - uint32_t n; + dma_addr_t ptr, n; ptr = td->cbp; n = 0x1000 - (ptr & 0xfff); if (n > len) n = len; - cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, n, write); + dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, n, dir); if (n == len) return; ptr = td->be & ~0xfffu; buf += n; - cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, len - n, write); + dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, len - n, dir); } /* Read/Write the contents of an ISO TD from/to main memory. */ static void ohci_copy_iso_td(OHCIState *ohci, uint32_t start_addr, uint32_t end_addr, - uint8_t *buf, int len, int write) + uint8_t *buf, int len, DMADirection dir) { - uint32_t ptr; - uint32_t n; + dma_addr_t ptr, n; ptr = start_addr; n = 0x1000 - (ptr & 0xfff); if (n > len) n = len; - cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, n, write); + dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, n, dir); if (n == len) return; ptr = end_addr & ~0xfffu; buf += n; - cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, len - n, write); + dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, len - n, dir); } static void ohci_process_lists(OHCIState *ohci, int completion); @@ -803,7 +803,8 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed, } if (len && dir != OHCI_TD_DIR_IN) { - ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len, 0); + ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len, + DMA_DIRECTION_TO_DEVICE); } if (completion) { @@ -827,7 +828,8 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed, /* Writeback */ if (dir == OHCI_TD_DIR_IN && ret >= 0 && ret <= len) { /* IN transfer succeeded */ - ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, ret, 1); + ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, ret, + DMA_DIRECTION_FROM_DEVICE); OHCI_SET_BM(iso_td.offset[relative_frame_number], TD_PSW_CC, OHCI_CC_NOERROR); OHCI_SET_BM(iso_td.offset[relative_frame_number], TD_PSW_SIZE, ret); @@ -971,7 +973,8 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed) pktlen = len; } if (!completion) { - ohci_copy_td(ohci, &td, ohci->usb_buf, pktlen, 0); + ohci_copy_td(ohci, &td, ohci->usb_buf, pktlen, + DMA_DIRECTION_TO_DEVICE); } } } @@ -1021,7 +1024,8 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed) } if (ret >= 0) { if (dir == OHCI_TD_DIR_IN) { - ohci_copy_td(ohci, &td, ohci->usb_buf, ret, 1); + ohci_copy_td(ohci, &td, ohci->usb_buf, ret, + DMA_DIRECTION_FROM_DEVICE); #ifdef DEBUG_PACKET DPRINTF(" data:"); for (i = 0; i < ret; i++) @@ -1748,11 +1752,14 @@ static USBBusOps ohci_bus_ops = { }; static int usb_ohci_init(OHCIState *ohci, DeviceState *dev, - int num_ports, uint32_t localmem_base, - char *masterbus, uint32_t firstport) + int num_ports, dma_addr_t localmem_base, + char *masterbus, uint32_t firstport, + DMAContext *dma) { int i; + ohci->dma = dma; + if (usb_frame_time == 0) { #ifdef OHCI_TIME_WARP usb_frame_time = get_ticks_per_sec(); @@ -1817,7 +1824,8 @@ static int usb_ohci_initfn_pci(struct PCIDevice *dev) ohci->pci_dev.config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */ if (usb_ohci_init(&ohci->state, &dev->qdev, ohci->num_ports, 0, - ohci->masterbus, ohci->firstport) != 0) { + ohci->masterbus, ohci->firstport, + pci_dma_context(dev)) != 0) { return -1; } ohci->state.irq = ohci->pci_dev.irq[0]; @@ -1831,7 +1839,7 @@ typedef struct { SysBusDevice busdev; OHCIState ohci; uint32_t num_ports; - target_phys_addr_t dma_offset; + dma_addr_t dma_offset; } OHCISysBusState; static int ohci_init_pxa(SysBusDevice *dev) @@ -1839,7 +1847,8 @@ static int ohci_init_pxa(SysBusDevice *dev) OHCISysBusState *s = FROM_SYSBUS(OHCISysBusState, dev); /* Cannot fail as we pass NULL for masterbus */ - usb_ohci_init(&s->ohci, &dev->qdev, s->num_ports, s->dma_offset, NULL, 0); + usb_ohci_init(&s->ohci, &dev->qdev, s->num_ports, s->dma_offset, NULL, 0, + NULL); sysbus_init_irq(dev, &s->ohci.irq); sysbus_init_mmio(dev, &s->ohci.mem); @@ -1875,7 +1884,7 @@ static TypeInfo ohci_pci_info = { static Property ohci_sysbus_properties[] = { DEFINE_PROP_UINT32("num-ports", OHCISysBusState, num_ports, 3), - DEFINE_PROP_TADDR("dma-offset", OHCISysBusState, dma_offset, 3), + DEFINE_PROP_DMAADDR("dma-offset", OHCISysBusState, dma_offset, 3), DEFINE_PROP_END_OF_LIST(), };