From: Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro> To: "Michael S. Tsirkin" <mst@redhat.com> Cc: joro@8bytes.org, blauwirbel@gmail.com, paul@codesourcery.com, avi@redhat.com, anthony@codemonkey.ws, av1474@comtv.ru, yamahata@valinux.co.jp, kvm@vger.kernel.org, qemu-devel@nongnu.org Subject: Re: [PATCH RFC] dma_rw.h (was Re: [PATCH 0/7] AMD IOMMU emulation patchset v4) Date: Thu, 16 Sep 2010 10:06:16 +0300 [thread overview] Message-ID: <20100916070616.GA7811@localhost> (raw) In-Reply-To: <20100913200120.GA20918@redhat.com> On Mon, Sep 13, 2010 at 10:01:20PM +0200, Michael S. Tsirkin wrote: > So I think the following will give the idea of what an API > might look like that will let us avoid the scary hacks in > e.g. the ide layer and other generic layers that need to do DMA, > without either binding us to pci, adding more complexity with > callbacks, or losing type safety with casts and void*. > > Basically we have DMADevice that we can use container_of on > to get a PCIDevice from, and DMAMmu that will get instanciated > in a specific MMU. > > This is not complete code - just a header - I might complete > this later if/when there's interest or hopefully someone interested > in iommu emulation will. Hi, I personally like this approach better. It also seems to make poisoning cpu_physical_memory_*() easier if we convert every device to this API. We could then ban cpu_physical_memory_*(), perhaps by requiring a #define and #ifdef-ing those declarations. > Notes: > the IOMMU_PERM_RW code seem unused, so I replaced > this with plain is_write. Is it ever useful? The original idea made provisions for stuff like full R/W memory maps. In that case cpu_physical_memory_map() would call the translation / checking function with perms == IOMMU_PERM_RW. That's not there yet so it can be removed at the moment, especially since it only affects these helpers. Also, I'm not sure if there are other sorts of accesses besides reads and writes we want to check or translate. > It seems that invalidate callback should be able to > get away with just a device, so I switched to that > from a void pointer for type safety. > Seems enough for the users I saw. I think this makes matters too complicated. Normally, a single DMADevice should be embedded within a <bus>Device, so doing this makes it really hard to invalidate a specific map when there are more of them. It forces device code to act as a bus, provide fake 'DMADevice's for each map and dispatch translation to the real DMATranslateFunc. I see no other way. If you really want more type-safety (although I think this is a case of a true opaque identifying something only device code understands), I have another proposal: have a DMAMap embedded in the opaque. Example from dma-helpers.c: typedef struct { DMADevice *owner; [...] } DMAMap; typedef struct { [...] DMAMap map; [...] } DMAAIOCB; /* The callback. */ static void dma_bdrv_cancel(DMAMap *map) { DMAAIOCB *dbs = container_of(map, DMAAIOCB, map); [...] } The upside is we only need to pass the DMAMap. That can also contain details of the actual map in case the device wants to release only the relevant range and remap the rest. > I saw devices do stl_le_phys and such, these > might need to be wrapped as well. stl_le_phys() is defined and used only by hw/eepro100.c. That's already dealt with by converting the device. Thanks, Eduard > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > diff --git a/hw/dma_rw.h b/hw/dma_rw.h > new file mode 100644 > index 0000000..d63fd17 > --- /dev/null > +++ b/hw/dma_rw.h > @@ -0,0 +1,122 @@ > +#ifndef DMA_RW_H > +#define DMA_RW_H > + > +#include "qemu-common.h" > + > +/* We currently only have pci mmus, but using > + a generic type makes it possible to use this > + e.g. from the generic ide code without callbacks. */ > +typedef uint64_t dma_addr_t; > + > +typedef struct DMAMmu DMAMmu; > +typedef struct DMADevice DMADevice; > + > +typedef int DMATranslateFunc(DMAMmu *mmu, > + DMADevice *dev, > + dma_addr_t addr, > + dma_addr_t *paddr, > + dma_addr_t *len, > + int is_write); > + > +typedef int DMAInvalidateMapFunc(DMADevice *); > +struct DMAMmu { > + /* invalidate, etc. */ > + DmaTranslateFunc *translate; > +}; > + > +struct DMADevice { > + DMAMmu *mmu; > + DMAInvalidateMapFunc *invalidate; > +}; > + > +void dma_device_init(DMADevice *, DMAMmu *, DMAInvalidateMapFunc *); > + > +static inline void dma_memory_rw(DMADevice *dev, > + dma_addr_t addr, > + void *buf, > + uint32_t len, > + int is_write) > +{ > + uint32_t plen; > + /* Fast-path non-iommu. > + * More importantly, makes it obvious what this function does. */ > + if (!dev->mmu) { > + cpu_physical_memory_rw(paddr, buf, plen, is_write); > + return; > + } > + while (len) { > + err = dev->mmu->translate(iommu, 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; > + } > +} > + > +void *dma_memory_map(DMADevice *dev, > + dma_addr_t addr, > + uint32_t *len, > + int is_write); > +void dma_memory_unmap(DMADevice *dev, > + void *buffer, > + uint32_t len, > + int is_write, > + uint32_t access_len); > + > + > ++#define DEFINE_DMA_LD(suffix, size) \ > ++uint##size##_t dma_ld##suffix(DMADevice *dev, dma_addr_t addr) \ > ++{ \ > ++ int err; \ > ++ target_phys_addr_t paddr, plen; \ > ++ if (!dev->mmu) { \ > ++ return ld##suffix##_phys(addr, val); \ > ++ } \ > ++ \ > ++ err = dev->mmu->translate(dev->bus->iommu, dev, \ > ++ addr, &paddr, &plen, IOMMU_PERM_READ); \ > ++ if (err || (plen < size / 8)) \ > ++ return 0; \ > ++ \ > ++ return ld##suffix##_phys(paddr); \ > ++} > ++ > ++#define DEFINE_DMA_ST(suffix, size) \ > ++void dma_st##suffix(DMADevice *dev, dma_addr_t addr, uint##size##_t val) \ > ++{ \ > ++ int err; \ > ++ target_phys_addr_t paddr, plen; \ > ++ \ > ++ if (!dev->mmu) { \ > ++ st##suffix##_phys(addr, val); \ > ++ return; \ > ++ } \ > ++ err = dev->mmu->translate(dev->bus->iommu, dev, \ > ++ addr, &paddr, &plen, IOMMU_PERM_WRITE); \ > ++ if (err || (plen < size / 8)) \ > ++ return; \ > ++ \ > ++ st##suffix##_phys(paddr, val); \ > ++} > + > +DEFINE_DMA_LD(ub, 8) > +DEFINE_DMA_LD(uw, 16) > +DEFINE_DMA_LD(l, 32) > +DEFINE_DMA_LD(q, 64) > + > +DEFINE_DMA_ST(b, 8) > +DEFINE_DMA_ST(w, 16) > +DEFINE_DMA_ST(l, 32) > +DEFINE_DMA_ST(q, 64) > + > +#endif > diff --git a/hw/pci.h b/hw/pci.h > index 1c6075e..9737f0e 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -5,6 +5,7 @@ > #include "qobject.h" > > #include "qdev.h" > +#include "dma_rw.h" > > /* PCI includes legacy ISA access. */ > #include "isa.h" > @@ -119,6 +120,10 @@ enum { > > struct PCIDevice { > DeviceState qdev; > + > + /* For devices that do DMA. */ > + DMADevice dma; > + > /* PCI config space */ > uint8_t *config; >
WARNING: multiple messages have this Message-ID (diff)
From: Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro> To: "Michael S. Tsirkin" <mst@redhat.com> Cc: kvm@vger.kernel.org, joro@8bytes.org, qemu-devel@nongnu.org, blauwirbel@gmail.com, yamahata@valinux.co.jp, paul@codesourcery.com, avi@redhat.com Subject: [Qemu-devel] Re: [PATCH RFC] dma_rw.h (was Re: [PATCH 0/7] AMD IOMMU emulation patchset v4) Date: Thu, 16 Sep 2010 10:06:16 +0300 [thread overview] Message-ID: <20100916070616.GA7811@localhost> (raw) In-Reply-To: <20100913200120.GA20918@redhat.com> On Mon, Sep 13, 2010 at 10:01:20PM +0200, Michael S. Tsirkin wrote: > So I think the following will give the idea of what an API > might look like that will let us avoid the scary hacks in > e.g. the ide layer and other generic layers that need to do DMA, > without either binding us to pci, adding more complexity with > callbacks, or losing type safety with casts and void*. > > Basically we have DMADevice that we can use container_of on > to get a PCIDevice from, and DMAMmu that will get instanciated > in a specific MMU. > > This is not complete code - just a header - I might complete > this later if/when there's interest or hopefully someone interested > in iommu emulation will. Hi, I personally like this approach better. It also seems to make poisoning cpu_physical_memory_*() easier if we convert every device to this API. We could then ban cpu_physical_memory_*(), perhaps by requiring a #define and #ifdef-ing those declarations. > Notes: > the IOMMU_PERM_RW code seem unused, so I replaced > this with plain is_write. Is it ever useful? The original idea made provisions for stuff like full R/W memory maps. In that case cpu_physical_memory_map() would call the translation / checking function with perms == IOMMU_PERM_RW. That's not there yet so it can be removed at the moment, especially since it only affects these helpers. Also, I'm not sure if there are other sorts of accesses besides reads and writes we want to check or translate. > It seems that invalidate callback should be able to > get away with just a device, so I switched to that > from a void pointer for type safety. > Seems enough for the users I saw. I think this makes matters too complicated. Normally, a single DMADevice should be embedded within a <bus>Device, so doing this makes it really hard to invalidate a specific map when there are more of them. It forces device code to act as a bus, provide fake 'DMADevice's for each map and dispatch translation to the real DMATranslateFunc. I see no other way. If you really want more type-safety (although I think this is a case of a true opaque identifying something only device code understands), I have another proposal: have a DMAMap embedded in the opaque. Example from dma-helpers.c: typedef struct { DMADevice *owner; [...] } DMAMap; typedef struct { [...] DMAMap map; [...] } DMAAIOCB; /* The callback. */ static void dma_bdrv_cancel(DMAMap *map) { DMAAIOCB *dbs = container_of(map, DMAAIOCB, map); [...] } The upside is we only need to pass the DMAMap. That can also contain details of the actual map in case the device wants to release only the relevant range and remap the rest. > I saw devices do stl_le_phys and such, these > might need to be wrapped as well. stl_le_phys() is defined and used only by hw/eepro100.c. That's already dealt with by converting the device. Thanks, Eduard > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > diff --git a/hw/dma_rw.h b/hw/dma_rw.h > new file mode 100644 > index 0000000..d63fd17 > --- /dev/null > +++ b/hw/dma_rw.h > @@ -0,0 +1,122 @@ > +#ifndef DMA_RW_H > +#define DMA_RW_H > + > +#include "qemu-common.h" > + > +/* We currently only have pci mmus, but using > + a generic type makes it possible to use this > + e.g. from the generic ide code without callbacks. */ > +typedef uint64_t dma_addr_t; > + > +typedef struct DMAMmu DMAMmu; > +typedef struct DMADevice DMADevice; > + > +typedef int DMATranslateFunc(DMAMmu *mmu, > + DMADevice *dev, > + dma_addr_t addr, > + dma_addr_t *paddr, > + dma_addr_t *len, > + int is_write); > + > +typedef int DMAInvalidateMapFunc(DMADevice *); > +struct DMAMmu { > + /* invalidate, etc. */ > + DmaTranslateFunc *translate; > +}; > + > +struct DMADevice { > + DMAMmu *mmu; > + DMAInvalidateMapFunc *invalidate; > +}; > + > +void dma_device_init(DMADevice *, DMAMmu *, DMAInvalidateMapFunc *); > + > +static inline void dma_memory_rw(DMADevice *dev, > + dma_addr_t addr, > + void *buf, > + uint32_t len, > + int is_write) > +{ > + uint32_t plen; > + /* Fast-path non-iommu. > + * More importantly, makes it obvious what this function does. */ > + if (!dev->mmu) { > + cpu_physical_memory_rw(paddr, buf, plen, is_write); > + return; > + } > + while (len) { > + err = dev->mmu->translate(iommu, 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; > + } > +} > + > +void *dma_memory_map(DMADevice *dev, > + dma_addr_t addr, > + uint32_t *len, > + int is_write); > +void dma_memory_unmap(DMADevice *dev, > + void *buffer, > + uint32_t len, > + int is_write, > + uint32_t access_len); > + > + > ++#define DEFINE_DMA_LD(suffix, size) \ > ++uint##size##_t dma_ld##suffix(DMADevice *dev, dma_addr_t addr) \ > ++{ \ > ++ int err; \ > ++ target_phys_addr_t paddr, plen; \ > ++ if (!dev->mmu) { \ > ++ return ld##suffix##_phys(addr, val); \ > ++ } \ > ++ \ > ++ err = dev->mmu->translate(dev->bus->iommu, dev, \ > ++ addr, &paddr, &plen, IOMMU_PERM_READ); \ > ++ if (err || (plen < size / 8)) \ > ++ return 0; \ > ++ \ > ++ return ld##suffix##_phys(paddr); \ > ++} > ++ > ++#define DEFINE_DMA_ST(suffix, size) \ > ++void dma_st##suffix(DMADevice *dev, dma_addr_t addr, uint##size##_t val) \ > ++{ \ > ++ int err; \ > ++ target_phys_addr_t paddr, plen; \ > ++ \ > ++ if (!dev->mmu) { \ > ++ st##suffix##_phys(addr, val); \ > ++ return; \ > ++ } \ > ++ err = dev->mmu->translate(dev->bus->iommu, dev, \ > ++ addr, &paddr, &plen, IOMMU_PERM_WRITE); \ > ++ if (err || (plen < size / 8)) \ > ++ return; \ > ++ \ > ++ st##suffix##_phys(paddr, val); \ > ++} > + > +DEFINE_DMA_LD(ub, 8) > +DEFINE_DMA_LD(uw, 16) > +DEFINE_DMA_LD(l, 32) > +DEFINE_DMA_LD(q, 64) > + > +DEFINE_DMA_ST(b, 8) > +DEFINE_DMA_ST(w, 16) > +DEFINE_DMA_ST(l, 32) > +DEFINE_DMA_ST(q, 64) > + > +#endif > diff --git a/hw/pci.h b/hw/pci.h > index 1c6075e..9737f0e 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -5,6 +5,7 @@ > #include "qobject.h" > > #include "qdev.h" > +#include "dma_rw.h" > > /* PCI includes legacy ISA access. */ > #include "isa.h" > @@ -119,6 +120,10 @@ enum { > > struct PCIDevice { > DeviceState qdev; > + > + /* For devices that do DMA. */ > + DMADevice dma; > + > /* PCI config space */ > uint8_t *config; >
next prev parent reply other threads:[~2010-09-16 7:08 UTC|newest] Thread overview: 96+ messages / expand[flat|nested] mbox.gz Atom feed top 2010-08-28 14:54 [PATCH 0/7] AMD IOMMU emulation patchset v4 Eduard - Gabriel Munteanu 2010-08-28 14:54 ` [Qemu-devel] " Eduard - Gabriel Munteanu 2010-08-28 14:54 ` [PATCH 1/7] pci: expand tabs to spaces in pci_regs.h Eduard - Gabriel Munteanu 2010-08-28 14:54 ` [Qemu-devel] " Eduard - Gabriel Munteanu 2010-08-31 20:29 ` Michael S. Tsirkin 2010-08-31 20:29 ` [Qemu-devel] " Michael S. Tsirkin 2010-08-31 22:58 ` Eduard - Gabriel Munteanu 2010-08-31 22:58 ` [Qemu-devel] " Eduard - Gabriel Munteanu 2010-09-01 10:39 ` Michael S. Tsirkin 2010-09-01 10:39 ` [Qemu-devel] " Michael S. Tsirkin 2010-08-28 14:54 ` [PATCH 2/7] pci: memory access API and IOMMU support Eduard - Gabriel Munteanu 2010-08-28 14:54 ` [Qemu-devel] " Eduard - Gabriel Munteanu 2010-09-02 5:28 ` Michael S. Tsirkin 2010-09-02 5:28 ` [Qemu-devel] " Michael S. Tsirkin 2010-09-02 8:40 ` Eduard - Gabriel Munteanu 2010-09-02 8:40 ` [Qemu-devel] " Eduard - Gabriel Munteanu 2010-09-02 9:49 ` Michael S. Tsirkin 2010-09-02 9:49 ` [Qemu-devel] " Michael S. Tsirkin 2010-09-04 9:01 ` Blue Swirl 2010-09-04 9:01 ` [Qemu-devel] " Blue Swirl 2010-09-05 7:10 ` Michael S. Tsirkin 2010-09-05 7:10 ` [Qemu-devel] " Michael S. Tsirkin 2010-08-28 14:54 ` [PATCH 3/7] AMD IOMMU emulation Eduard - Gabriel Munteanu 2010-08-28 14:54 ` [Qemu-devel] " Eduard - Gabriel Munteanu 2010-08-28 15:58 ` Blue Swirl 2010-08-28 15:58 ` [Qemu-devel] " Blue Swirl 2010-08-28 21:53 ` Eduard - Gabriel Munteanu 2010-08-28 21:53 ` [Qemu-devel] " Eduard - Gabriel Munteanu 2010-08-29 20:37 ` Blue Swirl 2010-08-29 20:37 ` [Qemu-devel] " Blue Swirl 2010-08-30 3:07 ` [Qemu-devel] " Isaku Yamahata 2010-08-30 3:07 ` Isaku Yamahata 2010-08-30 5:54 ` Eduard - Gabriel Munteanu 2010-08-30 5:54 ` Eduard - Gabriel Munteanu 2010-08-28 14:54 ` [PATCH 4/7] ide: use the PCI memory access interface Eduard - Gabriel Munteanu 2010-08-28 14:54 ` [Qemu-devel] " Eduard - Gabriel Munteanu 2010-09-02 5:19 ` Michael S. Tsirkin 2010-09-02 5:19 ` [Qemu-devel] " Michael S. Tsirkin 2010-09-02 9:12 ` Eduard - Gabriel Munteanu 2010-09-02 9:12 ` [Qemu-devel] " Eduard - Gabriel Munteanu 2010-09-02 9:58 ` Michael S. Tsirkin 2010-09-02 9:58 ` [Qemu-devel] " Michael S. Tsirkin 2010-09-02 15:01 ` Eduard - Gabriel Munteanu 2010-09-02 15:01 ` [Qemu-devel] " Eduard - Gabriel Munteanu 2010-09-02 15:24 ` Avi Kivity 2010-09-02 15:24 ` [Qemu-devel] " Avi Kivity 2010-09-02 15:39 ` Michael S. Tsirkin 2010-09-02 15:39 ` [Qemu-devel] " Michael S. Tsirkin 2010-09-02 16:07 ` Avi Kivity 2010-09-02 16:07 ` [Qemu-devel] " Avi Kivity 2010-09-02 15:31 ` Michael S. Tsirkin 2010-09-02 15:31 ` [Qemu-devel] " Michael S. Tsirkin 2010-08-28 14:54 ` [PATCH 5/7] rtl8139: " Eduard - Gabriel Munteanu 2010-08-28 14:54 ` [Qemu-devel] " Eduard - Gabriel Munteanu 2010-08-28 14:54 ` [PATCH 6/7] eepro100: " Eduard - Gabriel Munteanu 2010-08-28 14:54 ` [Qemu-devel] " Eduard - Gabriel Munteanu 2010-08-28 14:54 ` [PATCH 7/7] ac97: " Eduard - Gabriel Munteanu 2010-08-28 14:54 ` [Qemu-devel] " Eduard - Gabriel Munteanu 2010-08-28 16:00 ` [PATCH 0/7] AMD IOMMU emulation patchset v4 Blue Swirl 2010-08-28 16:00 ` [Qemu-devel] " Blue Swirl 2010-08-29 9:55 ` Joerg Roedel 2010-08-29 9:55 ` [Qemu-devel] " Joerg Roedel 2010-08-29 20:44 ` Blue Swirl 2010-08-29 20:44 ` [Qemu-devel] " Blue Swirl 2010-08-29 22:08 ` [PATCH 2/7] pci: memory access API and IOMMU support Eduard - Gabriel Munteanu 2010-08-29 22:08 ` [Qemu-devel] " Eduard - Gabriel Munteanu 2010-08-29 22:11 ` Eduard - Gabriel Munteanu 2010-08-29 22:11 ` [Qemu-devel] " Eduard - Gabriel Munteanu 2010-09-01 20:10 ` [Qemu-devel] " Stefan Weil 2010-09-01 20:10 ` Stefan Weil 2010-09-02 6:00 ` Michael S. Tsirkin 2010-09-02 6:00 ` Michael S. Tsirkin 2010-09-02 9:08 ` Eduard - Gabriel Munteanu 2010-09-02 9:08 ` Eduard - Gabriel Munteanu 2010-09-02 13:24 ` Anthony Liguori 2010-09-02 13:24 ` Anthony Liguori 2010-09-02 8:51 ` Eduard - Gabriel Munteanu 2010-09-02 8:51 ` Eduard - Gabriel Munteanu 2010-09-02 16:05 ` Stefan Weil 2010-09-02 16:05 ` Stefan Weil 2010-09-02 16:14 ` Eduard - Gabriel Munteanu 2010-09-02 16:14 ` Eduard - Gabriel Munteanu 2010-09-13 20:01 ` [PATCH RFC] dma_rw.h (was Re: [PATCH 0/7] AMD IOMMU emulation patchset v4) Michael S. Tsirkin 2010-09-13 20:01 ` [Qemu-devel] " Michael S. Tsirkin 2010-09-13 20:45 ` Anthony Liguori 2010-09-13 20:45 ` Anthony Liguori 2010-09-16 7:12 ` Eduard - Gabriel Munteanu 2010-09-16 7:12 ` Eduard - Gabriel Munteanu 2010-09-16 9:35 ` Michael S. Tsirkin 2010-09-16 9:35 ` Michael S. Tsirkin 2010-09-16 7:06 ` Eduard - Gabriel Munteanu [this message] 2010-09-16 7:06 ` [Qemu-devel] " Eduard - Gabriel Munteanu 2010-09-16 9:20 ` Michael S. Tsirkin 2010-09-16 9:20 ` [Qemu-devel] " Michael S. Tsirkin 2010-09-16 11:15 ` Eduard - Gabriel Munteanu 2010-09-16 11:15 ` [Qemu-devel] " Eduard - Gabriel Munteanu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20100916070616.GA7811@localhost \ --to=eduard.munteanu@linux360.ro \ --cc=anthony@codemonkey.ws \ --cc=av1474@comtv.ru \ --cc=avi@redhat.com \ --cc=blauwirbel@gmail.com \ --cc=joro@8bytes.org \ --cc=kvm@vger.kernel.org \ --cc=mst@redhat.com \ --cc=paul@codesourcery.com \ --cc=qemu-devel@nongnu.org \ --cc=yamahata@valinux.co.jp \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.