[RFC,1/4] pci: memory access API and IOMMU support

PCI devices should access memory through pci_memory_*() instead of
cpu_physical_memory_*(). This also provides support for translation and
access checking in case an IOMMU is emulated.

Memory maps are treated as remote IOTLBs (that is, translation caches
belonging to the IOMMU-aware device itself). Clients (devices) must
provide callbacks for map invalidation in case these maps are
persistent beyond the current I/O context, e.g. AIO DMA transfers.

Signed-off-by: Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro>
---
 hw/pci.c      |  145 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci.h      |  130 +++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-common.h |    1 +
 3 files changed, 276 insertions(+), 0 deletions(-)

On Wed, Aug 4, 2010 at 10:32 PM, Eduard - Gabriel Munteanu
<eduard.munteanu@linux360.ro> wrote:
> PCI devices should access memory through pci_memory_*() instead of
> cpu_physical_memory_*(). This also provides support for translation and
> access checking in case an IOMMU is emulated.
>
> Memory maps are treated as remote IOTLBs (that is, translation caches
> belonging to the IOMMU-aware device itself). Clients (devices) must
> provide callbacks for map invalidation in case these maps are
> persistent beyond the current I/O context, e.g. AIO DMA transfers.
>
> Signed-off-by: Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro>
> ---
>  hw/pci.c      |  145 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/pci.h      |  130 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-common.h |    1 +
>  3 files changed, 276 insertions(+), 0 deletions(-)
>
> diff --git a/hw/pci.c b/hw/pci.c
> index 6871728..ce2734b 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -58,6 +58,10 @@ struct PCIBus {
>        Keep a count of the number of devices with raised IRQs.  */
>     int nirq;
>     int *irq_count;
> +
> +#ifdef CONFIG_PCI_IOMMU

The code should not be conditional.

> +    PCIIOMMU *iommu;
> +#endif
>  };
>
>  static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent);
> @@ -2029,6 +2033,147 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent)
>     }
>  }
>
> +#ifdef CONFIG_PCI_IOMMU
> +
> +void pci_register_iommu(PCIDevice *dev, PCIIOMMU *iommu)
> +{
> +    dev->bus->iommu = iommu;
> +}
> +
> +void pci_memory_rw(PCIDevice *dev,
> +                   pci_addr_t addr,
> +                   uint8_t *buf,
> +                   pci_addr_t len,
> +                   int is_write)
> +{
> +    int err, plen;
> +    unsigned perms;
> +    PCIIOMMU *iommu = dev->bus->iommu;
> +    target_phys_addr_t paddr;
> +
> +    if (!iommu || !iommu->translate)
> +        return cpu_physical_memory_rw(addr, buf, len, is_write);

Instead of these kind of checks, please add default handlers which
call cpu_physical_memory_rw() etc.

> +
> +    perms = is_write ? IOMMU_PERM_WRITE : IOMMU_PERM_READ;

Is this useful? How about just passing is_write as perms?

> +
> +    while (len) {
> +        err = iommu->translate(iommu, dev, addr, &paddr, &plen, perms);
> +        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;
> +    }
> +}
> +
> +void *pci_memory_map(PCIDevice *dev,
> +                     PCIInvalidateIOTLBFunc *cb,
> +                     void *opaque,
> +                     pci_addr_t addr,
> +                     target_phys_addr_t *len,
> +                     int is_write)
> +{
> +    int err, plen;
> +    unsigned perms;
> +    PCIIOMMU *iommu = dev->bus->iommu;
> +    target_phys_addr_t paddr;
> +
> +    if (!iommu || !iommu->translate)
> +        return cpu_physical_memory_map(addr, len, is_write);
> +
> +    perms = is_write ? IOMMU_PERM_WRITE : IOMMU_PERM_READ;
> +
> +    plen = *len;
> +    err = iommu->translate(iommu, dev, addr, &paddr, &plen, perms);
> +    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 && iommu->register_iotlb_invalidator)
> +        iommu->register_iotlb_invalidator(iommu, dev, addr, cb, opaque);
> +
> +    return cpu_physical_memory_map(paddr, len, is_write);
> +}
> +
> +void pci_memory_unmap(PCIDevice *dev,
> +                      void *buffer,
> +                      target_phys_addr_t len,
> +                      int is_write,
> +                      target_phys_addr_t access_len)
> +{
> +    cpu_physical_memory_unmap(buffer, len, is_write, access_len);
> +}
> +
> +#define DEFINE_PCI_LD(suffix, size)                                       \
> +uint##size##_t pci_ld##suffix(PCIDevice *dev, pci_addr_t addr)            \
> +{                                                                         \
> +    PCIIOMMU *iommu = dev->bus->iommu;                                    \
> +    target_phys_addr_t paddr;                                             \
> +    int plen, err;                                                        \
> +                                                                          \
> +    if (!iommu || !iommu->translate)                                      \
> +        return ld##suffix##_phys(addr);                                   \
> +                                                                          \
> +    err = iommu->translate(iommu, dev,                                    \
> +                           addr, &paddr, &plen, IOMMU_PERM_READ);         \
> +    if (err || (plen < size / 8))                                         \
> +        return 0;                                                         \
> +                                                                          \
> +    return ld##suffix##_phys(paddr);                                      \
> +}
> +
> +#define DEFINE_PCI_ST(suffix, size)                                       \
> +void pci_st##suffix(PCIDevice *dev, pci_addr_t addr, uint##size##_t val)  \
> +{                                                                         \
> +    PCIIOMMU *iommu = dev->bus->iommu;                                    \
> +    target_phys_addr_t paddr;                                             \
> +    int plen, err;                                                        \
> +                                                                          \
> +    if (!iommu || !iommu->translate) {                                    \
> +        st##suffix##_phys(addr, val);                                     \
> +        return;                                                           \
> +    }                                                                     \
> +                                                                          \
> +    err = iommu->translate(iommu, dev,                                    \
> +                           addr, &paddr, &plen, IOMMU_PERM_WRITE);        \
> +    if (err || (plen < size / 8))                                         \
> +        return;                                                           \
> +                                                                          \
> +    st##suffix##_phys(paddr, val);                                        \
> +}
> +
> +#else /* !defined(CONFIG_PCI_IOMMU) */
> +
> +#define DEFINE_PCI_LD(suffix, size)
> +#define DEFINE_PCI_ST(suffix, size)
> +
> +#endif /* CONFIG_PCI_IOMMU */
> +
> +DEFINE_PCI_LD(ub, 8)
> +DEFINE_PCI_LD(uw, 16)
> +DEFINE_PCI_LD(l, 32)
> +DEFINE_PCI_LD(q, 64)
> +
> +DEFINE_PCI_ST(b, 8)
> +DEFINE_PCI_ST(w, 16)
> +DEFINE_PCI_ST(l, 32)
> +DEFINE_PCI_ST(q, 64)
> +
>  static PCIDeviceInfo bridge_info = {
>     .qdev.name    = "pci-bridge",
>     .qdev.size    = sizeof(PCIBridge),
> diff --git a/hw/pci.h b/hw/pci.h
> index 4bd8a1a..bd8c21b 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -430,4 +430,134 @@ static inline int ranges_overlap(uint64_t first1, uint64_t len1,
>     return !(last2 < first1 || last1 < first2);
>  }
>
> +/*
> + * Memory I/O and PCI IOMMU definitions.
> + */
> +
> +typedef target_phys_addr_t pci_addr_t;

There is already pcibus_t.

> +
> +typedef int PCIInvalidateIOTLBFunc(void *opaque);

I think some type safety tricks could be used with for example PCIDevice *.

> +
> +#ifndef CONFIG_PCI_IOMMU
> +
> +static inline void pci_memory_rw(PCIDevice *dev,
> +                                 pci_addr_t addr,
> +                                 uint8_t *buf,
> +                                 pci_addr_t len,
> +                                 int is_write)
> +{
> +    cpu_physical_memory_rw(addr, buf, len, is_write);
> +}
> +
> +static inline void *pci_memory_map(PCIDevice *dev,
> +                                   PCIInvalidateIOTLBFunc *cb,
> +                                   void *opaque,
> +                                   pci_addr_t addr,
> +                                   target_phys_addr_t *len,
> +                                   int is_write)
> +{
> +    return cpu_physical_memory_map(addr, plen, is_write);
> +}
> +
> +static inline void pci_memory_unmap(PCIDevice *dev,
> +                                    void *buffer,
> +                                    target_phys_addr_t len,
> +                                    int is_write,
> +                                    target_phys_addr_t access_len)
> +{
> +    cpu_physical_memory_unmap(buffer, len, is_write, access_len);
> +}
> +
> +#define DECLARE_PCI_LD(suffix, size)                                    \
> +static inline uint##size##_t pci_ld##suffix(PCIDevice *dev,             \
> +                                            pci_addr_t addr)            \
> +{                                                                       \
> +    return ld##suffix##_phys(addr);                                     \
> +}
> +
> +#define DECLARE_PCI_ST(suffix, size)                                    \
> +static inline void pci_st##suffix(PCIDevice *dev,                       \
> +                                  pci_addr_t addr,                      \
> +                                  uint##size##_t val)                   \
> +{                                                                       \
> +    st##suffix##_phys(addr, val);                                       \
> +}
> +
> +#else /* defined(CONFIG_PCI_IOMMU) */
> +
> +struct PCIIOMMU {
> +    void *opaque;
> +
> +    void (*register_iotlb_invalidator)(PCIIOMMU *iommu,
> +                                       PCIDevice *dev,
> +                                       pci_addr_t addr,
> +                                       PCIInvalidateIOTLBFunc *cb,
> +                                       void *opaque);
> +    int (*translate)(PCIIOMMU *iommu,
> +                     PCIDevice *dev,
> +                     pci_addr_t addr,
> +                     target_phys_addr_t *paddr,
> +                     int *len,
> +                     unsigned perms);
> +};
> +
> +#define IOMMU_PERM_READ     (1 << 0)
> +#define IOMMU_PERM_WRITE    (1 << 1)
> +#define IOMMU_PERM_RW       (IOMMU_PERM_READ | IOMMU_PERM_WRITE)
> +
> +extern void pci_memory_rw(PCIDevice *dev,
> +                          pci_addr_t addr,
> +                          uint8_t *buf,
> +                          pci_addr_t len,
> +                          int is_write);
> +extern void *pci_memory_map(PCIDevice *dev,
> +                            PCIInvalidateIOTLBFunc *cb,
> +                            void *opaque,
> +                            pci_addr_t addr,
> +                            target_phys_addr_t *len,
> +                            int is_write);
> +extern void pci_memory_unmap(PCIDevice *dev,
> +                             void *buffer,
> +                             target_phys_addr_t len,
> +                             int is_write,
> +                             target_phys_addr_t access_len);
> +extern void pci_register_iommu(PCIDevice *dev,
> +                               PCIIOMMU *iommu);
> +
> +#define DECLARE_PCI_LD(suffix, size)                                    \
> +extern uint##size##_t pci_ld##suffix(PCIDevice *dev,  pci_addr_t addr);
> +
> +#define DECLARE_PCI_ST(suffix, size)                                    \
> +extern void pci_st##suffix(PCIDevice *dev,                              \
> +                           pci_addr_t addr,                             \
> +                           uint##size##_t val);
> +
> +#endif /* CONFIG_PCI_IOMMU */
> +
> +static inline void pci_memory_read(PCIDevice *dev,
> +                                   pci_addr_t addr,
> +                                   uint8_t *buf,
> +                                   pci_addr_t len)
> +{
> +    pci_memory_rw(dev, addr, buf, len, 0);
> +}
> +
> +static inline void pci_memory_write(PCIDevice *dev,
> +                                    pci_addr_t addr,
> +                                    const uint8_t *buf,
> +                                    pci_addr_t len)
> +{
> +    pci_memory_rw(dev, addr, (uint8_t *) buf, len, 1);
> +}
> +
> +DECLARE_PCI_LD(ub, 8)
> +DECLARE_PCI_LD(uw, 16)
> +DECLARE_PCI_LD(l, 32)
> +DECLARE_PCI_LD(q, 64)
> +
> +DECLARE_PCI_ST(b, 8)
> +DECLARE_PCI_ST(w, 16)
> +DECLARE_PCI_ST(l, 32)
> +DECLARE_PCI_ST(q, 64)
> +
>  #endif
> diff --git a/qemu-common.h b/qemu-common.h
> index 3fb2f0b..8daf962 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -219,6 +219,7 @@ typedef struct PCIHostState PCIHostState;
>  typedef struct PCIExpressHost PCIExpressHost;
>  typedef struct PCIBus PCIBus;
>  typedef struct PCIDevice PCIDevice;
> +typedef struct PCIIOMMU PCIIOMMU;
>  typedef struct SerialState SerialState;
>  typedef struct IRQState *qemu_irq;
>  typedef struct PCMCIACardState PCMCIACardState;
> --
> 1.7.1
>
>
>

On Thu, Aug 05, 2010 at 09:23:30PM +0000, Blue Swirl wrote:
> On Wed, Aug 4, 2010 at 10:32 PM, Eduard - Gabriel Munteanu

[snip]

> > @@ -58,6 +58,10 @@ struct PCIBus {
> > ?? ?? ?? ??Keep a count of the number of devices with raised IRQs. ??*/
> > ?? ?? int nirq;
> > ?? ?? int *irq_count;
> > +
> > +#ifdef CONFIG_PCI_IOMMU
> 
> The code should not be conditional.
> 
> > + ?? ??PCIIOMMU *iommu;
> > +#endif
> > ??};
> >
> > ??static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent);
> > @@ -2029,6 +2033,147 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent)
> > ?? ?? }
> > ??}
> >
> > +#ifdef CONFIG_PCI_IOMMU
> > +
> > +void pci_register_iommu(PCIDevice *dev, PCIIOMMU *iommu)
> > +{
> > + ?? ??dev->bus->iommu = iommu;
> > +}
> > +
> > +void pci_memory_rw(PCIDevice *dev,
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? pci_addr_t addr,
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? uint8_t *buf,
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? pci_addr_t len,
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? int is_write)
> > +{
> > + ?? ??int err, plen;
> > + ?? ??unsigned perms;
> > + ?? ??PCIIOMMU *iommu = dev->bus->iommu;
> > + ?? ??target_phys_addr_t paddr;
> > +
> > + ?? ??if (!iommu || !iommu->translate)
> > + ?? ?? ?? ??return cpu_physical_memory_rw(addr, buf, len, is_write);
> 
> Instead of these kind of checks, please add default handlers which
> call cpu_physical_memory_rw() etc.
> 

Ok. I'm trying to minimize impact (non-inlineable function calls) when
the IOMMU is disabled at compile-time. I think I can do it some other
way, as you suggest.

> > +
> > + ?? ??perms = is_write ? IOMMU_PERM_WRITE : IOMMU_PERM_READ;
> 
> Is this useful? How about just passing is_write as perms?
> 

Only in theory: it might come in handy if we ever support RW operations,
like read-modify-write memory maps. Also, write permissions include
zero-byte reads for the AMD IOMMU, so IOMMU_PERM_* could be further
refined.

I'm happy to remove it, though.

[snip]

> > +/*
> > + * Memory I/O and PCI IOMMU definitions.
> > + */
> > +
> > +typedef target_phys_addr_t pci_addr_t;
> 
> There is already pcibus_t.
> 

Thanks, I'll use that.

> > +
> > +typedef int PCIInvalidateIOTLBFunc(void *opaque);
> 
> I think some type safety tricks could be used with for example PCIDevice *.
> 

Note that 'opaque' belongs to the caller (the code that requests
memory maps).

Some device might make multiple maps that can be invalidated separately.
The actual stuff that describes the map might not be straightforward to
recover from a PCIDevice.

We could add another parameter to PCIInvalidateIOTLBFunc(), but since
the main user is DMA code, it's going to complicate things further.

[snip]

	Eduard