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

[Anthony Liguori <anthony@codemonkey.ws>]

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