Re: [Qemu-devel] [RFC PATCH 01/13] Generic DMA memory access interface

2011-06-02 Thread David Gibson
On Wed, Jun 01, 2011 at 08:45:56AM -0700, Richard Henderson wrote:
 On 06/01/2011 08:35 AM, Eduard - Gabriel Munteanu wrote:
  Maybe it's not nice, but you're missing the fact upcasting gives you
  some type safety. With opaques you have none.
 
 Lol.  Do you understand what container_of does?
 This is not dynamic_cast with RTTI.
 
 You can put any type name in there that you like,
 so long as it has a field name to match.  The type
 of the field you give doesn't even have to match
 the type of the pointer that you pass in.

Uh, if that's true, that's a bug in the container_of implementation.
The ccan container_of implementation, for example, certainly does
check that the given field has type matching the pointer.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [Qemu-devel] [RFC PATCH 01/13] Generic DMA memory access interface

2011-06-02 Thread David Gibson
On Wed, Jun 01, 2011 at 08:16:44AM -0700, Richard Henderson wrote:
 On 06/01/2011 07:29 AM, Avi Kivity wrote:
  On 06/01/2011 05:01 PM, Richard Henderson wrote:
+err = dev-mmu-translate(dev, addr,paddr,plen, is_write);
 
  I see you didn't take my suggestion for using an opaque callback pointer.
  Really and truly, I won't be able to use this as-is for Alpha.
 
  
  Rather than opaques, please pass the DMA engine itself and use 
  container_of().
 
 The dma engine object is currently sitting in the PCIBus structure.
 Which is private, and can't be extended by a host bridge implementation.
 
 The entire code could be re-arranged, true, but please suggest something
 reasonable.
 
  We should be removing opaques, not adding them.
 
 See my followup elsewhere.  Opaques *can* be cleaner than upcasting,
 particularly if there are too many hoops through which to jump.

So, in the meantime, I've also done a version of Eduard's earlier
patches, with added support for the PAPR hypervisor managed IOMMU.

I have also significantly reworked how the structure lookup works,
partly because in my case I'l looking at IOMMU translation for non-PCI
devices, but I think it may also address your concerns.  I'm still
using upcasts, but there are less steps from the device to the IOMMU
state.

I've been sick and haven't had a chance to merge my stuff with
Eduard's changes.  I'll post them anyway, as another discussion
point.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [Qemu-devel] [RFC PATCH 01/13] Generic DMA memory access interface

2011-06-01 Thread Richard Henderson
On 05/31/2011 06:38 PM, Eduard - Gabriel Munteanu wrote:
 +static inline void dma_memory_rw(DMADevice *dev,
 + dma_addr_t addr,
 + void *buf,
 + dma_addr_t len,
 + int is_write)

I don't think this needs to be inline...

 +{
 +/*
 + * Fast-path non-iommu.
 + * More importantly, makes it obvious what this function does.
 + */
 +if (!dev || !dev-mmu) {
 +cpu_physical_memory_rw(addr, buf, len, is_write);
 +return;
 +}

... because you'll never be able to eliminate the if or the calls.
You might as well make the overall code smaller by taking the
entire function out of line.

 +#define DEFINE_DMA_LD(prefix, suffix, devtype, dmafield, 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);   \
 +} \

Similarly for all the ld/st functions.

 +#define DEFINE_DMA_MEMORY_RW(prefix, devtype, dmafield)
 +#define DEFINE_DMA_MEMORY_READ(prefix, devtype, dmafield)
 +#define DEFINE_DMA_MEMORY_WRITE(prefix, devtype, dmafield)
 +
 +#define DEFINE_DMA_OPS(prefix, devtype, dmafield)  \

I think this is a bit over the top, really.

 +err = dev-mmu-translate(dev, addr, paddr, plen, is_write);

I see you didn't take my suggestion for using an opaque callback pointer.
Really and truly, I won't be able to use this as-is for Alpha.


r~
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [Qemu-devel] [RFC PATCH 01/13] Generic DMA memory access interface

2011-06-01 Thread Avi Kivity

On 06/01/2011 05:01 PM, Richard Henderson wrote:

  +err = dev-mmu-translate(dev, addr,paddr,plen, is_write);

I see you didn't take my suggestion for using an opaque callback pointer.
Really and truly, I won't be able to use this as-is for Alpha.



Rather than opaques, please pass the DMA engine itself and use 
container_of().


We should be removing opaques, not adding them.

--
error compiling committee.c: too many arguments to function

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [Qemu-devel] [RFC PATCH 01/13] Generic DMA memory access interface

2011-06-01 Thread Eduard - Gabriel Munteanu
On Wed, Jun 01, 2011 at 07:01:42AM -0700, Richard Henderson wrote:
 On 05/31/2011 06:38 PM, Eduard - Gabriel Munteanu wrote:
  +static inline void dma_memory_rw(DMADevice *dev,
  + dma_addr_t addr,
  + void *buf,
  + dma_addr_t len,
  + int is_write)
 
 I don't think this needs to be inline...
 
  +{
  +/*
  + * Fast-path non-iommu.
  + * More importantly, makes it obvious what this function does.
  + */
  +if (!dev || !dev-mmu) {
  +cpu_physical_memory_rw(addr, buf, len, is_write);
  +return;
  +}
 
 ... because you'll never be able to eliminate the if or the calls.
 You might as well make the overall code smaller by taking the
 entire function out of line.
 
  +#define DEFINE_DMA_LD(prefix, suffix, devtype, dmafield, 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);   \
  +} \
 
 Similarly for all the ld/st functions.
 

The idea was to get to the fastpath as soon as possible. I'm not really
concerned about the case where there's an IOMMU present, since
translation/checking does a lot more work. But other people might be
worried about that additional function call when there's no IOMMU.

And these functions are quite small anyway.

Thoughts, anybody else?

  +#define DEFINE_DMA_MEMORY_RW(prefix, devtype, dmafield)
  +#define DEFINE_DMA_MEMORY_READ(prefix, devtype, dmafield)
  +#define DEFINE_DMA_MEMORY_WRITE(prefix, devtype, dmafield)
  +
  +#define DEFINE_DMA_OPS(prefix, devtype, dmafield)  \
 
 I think this is a bit over the top, really.
 

Yeah, it's a bit unconventional, but why do you think that?

The main selling point is there are more chances to screw up if every
bus layer implements these manually. And it's really convenient,
especially if we get to add another ld/st.

I do have one concern about it, though: it might increase compile time
due to additional preprocessing work. I haven't done any benchmarks on
that. But apart from this, are there any other objections?

  +err = dev-mmu-translate(dev, addr, paddr, plen, is_write);
 
 I see you didn't take my suggestion for using an opaque callback pointer.
 Really and truly, I won't be able to use this as-is for Alpha.
 

If I understand correctly you need some sort of shared state between
IOMMUs or units residing on different buses. Then you should be able to
get to it even with this API, just like I do with my AMD IOMMU state by
upcasting. It doesn't seem to matter whether you've got an opaque, that
opaque could very well be reachable by upcasting.

Did I get this wrong?


Eduard

 
 r~
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [Qemu-devel] [RFC PATCH 01/13] Generic DMA memory access interface

2011-06-01 Thread Richard Henderson
On 06/01/2011 07:52 AM, Eduard - Gabriel Munteanu wrote:
 The main selling point is there are more chances to screw up if every
 bus layer implements these manually. And it's really convenient,
 especially if we get to add another ld/st.

If we drop the ld/st, we're talking about 5 lines for every bus layer.

If I recall, there was just the one driver that actually uses the ld/st
interface; most used the read/write interface.

 If I understand correctly you need some sort of shared state between
 IOMMUs or units residing on different buses. Then you should be able to
 get to it even with this API, just like I do with my AMD IOMMU state by
 upcasting. It doesn't seem to matter whether you've got an opaque, that
 opaque could very well be reachable by upcasting.
 
 Did I get this wrong?

Can you honestly tell me that 

 +static int amd_iommu_translate(DMADevice *dev,
 +   dma_addr_t addr,
 +   dma_addr_t *paddr,
 +   dma_addr_t *len,
 +   int is_write)
 +{
 +PCIDevice *pci_dev = container_of(dev, PCIDevice, dma);
 +PCIDevice *iommu_dev = DO_UPCAST(PCIDevice, qdev, dev-mmu-iommu);
 +AMDIOMMUState *s = DO_UPCAST(AMDIOMMUState, dev, iommu_dev);

THREE (3) upcasts is a sane to write maintainable software?
The margin for error here is absolutely enormous.

If you had just passed in that AMDIOMMUState* as the opaque
value, it would be trivial to look at the initialization
statement and the callback function to verify that the right
value is being passed.


r~
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [Qemu-devel] [RFC PATCH 01/13] Generic DMA memory access interface

2011-06-01 Thread Richard Henderson
On 06/01/2011 07:29 AM, Avi Kivity wrote:
 On 06/01/2011 05:01 PM, Richard Henderson wrote:
   +err = dev-mmu-translate(dev, addr,paddr,plen, is_write);

 I see you didn't take my suggestion for using an opaque callback pointer.
 Really and truly, I won't be able to use this as-is for Alpha.

 
 Rather than opaques, please pass the DMA engine itself and use container_of().

The dma engine object is currently sitting in the PCIBus structure.
Which is private, and can't be extended by a host bridge implementation.

The entire code could be re-arranged, true, but please suggest something
reasonable.

 We should be removing opaques, not adding them.

See my followup elsewhere.  Opaques *can* be cleaner than upcasting,
particularly if there are too many hoops through which to jump.


r~
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [Qemu-devel] [RFC PATCH 01/13] Generic DMA memory access interface

2011-06-01 Thread Eduard - Gabriel Munteanu
On Wed, Jun 01, 2011 at 08:09:29AM -0700, Richard Henderson wrote:
 On 06/01/2011 07:52 AM, Eduard - Gabriel Munteanu wrote:
  The main selling point is there are more chances to screw up if every
  bus layer implements these manually. And it's really convenient,
  especially if we get to add another ld/st.
 
 If we drop the ld/st, we're talking about 5 lines for every bus layer.
 
 If I recall, there was just the one driver that actually uses the ld/st
 interface; most used the read/write interface.

Hm, indeed there seem to be far fewer uses of those now, actually my
patches don't seem to be using those. 

What do you guys think? Will these go away completely?

  If I understand correctly you need some sort of shared state between
  IOMMUs or units residing on different buses. Then you should be able to
  get to it even with this API, just like I do with my AMD IOMMU state by
  upcasting. It doesn't seem to matter whether you've got an opaque, that
  opaque could very well be reachable by upcasting.
  
  Did I get this wrong?
 
 Can you honestly tell me that 
 
  +static int amd_iommu_translate(DMADevice *dev,
  +   dma_addr_t addr,
  +   dma_addr_t *paddr,
  +   dma_addr_t *len,
  +   int is_write)
  +{
  +PCIDevice *pci_dev = container_of(dev, PCIDevice, dma);
  +PCIDevice *iommu_dev = DO_UPCAST(PCIDevice, qdev, dev-mmu-iommu);
  +AMDIOMMUState *s = DO_UPCAST(AMDIOMMUState, dev, iommu_dev);
 
 THREE (3) upcasts is a sane to write maintainable software?
 The margin for error here is absolutely enormous.
 
 If you had just passed in that AMDIOMMUState* as the opaque
 value, it would be trivial to look at the initialization
 statement and the callback function to verify that the right
 value is being passed.

Maybe it's not nice, but you're missing the fact upcasting gives you
some type safety. With opaques you have none. Plus you also get the PCI
device that made the call while you're at it.


Eduard

 r~
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [Qemu-devel] [RFC PATCH 01/13] Generic DMA memory access interface

2011-06-01 Thread Richard Henderson
On 06/01/2011 08:35 AM, Eduard - Gabriel Munteanu wrote:
 Maybe it's not nice, but you're missing the fact upcasting gives you
 some type safety. With opaques you have none.

Lol.  Do you understand what container_of does?
This is not dynamic_cast with RTTI.

You can put any type name in there that you like,
so long as it has a field name to match.  The type
of the field you give doesn't even have to match
the type of the pointer that you pass in.

Type safety this is not.


r~
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[RFC PATCH 01/13] Generic DMA memory access interface

2011-05-31 Thread Eduard - Gabriel Munteanu
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.munte...@linux360.ro
---
 Makefile.target |2 +-
 hw/dma_rw.c |  155 +++
 hw/dma_rw.h |  217 +++
 3 files changed, 373 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 21f864a..ee0c80d 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -224,7 +224,7 @@ obj-i386-y += cirrus_vga.o apic.o ioapic.o piix_pci.o
 obj-i386-y += vmport.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 kvmclock.o
+obj-i386-y += pc_piix.o kvmclock.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 000..824db83
--- /dev/null
+++ b/hw/dma_rw.c
@@ -0,0 +1,155 @@
+/*
+ * 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,
+void *buffer,
+dma_addr_t addr,
+dma_addr_t len,
+DMAInvalidateMapFunc *invalidate,
+void *invalidate_opaque)
+{
+DMAMemoryMap *map;
+
+map = qemu_malloc(sizeof(DMAMemoryMap));
+map-buffer = buffer;
+map-addr   = addr;
+map-len= len;
+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,
+  void *buffer,
+  dma_addr_t len)
+{
+DMAMemoryMap *map;
+
+QLIST_FOREACH(map, dev-mmu-memory_maps, list) {
+if (map-buffer == buffer  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;
+void *buf;
+
+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;
+}
+
+buf = cpu_physical_memory_map(paddr, len, is_write);
+
+/* We treat maps as remote TLBs to cope with stuff like AIO. */
+if (cb) {
+dma_register_memory_map(dev, buf, addr, *len, cb, opaque);
+}
+
+return buf;
+}
+
+void dma_memory_unmap(DMADevice *dev,
+  void *buffer,
+   

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

2011-02-06 Thread Michael S. Tsirkin
On Fri, Feb 04, 2011 at 01:32:55AM +0200, Eduard - Gabriel Munteanu 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.munte...@linux360.ro
 ---
  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 000..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) {
 + 

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

2011-02-06 Thread Michael S. Tsirkin
On Fri, Feb 04, 2011 at 01:32:55AM +0200, Eduard - Gabriel Munteanu 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.munte...@linux360.ro
 ---
  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

Does this need to be target specific?

  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 000..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 

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

2011-02-05 Thread Blue Swirl
On Thu, Feb 3, 2011 at 11:32 PM, Eduard - Gabriel Munteanu
eduard.munte...@linux360.ro 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.munte...@linux360.ro
 ---
  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 000..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. */
 +  

[PATCH 01/13] Generic DMA memory access interface

2011-01-29 Thread Eduard - Gabriel Munteanu
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.munte...@linux360.ro
---
 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 000..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,
+