On Fri, Feb 24, 2012 at 02:27:39PM +1100, David Gibson wrote:
> Not that long ago, every device implementation using DMA directly
> accessed guest memory using cpu_physical_memory_*(). This meant that
> adding support for a guest visible IOMMU would require changing every
> one of these devices to go through IOMMU translation.
>
> Shortly before qemu 1.0, I made a start on fixing this by providing
> helper functions for PCI DMA. These are currently just stubs which
> call the direct access functions, but mean that an IOMMU can be
> implemented in one place, rather than for every PCI device.
>
> Clearly, this doesn't help for non PCI devices, which could also be
> IOMMU translated on some platforms. It is also problematic for the
> devices which have both PCI and non-PCI version (e.g. OHCI, AHCI) - we
> cannot use the the pci_dma_*() functions, because they assume the
> presence of a PCIDevice, but we don't want to have to check between
> pci_dma_*() and cpu_physical_memory_*() every time we do a DMA in the
> device code.
>
> This patch makes the first step on addressing both these problems, by
> introducing new (stub) dma helper functions which can be used for any
> DMA capable device.
>
> These dma functions take a DMAContext *, a new (currently empty)
> variable describing the DMA address space in which the operation is to
> take place. NULL indicates untranslated DMA directly into guest
> physical address space. The intention is that in future non-NULL
> values will given information about any necessary IOMMU translation.
>
> DMA using devices must obtain a DMAContext (or, potentially, contexts)
> from their bus or platform. For now this patch just converts the PCI
> wrappers to be implemented in terms of the universal wrappers,
> converting other drivers can take place over time.
>
> Cc: Michael S. Tsirkin <m...@redhat.com>
> Cc: Joerg Rodel <joerg.ro...@amd.com>
> Cc: Eduard - Gabriel Munteanu <eduard.munte...@linux360.ro>
> Cc: Richard Henderson <r...@twiddle.net>
>
> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
I'm a bit confused with all the stubbing going on.
Is this the final form of the pci_* functions or just
a stub? If the final form, we probably should just
open-code them - they don't buy us much.
If not, let's add a comment?
> ---
> dma.h | 87
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> hw/pci.h | 21 ++++++++------
> 2 files changed, 99 insertions(+), 9 deletions(-)
>
> diff --git a/dma.h b/dma.h
> index 79be131..d7428df 100644
> --- a/dma.h
> +++ b/dma.h
> @@ -28,6 +28,93 @@ typedef enum {
> DMA_DIRECTION_FROM_DEVICE = 1,
> } DMADirection;
>
> +typedef struct DMAContext {
why do we need the empty struct? Someone
will allocate an instance of it?
If not,
typedef struct DMAContext DMAContext;
in qemu-common.h would be enough.
> +
> +typedef void DMAInvalidateMapFunc(void *);
> +
> +static inline int dma_memory_rw(DMAContext *dma, dma_addr_t addr,
> + void *buf, dma_addr_t len, DMADirection dir)
> +{
> + cpu_physical_memory_rw(addr, buf, (target_phys_addr_t)len,
> + dir == DMA_DIRECTION_FROM_DEVICE);
> + return 0;
> +}
> +
> +static inline int dma_memory_read(DMAContext *dma, dma_addr_t addr,
> + void *buf, dma_addr_t len)
> +{
> + return dma_memory_rw(dma, addr, buf, len, DMA_DIRECTION_TO_DEVICE);
> +}
> +
> +static inline int dma_memory_write(DMAContext *dma, dma_addr_t addr,
> + const void *buf, dma_addr_t len)
> +{
> + return dma_memory_rw(dma, addr, (void *)buf, len,
> + DMA_DIRECTION_FROM_DEVICE);
> +}
> +
> +static inline void *dma_memory_map(DMAContext *dma,
> + DMAInvalidateMapFunc *cb, void *opaque,
> + dma_addr_t addr, dma_addr_t *len,
> + DMADirection dir)
> +{
> + target_phys_addr_t xlen = *len;
> + void *p;
> +
> + p = cpu_physical_memory_map(addr, &xlen,
> + dir == DMA_DIRECTION_FROM_DEVICE);
> + *len = xlen;
> + return p;
> +}
> +
> +static inline void dma_memory_unmap(DMAContext *dma,
> + void *buffer, dma_addr_t len,
> + DMADirection dir, dma_addr_t access_len)
> +{
> + return cpu_physical_memory_unmap(buffer, (target_phys_addr_t)len,
> + dir == DMA_DIRECTION_FROM_DEVICE,
> + access_len);
> +}
> +
> +#define DEFINE_LDST_DMA(_lname, _sname, _bits, _end) \
> + static inline uint##_bits##_t ld##_lname##_##_end##_dma(DMAContext *dma,
> \
> + dma_addr_t addr)
> \
> + { \
> + uint##_bits##_t val; \
> + dma_memory_read(dma, addr, &val, (_bits) / 8); \
> + return _end##_bits##_to_cpu(val); \
> + } \
> + static inline void st##_sname##_##_end##_dma(DMAContext *dma, \
> + dma_addr_t addr, \
> + uint##_bits##_t val) \
> + { \
> + val = cpu_to_##_end##_bits(val); \
> + dma_memory_write(dma, addr, &val, (_bits) / 8); \
> + }
> +
> +static inline uint8_t ldub_dma(DMAContext *dma, dma_addr_t addr)
> +{
> + uint8_t val;
> +
> + dma_memory_read(dma, addr, &val, 1);
> + return val;
> +}
> +
> +static inline void stb_dma(DMAContext *dma, dma_addr_t addr, uint8_t val)
> +{
> + dma_memory_write(dma, addr, &val, 1);
> +}
> +
> +DEFINE_LDST_DMA(uw, w, 16, le);
> +DEFINE_LDST_DMA(l, l, 32, le);
> +DEFINE_LDST_DMA(q, q, 64, le);
> +DEFINE_LDST_DMA(uw, w, 16, be);
> +DEFINE_LDST_DMA(l, l, 32, be);
> +DEFINE_LDST_DMA(q, q, 64, be);
> +
> +#undef DEFINE_LDST_DMA
> +
> struct ScatterGatherEntry {
> dma_addr_t base;
> dma_addr_t len;
> diff --git a/hw/pci.h b/hw/pci.h
> index ee53f26..64734a1 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -544,10 +544,15 @@ static inline uint32_t pci_config_size(const PCIDevice
> *d)
> }
>
> /* DMA access functions */
> +static inline DMAContext *pci_dma_context(PCIDevice *dev)
> +{
> + return NULL;
This is a stub, right? Pls add a comment.
> +}
> +
> static inline int pci_dma_rw(PCIDevice *dev, dma_addr_t addr,
> void *buf, dma_addr_t len, DMADirection dir)
> {
> - cpu_physical_memory_rw(addr, buf, len, dir == DMA_DIRECTION_FROM_DEVICE);
> + dma_memory_rw(pci_dma_context(dev), addr, buf, len, dir);
> return 0;
> }
>
> @@ -567,12 +572,12 @@ static inline int pci_dma_write(PCIDevice *dev,
> dma_addr_t addr,
> static inline uint##_bits##_t ld##_l##_pci_dma(PCIDevice *dev, \
> dma_addr_t addr) \
> { \
> - return ld##_l##_phys(addr); \
> + return ld##_l##_dma(pci_dma_context(dev), addr); \
> } \
> static inline void st##_s##_pci_dma(PCIDevice *dev, \
> - dma_addr_t addr, uint##_bits##_t val) \
> + dma_addr_t addr, uint##_bits##_t
> val) \
> { \
> - st##_s##_phys(addr, val); \
> + st##_s##_dma(pci_dma_context(dev), addr, val); \
> }
>
> PCI_DMA_DEFINE_LDST(ub, b, 8);
> @@ -586,21 +591,19 @@ PCI_DMA_DEFINE_LDST(q_be, q_be, 64);
> #undef PCI_DMA_DEFINE_LDST
>
> static inline void *pci_dma_map(PCIDevice *dev, dma_addr_t addr,
> + DMAInvalidateMapFunc *cb, void *opaque,
> dma_addr_t *plen, DMADirection dir)
> {
> - target_phys_addr_t len = *plen;
> void *buf;
>
> - buf = cpu_physical_memory_map(addr, &len, dir ==
> DMA_DIRECTION_FROM_DEVICE);
> - *plen = len;
> + buf = dma_memory_map(pci_dma_context(dev), cb, opaque, addr, plen, dir);
> return buf;
> }
>
> static inline void pci_dma_unmap(PCIDevice *dev, void *buffer, dma_addr_t
> len,
> DMADirection dir, dma_addr_t access_len)
> {
> - cpu_physical_memory_unmap(buffer, len, dir == DMA_DIRECTION_FROM_DEVICE,
> - access_len);
> + dma_memory_unmap(pci_dma_context(dev), buffer, len, dir, access_len);
> }
>
> static inline void pci_dma_sglist_init(QEMUSGList *qsg, PCIDevice *dev,
> --
> 1.7.9
