Re: [Qemu-devel] [PATCH 04/13] usb-ohci: Use universal DMA helper functions

[Benjamin Herrenschmidt]

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(),
> >>>    };
> >>>
> >
> >