Re: [PATCH 01/13] Generic DMA memory access interface

[Blue Swirl <blauwirbel@xxxxxxxxx>]

On Thu, Feb 3, 2011 at 11:32 PM, Eduard - Gabriel Munteanu
<eduard.munteanu@xxxxxxxxxxx> wrote:
> This introduces replacements for memory access functions like
> cpu_physical_memory_read(). The new interface can handle address
> translation and access checking through an IOMMU.
>
> Signed-off-by: Eduard - Gabriel Munteanu <eduard.munteanu@xxxxxxxxxxx>
> ---
> ÂMakefile.target | Â Â2 +-
> Âhw/dma_rw.c   | Â124 +++++++++++++++++++++++++++++++++++++++++++
> Âhw/dma_rw.h   | Â157 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> Â3 files changed, 282 insertions(+), 1 deletions(-)
> Âcreate mode 100644 hw/dma_rw.c
> Âcreate mode 100644 hw/dma_rw.h
>
> diff --git a/Makefile.target b/Makefile.target
> index e15b1c4..e5817ab 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -218,7 +218,7 @@ obj-i386-y += cirrus_vga.o apic.o ioapic.o piix_pci.o
> Âobj-i386-y += vmmouse.o vmport.o hpet.o applesmc.o
> Âobj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
> Âobj-i386-y += debugcon.o multiboot.o
> -obj-i386-y += pc_piix.o
> +obj-i386-y += pc_piix.o dma_rw.o
> Âobj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>
> Â# shared objects
> diff --git a/hw/dma_rw.c b/hw/dma_rw.c
> new file mode 100644
> index 0000000..ef8e7f8
> --- /dev/null
> +++ b/hw/dma_rw.c
> @@ -0,0 +1,124 @@
> +/*
> + * Generic DMA memory access interface.
> + *
> + * Copyright (c) 2011 Eduard - Gabriel Munteanu
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "dma_rw.h"
> +#include "range.h"
> +
> +static void dma_register_memory_map(DMADevice *dev,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âdma_addr_t addr,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âdma_addr_t len,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âtarget_phys_addr_t paddr,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂDMAInvalidateMapFunc *invalidate,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âvoid *invalidate_opaque)
> +{
> + Â ÂDMAMemoryMap *map;
> +
> + Â Âmap = qemu_malloc(sizeof(DMAMemoryMap));
> +  Âmap->addr        = addr;
> +  Âmap->len        Â= len;
> +  Âmap->paddr       Â= paddr;
> +  Âmap->invalidate     = invalidate;
> + Â Âmap->invalidate_opaque Â= invalidate_opaque;
> +
> + Â ÂQLIST_INSERT_HEAD(&dev->mmu->memory_maps, map, list);
> +}
> +
> +static void dma_unregister_memory_map(DMADevice *dev,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âtarget_phys_addr_t paddr,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âdma_addr_t len)
> +{
> + Â ÂDMAMemoryMap *map;
> +
> + Â ÂQLIST_FOREACH(map, &dev->mmu->memory_maps, list) {
> + Â Â Â Âif (map->paddr == paddr && map->len == len) {
> + Â Â Â Â Â ÂQLIST_REMOVE(map, list);
> + Â Â Â Â Â Âfree(map);
> + Â Â Â Â}
> + Â Â}
> +}
> +
> +void dma_invalidate_memory_range(DMADevice *dev,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dma_addr_t addr,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dma_addr_t len)
> +{
> + Â ÂDMAMemoryMap *map;
> +
> + Â ÂQLIST_FOREACH(map, &dev->mmu->memory_maps, list) {
> + Â Â Â Âif (ranges_overlap(addr, len, map->addr, map->len)) {
> + Â Â Â Â Â Âmap->invalidate(map->invalidate_opaque);
> + Â Â Â Â Â ÂQLIST_REMOVE(map, list);
> + Â Â Â Â Â Âfree(map);
> + Â Â Â Â}
> + Â Â}
> +}
> +
> +void *dma_memory_map(DMADevice *dev,
> + Â Â Â Â Â Â Â Â Â Â DMAInvalidateMapFunc *cb,
> + Â Â Â Â Â Â Â Â Â Â void *opaque,
> + Â Â Â Â Â Â Â Â Â Â dma_addr_t addr,
> + Â Â Â Â Â Â Â Â Â Â dma_addr_t *len,
> + Â Â Â Â Â Â Â Â Â Â int is_write)
> +{
> + Â Âint err;
> + Â Âtarget_phys_addr_t paddr, plen;
> +
> + Â Âif (!dev || !dev->mmu) {
> + Â Â Â Âreturn cpu_physical_memory_map(addr, len, is_write);
> + Â Â}
> +
> + Â Âplen = *len;
> + Â Âerr = dev->mmu->translate(dev, addr, &paddr, &plen, is_write);
> + Â Âif (err) {
> + Â Â Â Âreturn NULL;
> + Â Â}
> +
> + Â Â/*
> + Â Â * If this is true, the virtual region is contiguous,
> + Â Â * but the translated physical region isn't. We just
> + Â Â * clamp *len, much like cpu_physical_memory_map() does.
> + Â Â */
> + Â Âif (plen < *len) {
> + Â Â Â Â*len = plen;
> + Â Â}
> +
> + Â Â/* We treat maps as remote TLBs to cope with stuff like AIO. */
> + Â Âif (cb) {
> + Â Â Â Âdma_register_memory_map(dev, addr, *len, paddr, cb, opaque);
> + Â Â}
> +
> + Â Âreturn cpu_physical_memory_map(paddr, len, is_write);
> +}
> +
> +void dma_memory_unmap(DMADevice *dev,
> + Â Â Â Â Â Â Â Â Â Â Âvoid *buffer,
> + Â Â Â Â Â Â Â Â Â Â Âdma_addr_t len,
> + Â Â Â Â Â Â Â Â Â Â Âint is_write,
> + Â Â Â Â Â Â Â Â Â Â Âdma_addr_t access_len)
> +{
> + Â Âcpu_physical_memory_unmap(buffer, len, is_write, access_len);
> + Â Âif (dev && dev->mmu) {
> + Â Â Â Âdma_unregister_memory_map(dev, (target_phys_addr_t) buffer, len);
> + Â Â}
> +}
> +
> diff --git a/hw/dma_rw.h b/hw/dma_rw.h
> new file mode 100644
> index 0000000..bc93511
> --- /dev/null
> +++ b/hw/dma_rw.h
> @@ -0,0 +1,157 @@
> +#ifndef DMA_RW_H
> +#define DMA_RW_H
> +
> +#include "qemu-common.h"
> +
> +typedef uint64_t dma_addr_t;
> +
> +typedef struct DMAMmu DMAMmu;
> +typedef struct DMADevice DMADevice;
> +typedef struct DMAMemoryMap DMAMemoryMap;
> +
> +typedef int DMATranslateFunc(DMADevice *dev,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â dma_addr_t addr,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â dma_addr_t *paddr,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â dma_addr_t *len,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â int is_write);
> +
> +typedef void DMAInvalidateMapFunc(void *);
> +
> +struct DMAMmu {
> + Â ÂDeviceState *iommu;
> + Â ÂDMATranslateFunc *translate;
> + Â ÂQLIST_HEAD(memory_maps, DMAMemoryMap) memory_maps;
> +};
> +
> +struct DMADevice {
> + Â ÂDMAMmu *mmu;
> +};
> +
> +struct DMAMemoryMap {
> +  Âdma_addr_t       Âaddr;
> +  Âdma_addr_t       Âlen;
> +  Âtarget_phys_addr_t   Âpaddr;
> +  ÂDMAInvalidateMapFunc  Â*invalidate;
> +  Âvoid          Â*invalidate_opaque;
> +
> + Â ÂQLIST_ENTRY(DMAMemoryMap) list;
> +};
> +
> +static inline void dma_memory_rw(DMADevice *dev,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dma_addr_t addr,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â void *buf,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dma_addr_t len,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â int is_write)
> +{
> + Â Âdma_addr_t paddr, plen;
> + Â Âint err;
> +
> + Â Â/*
> + Â Â * Fast-path non-iommu.
> + Â Â * More importantly, makes it obvious what this function does.
> + Â Â */
> + Â Âif (!dev || !dev->mmu) {
> + Â Â Â Âcpu_physical_memory_rw(addr, buf, plen, is_write);
> + Â Â Â Âreturn;
> + Â Â}
> +
> + Â Âwhile (len) {
> + Â Â Â Âerr = dev->mmu->translate(dev, addr, &paddr, &plen, is_write);
> + Â Â Â Âif (err) {
> + Â Â Â Â Â Âreturn;
> + Â Â Â Â}
> +
> + Â Â Â Â/* The translation might be valid for larger regions. */
> + Â Â Â Âif (plen > len) {
> + Â Â Â Â Â Âplen = len;
> + Â Â Â Â}
> +
> + Â Â Â Âcpu_physical_memory_rw(paddr, buf, plen, is_write);
> +
> + Â Â Â Âlen -= plen;
> + Â Â Â Âaddr += plen;
> + Â Â Â Âbuf += plen;
> + Â Â}
> +}
> +
> +static inline void dma_memory_read(DMADevice *dev,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dma_addr_t addr,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â void *buf,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dma_addr_t len)
> +{
> + Â Âdma_memory_rw(dev, addr, buf, len, 0);
> +}
> +
> +static inline void dma_memory_write(DMADevice *dev,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âdma_addr_t addr,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âconst void *buf,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âdma_addr_t len)
> +{
> + Â Âdma_memory_rw(dev, addr, (void *) buf, len, 1);
> +}
> +
> +void *dma_memory_map(DMADevice *dev,
> + Â Â Â Â Â Â Â Â Â Â DMAInvalidateMapFunc *cb,
> + Â Â Â Â Â Â Â Â Â Â void *opaque,
> + Â Â Â Â Â Â Â Â Â Â dma_addr_t addr,
> + Â Â Â Â Â Â Â Â Â Â dma_addr_t *len,
> + Â Â Â Â Â Â Â Â Â Â int is_write);
> +void dma_memory_unmap(DMADevice *dev,
> + Â Â Â Â Â Â Â Â Â Â Âvoid *buffer,
> + Â Â Â Â Â Â Â Â Â Â Âdma_addr_t len,
> + Â Â Â Â Â Â Â Â Â Â Âint is_write,
> + Â Â Â Â Â Â Â Â Â Â Âdma_addr_t access_len);
> +
> +
> +void dma_invalidate_memory_range(DMADevice *dev,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dma_addr_t addr,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dma_addr_t len);
> +
> +
> +#define DEFINE_DMA_LD(suffix, size) Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
> +static inline uint##size##_t                       Â\
> +dma_ld##suffix(DMADevice *dev, dma_addr_t addr) Â Â Â Â Â Â Â Â Â Â Â Â Â \
> +{ Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
> + Â Âint err; Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â\
> + Â Âdma_addr_t paddr, plen; Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â\
> + Â Âif (!dev || !dev->mmu) { Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â\
> + Â Â Â Âreturn ld##suffix##_phys(addr); Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
> + Â Â} Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â\
> + Â Âerr = dev->mmu->translate(dev, addr, &paddr, &plen, 0); Â Â Â Â Â Â Â \
> + Â Âif (err || (plen < size / 8)) Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \

If the access is unaligned and the translation splits it to two (for
example, because of page boundary), the access is ignored, which can't
be correct.

Do we have such cases? If yes, should this be handled by the caller
instead (maybe not)?
ÿô.nlj·Ÿ®‰­†+%ŠË±é¥Šwÿº{.nlj·¤¾oâ^n‡r¡öë¨è&£ûz¹Þúzf£¢·hšˆ§~†­†Ûÿÿïÿ‘ê_èæ+v‰¨þ)ßø