[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/9] Add stub functions for PCI device models to
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH 1/9] Add stub functions for PCI device models to do PCI DMA |
Date: |
Sun, 2 Oct 2011 12:25:48 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Mon, Sep 05, 2011 at 02:34:56PM +1000, David Gibson wrote:
> This patch adds functions to pci.[ch] to perform PCI DMA operations. At
> present, these are just stubs which perform directly cpu physical memory
> accesses.
>
> Using these stubs, however, distinguishes PCI device DMA transactions from
> other accesses to physical memory, which will allow PCI IOMMU support to
> be added in one place, rather than updating every PCI driver at that time.
>
> That is, it allows us to update individual PCI drivers to support an IOMMU
> without having yet determined the details of how the IOMMU emulation will
> operate. This will let us remove the most bitrot-sensitive part of an
> IOMMU patch in advance.
>
> Signed-off-by: David Gibson <address@hidden>
So something I just thought about:
all wrappers now go through cpu_physical_memory_rw.
This is a problem as e.g. virtio assumes that
accesses such as stw are atomic. cpu_physical_memory_rw
is a memcpy which makes no such guarantees.
> ---
> dma.h | 2 ++
> hw/pci.c | 31 +++++++++++++++++++++++++++++++
> hw/pci.h | 33 +++++++++++++++++++++++++++++++++
> 3 files changed, 66 insertions(+), 0 deletions(-)
>
> diff --git a/dma.h b/dma.h
> index a6db5ba..06e91cb 100644
> --- a/dma.h
> +++ b/dma.h
> @@ -15,6 +15,8 @@
> #include "hw/hw.h"
> #include "block.h"
>
> +typedef target_phys_addr_t dma_addr_t;
> +
> typedef struct {
> target_phys_addr_t base;
> target_phys_addr_t len;
> diff --git a/hw/pci.c b/hw/pci.c
> index 1cdcbb7..0be7611 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -2211,3 +2211,34 @@ MemoryRegion *pci_address_space(PCIDevice *dev)
> {
> return dev->bus->address_space_mem;
> }
> +
> +#define PCI_DMA_DEFINE_LDST(_lname, _sname, _bits) \
> + uint##_bits##_t ld##_lname##_pci_dma(PCIDevice *dev, dma_addr_t addr) \
> + { \
> + uint##_bits##_t val; \
> + pci_dma_read(dev, addr, &val, sizeof(val)); \
> + return le##_bits##_to_cpu(val); \
> + } \
> + void st##_sname##_pci_dma(PCIDevice *dev, \
> + dma_addr_t addr, uint##_bits##_t val) \
> + { \
> + val = cpu_to_le##_bits(val); \
> + pci_dma_write(dev, addr, &val, sizeof(val)); \
> + }
> +
I am still not 100% positive why do we do the LE conversions here.
st4_phys and friends don't seem to do it ...
Has something to do with the fact we pass a value as an array?
Probably worth a comment.
> +uint8_t ldub_pci_dma(PCIDevice *dev, dma_addr_t addr)
> +{
> + uint8_t val;
> +
> + pci_dma_read(dev, addr, &val, sizeof(val));
> + return val;
> +}
> +
> +void stb_pci_dma(PCIDevice *dev, dma_addr_t addr, uint8_t val)
> +{
> + pci_dma_write(dev, addr, &val, sizeof(val));
> +}
> +
pci_ XXX would be better names?
> +PCI_DMA_DEFINE_LDST(uw, w, 16);
> +PCI_DMA_DEFINE_LDST(l, l, 32);
> +PCI_DMA_DEFINE_LDST(q, q, 64);
> diff --git a/hw/pci.h b/hw/pci.h
> index 391217e..4426e9d 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -6,6 +6,7 @@
>
> #include "qdev.h"
> #include "memory.h"
> +#include "dma.h"
>
> /* PCI includes legacy ISA access. */
> #include "isa.h"
> @@ -492,4 +493,36 @@ static inline uint32_t pci_config_size(const PCIDevice
> *d)
> return pci_is_express(d) ? PCIE_CONFIG_SPACE_SIZE :
> PCI_CONFIG_SPACE_SIZE;
> }
>
> +/* DMA access functions */
> +static inline int pci_dma_rw(PCIDevice *dev, dma_addr_t addr,
> + void *buf, dma_addr_t len, int is_write)
> +{
> + cpu_physical_memory_rw(addr, buf, len, is_write);
> + return 0;
> +}
> +
> +static inline int pci_dma_read(PCIDevice *dev, dma_addr_t addr,
> + void *buf, dma_addr_t len)
> +{
> + return pci_dma_rw(dev, addr, buf, len, 0);
> +}
> +
> +static inline int pci_dma_write(PCIDevice *dev, dma_addr_t addr,
> + const void *buf, dma_addr_t len)
> +{
> + return pci_dma_rw(dev, addr, (void *) buf, len, 1);
> +}
> +
> +#define PCI_DMA_DECLARE_LDST(_lname, _sname, _bits) \
> + uint##_bits##_t ld##_lname##_pci_dma(PCIDevice *dev, dma_addr_t addr); \
> + void st##_sname##_pci_dma(PCIDevice *dev, dma_addr_t addr, \
> + uint##_bits##_t val); \
> +
> +PCI_DMA_DECLARE_LDST(ub, b, 8);
> +PCI_DMA_DECLARE_LDST(uw, w, 16);
> +PCI_DMA_DECLARE_LDST(l, l, 32);
> +PCI_DMA_DECLARE_LDST(q, q, 64);
> +
> +#undef DECLARE_LDST_DMA
> +
I think macros should just create stX_phys/ldX_phys calls
directly, in the .h file. This will also make it clearer what is going on,
with less levels of indirection.
> #endif
> --
> 1.7.5.4
- Re: [Qemu-devel] [PATCH 1/9] Add stub functions for PCI device models to do PCI DMA,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PATCH 1/9] Add stub functions for PCI device models to do PCI DMA, Avi Kivity, 2011/10/02
- Re: [Qemu-devel] [PATCH 1/9] Add stub functions for PCI device models to do PCI DMA, Michael S. Tsirkin, 2011/10/02
- Re: [Qemu-devel] [PATCH 1/9] Add stub functions for PCI device models to do PCI DMA, Avi Kivity, 2011/10/02
- Re: [Qemu-devel] [PATCH 1/9] Add stub functions for PCI device models to do PCI DMA, Michael S. Tsirkin, 2011/10/02
- Re: [Qemu-devel] [PATCH 1/9] Add stub functions for PCI device models to do PCI DMA, Avi Kivity, 2011/10/02
- Re: [Qemu-devel] [PATCH 1/9] Add stub functions for PCI device models to do PCI DMA, Anthony Liguori, 2011/10/03