Skip to contentSkip to conversation

AMD IOMMU emulation patchset

RFC PATCH In review Eduard - Gabriel Munteanu 8 participants 42 messages Jul 14, 2010 – Jul 15, 2010

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover AMD IOMMU emulation patchset

Hi everybody,

This is my work on the AMD IOMMU emulation project. I've put this, along with the SeaBIOS patches (which you need to test), in my Git repos here:

http://repo.or.cz/w/qemu-kvm/amd-iommu.git
http://repo.or.cz/w/seabios/amd-iommu.git

While it works for Linux guests (didn't try anything else), it's not yet complete. But the ported devices work, and so may others that either don't do DMA (VGA for example) or they're not on a PCI bus.

Passing devices from a guest to a nested guest also works. It seems no modifications were needed for that (well, except getting a host kernel on which nested SVM works right ;), and I'd recommend 2.6.34.1), but I only tested with the rtl8139. Will test soon with a doubly nested guest to cover that scenario as well, along with testing other emulated hw.

I'd like your opinions on this. Perhaps you could test it and report if you find any issues. Some things aren't done/complete yet:
- fixing the theoretical AIO issue, but it's not a priority right now
- actually testing that access checking works (should inject faults)
- implementing skipped translation levels
- a translation cache might be a good idea
- implementing features not used by Linux (e.g. interrupt remapping)

That being said, any feedback is welcome.

Thanks,
Eduard

P.S.: I'd also like to thank Paul Brook for his help on figuring out some aspects of the IOMMU layer.

Eduard - Gabriel Munteanu (7):
Generic IOMMU layer
AMD IOMMU emulation
pci: call IOMMU hooks
ide: IOMMU support
rtl8139: IOMMU support
eepro100: IOMMU support
ac97: IOMMU support

 Makefile.target |    3 +
 configure       |   11 +
 hw/ac97.c       |   20 ++-
 hw/amd_iommu.c  |  621 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/eepro100.c   |  141 +++++++++----
 hw/ide/core.c   |   46 +++--
 hw/iommu.c      |   82 ++++++++
 hw/iommu.h      |  260 +++++++++++++++++++++++
 hw/pc.c         |    4 +
 hw/pc.h         |    3 +
 hw/pci.c        |   21 ++
 hw/pci_ids.h    |    2 +
 hw/pci_regs.h   |    1 +
 hw/qdev.h       |    6 +
 hw/rtl8139.c    |   98 ++++++----
 15 files changed, 1225 insertions(+), 94 deletions(-)
 create mode 100644 hw/amd_iommu.c
 create mode 100644 hw/iommu.c
 create mode 100644 hw/iommu.h
#1 lore raw

RFC PATCH 1/7 Generic IOMMU layer

This provides an API for abstracting IOMMU functions. Hardware emulation code can use it to request address translation and access checking. In the absence of an emulated IOMMU, no translation/checking happens and I/O goes through as before.

IOMMU emulation code must provide implementation-specific hooks for this layer.

---
 Makefile.target |    1 +
 hw/iommu.c      |   82 +++++++++++++++++
 hw/iommu.h      |  260 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h       |    6 ++
 4 files changed, 349 insertions(+), 0 deletions(-)
 create mode 100644 hw/iommu.c
 create mode 100644 hw/iommu.h
Makefile.target +1 -0
index 70a9c1b..3f895ae 100644
@@ -183,6 +183,7 @@ obj-$(CONFIG_VIRTFS) += virtio-9p.o
 obj-y += rwhandler.o
 obj-$(CONFIG_KVM) += kvm.o kvm-all.o
 obj-$(CONFIG_NO_KVM) += kvm-stub.o
+obj-$(CONFIG_IOMMU) += iommu.o
 
 # MSI-X depends on kvm for interrupt injection,
 # so moved it from Makefile.objs to Makefile.target for now
hw/iommu.c new file +82 -0

82 lines not on this page · raw message

hw/iommu.h new file +260 -0

260 lines not on this page · raw message

hw/qdev.h +6 -0
index be5ad67..deb71fd 100644
@@ -56,6 +56,8 @@ struct BusInfo {
     Property *props;
 };
 
+struct iommu;
+
 struct BusState {
     DeviceState *parent;
     BusInfo *info;
@@ -64,6 +66,10 @@ struct BusState {
     int qdev_allocated;
     QLIST_HEAD(, DeviceState) children;
     QLIST_ENTRY(BusState) sibling;
+
+#ifdef CONFIG_IOMMU
+    struct iommu *iommu;
+#endif
 };
 
 struct Property {
Signature · 1 line
1.7.1
#2 lore raw

Re: [RFC PATCH 1/7] Generic IOMMU layer

Quoted 8 lines · Eduard - Gabriel Munteanu

This provides an API for abstracting IOMMU functions. Hardware emulation code can use it to request address translation and access checking. In the absence of an emulated IOMMU, no translation/checking happens and I/O goes through as before.

IOMMU emulation code must provide implementation-specific hooks for this layer.

[..snip..]

hw/iommu.c line 56

Quoted 6 lines
hw/iommu.c new file +6 -0
+int __iommu_rw(struct iommu *iommu,
+               DeviceState *dev,
+               target_phys_addr_t addr,
+               uint8_t *buf,
+               int len,
+               int is_write)

Do not use leading double underscore.

[..snip..]

Signature · 1 line
mailto:[email protected]
#3 lore raw

Re: [Qemu-devel] [RFC PATCH 1/7] Generic IOMMU layer

Quoted 20 lines · malc
Quoted 8 lines · Eduard - Gabriel Munteanu

This provides an API for abstracting IOMMU functions. Hardware emulation code can use it to request address translation and access checking. In the absence of an emulated IOMMU, no translation/checking happens and I/O goes through as before.

IOMMU emulation code must provide implementation-specific hooks for this layer.

[..snip..]

hw/iommu.c line 56

Quoted 6 lines
hw/iommu.c new file +6 -0
+int __iommu_rw(struct iommu *iommu,
+               DeviceState *dev,
+               target_phys_addr_t addr,
+               uint8_t *buf,
+               int len,
+               int is_write)
Quoted 6 lines

Do not use leading double underscore.

[..snip..]

Signature · 1 line
mailto:[email protected]

Thanks, will fix it.

Eduard

#4 lore raw

RFC PATCH 2/7 AMD IOMMU emulation

In reply to Eduard - Gabriel Munteanu

This introduces emulation for the AMD IOMMU, described in "AMD I/O Virtualization Technology (IOMMU) Specification".

---
 Makefile.target |    2 +
 configure       |   11 +
 hw/amd_iommu.c  |  621 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pc.c         |    4 +
 hw/pc.h         |    3 +
 hw/pci_ids.h    |    2 +
 hw/pci_regs.h   |    1 +
 7 files changed, 644 insertions(+), 0 deletions(-)
 create mode 100644 hw/amd_iommu.c
Makefile.target +2 -0
index 3f895ae..eb164ba 100644
@@ -220,6 +220,8 @@ obj-i386-y += pcspk.o i8254.o
 obj-i386-$(CONFIG_KVM_PIT) += i8254-kvm.o
 obj-i386-$(CONFIG_KVM_DEVICE_ASSIGNMENT) += device-assignment.o
 
+obj-i386-$(CONFIG_AMD_IOMMU) += amd_iommu.o
+
 # Hardware support
 obj-ia64-y += ide.o pckbd.o vga.o $(SOUND_HW) dma.o $(AUDIODRV)
 obj-ia64-y += fdc.o mc146818rtc.o serial.o i8259.o ipf.o
configure +11 -0
index af50607..a0730b7 100755
@@ -317,6 +317,7 @@ io_thread="no"
 mixemu="no"
 kvm_cap_pit=""
 kvm_cap_device_assignment=""
+amd_iommu="no"
 kerneldir=""
 aix="no"
 blobs="yes"
@@ -629,6 +630,8 @@ for opt do
   ;;
   --enable-kvm-device-assignment) kvm_cap_device_assignment="yes"
   ;;
+  --enable-amd-iommu-emul) amd_iommu="yes"
+  ;;
   --enable-profiler) profiler="yes"
   ;;
   --enable-cocoa)
@@ -871,6 +874,8 @@ echo "  --disable-kvm-pit        disable KVM pit support"
 echo "  --enable-kvm-pit         enable KVM pit support"
 echo "  --disable-kvm-device-assignment  disable KVM device assignment support"
 echo "  --enable-kvm-device-assignment   enable KVM device assignment support"
+echo "  --disable-amd-iommu-emul disable AMD IOMMU emulation"
+echo "  --enable-amd-iommu-emul  enable AMD IOMMU emulation"
 echo "  --disable-nptl           disable usermode NPTL support"
 echo "  --enable-nptl            enable usermode NPTL support"
 echo "  --enable-system          enable all system emulation targets"
@@ -2251,6 +2256,7 @@ echo "Install blobs     $blobs"
 echo "KVM support       $kvm"
 echo "KVM PIT support   $kvm_cap_pit"
 echo "KVM device assig. $kvm_cap_device_assignment"
+echo "AMD IOMMU emul.   $amd_iommu"
 echo "fdt support       $fdt"
 echo "preadv support    $preadv"
 echo "fdatasync         $fdatasync"
@@ -2645,6 +2651,11 @@ case "$target_arch2" in
   x86_64)
     TARGET_BASE_ARCH=i386
     target_phys_bits=64
+    if test "$amd_iommu" = "yes"; then
+      echo "CONFIG_AMD_IOMMU=y" >> $config_target_mak
+      echo "CONFIG_IOMMU=y" >> $config_target_mak
+      echo "CONFIG_IOMMU=y" >> $config_host_mak
+    fi
   ;;
   ia64)
     target_phys_bits=64
hw/amd_iommu.c new file +621 -0

621 lines not on this page · raw message

hw/pc.c +4 -0
index 186e322..4c929f9 100644
@@ -1066,6 +1066,10 @@ void pc_pci_device_init(PCIBus *pci_bus)
     int max_bus;
     int bus;
 
+#ifdef CONFIG_AMD_IOMMU
+    amd_iommu_init(pci_bus);
+#endif
+
     max_bus = drive_get_max_bus(IF_SCSI);
     for (bus = 0; bus <= max_bus; bus++) {
         pci_create_simple(pci_bus, -1, "lsi53c895a");
hw/pc.h +3 -0
index 3ef2f75..255ad93 100644
@@ -191,4 +191,7 @@ void extboot_init(BlockDriverState *bs);
 
 int e820_add_entry(uint64_t, uint64_t, uint32_t);
 
+/* amd_iommu.c */
+void amd_iommu_init(PCIBus *bus);
+
 #endif
hw/pci_ids.h +2 -0
index 39e9f1d..d790312 100644
@@ -26,6 +26,7 @@
 
 #define PCI_CLASS_MEMORY_RAM             0x0500
 
+#define PCI_CLASS_SYSTEM_IOMMU           0x0806
 #define PCI_CLASS_SYSTEM_OTHER           0x0880
 
 #define PCI_CLASS_SERIAL_USB             0x0c03
@@ -56,6 +57,7 @@
 
 #define PCI_VENDOR_ID_AMD                0x1022
 #define PCI_DEVICE_ID_AMD_LANCE          0x2000
+#define PCI_DEVICE_ID_AMD_IOMMU          0x0000     /* FIXME */
 
 #define PCI_VENDOR_ID_MOTOROLA           0x1057
 #define PCI_DEVICE_ID_MOTOROLA_MPC106    0x0002
hw/pci_regs.h +1 -0
index 1c675dc..6399b5d 100644
@@ -216,6 +216,7 @@
 #define  PCI_CAP_ID_SHPC 	0x0C	/* PCI Standard Hot-Plug Controller */
 #define  PCI_CAP_ID_SSVID	0x0D	/* Bridge subsystem vendor/device ID */
 #define  PCI_CAP_ID_AGP3	0x0E	/* AGP Target PCI-PCI bridge */
+#define  PCI_CAP_ID_SEC     0x0F    /* Secure Device (AMD IOMMU) */
 #define  PCI_CAP_ID_EXP 	0x10	/* PCI Express */
 #define  PCI_CAP_ID_MSIX	0x11	/* MSI-X */
 #define  PCI_CAP_ID_AF		0x13	/* PCI Advanced Features */
Signature · 1 line
1.7.1
#5 lore raw

Re: [Qemu-devel] [RFC PATCH 2/7] AMD IOMMU emulation

hw/pc.c void pc_pci_device_init(PCIBus *pci_bus) · line 1071

Quoted 5 lines
hw/pc.c +4 -0
+  --enable-amd-iommu-emul) amd_iommu="yes"
 
+#ifdef CONFIG_AMD_IOMMU
+    amd_iommu_init(pci_bus);
+#endif

This should not be a configure option.

Paul

#6 lore raw

RFC PATCH 3/7 pci: call IOMMU hooks

In reply to Eduard - Gabriel Munteanu

Memory accesses must go through the IOMMU layer.

---
 hw/pci.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)
hw/pci.c +21 -0
index 6871728..9c5d706 100644
@@ -22,6 +22,7 @@
  * THE SOFTWARE.
  */
 #include "hw.h"
+#include "iommu.h"
 #include "pci.h"
 #include "monitor.h"
 #include "net.h"
@@ -733,12 +734,25 @@ static void do_pci_unregister_device(PCIDevice *pci_dev)
     pci_config_free(pci_dev);
 }
 
+#ifdef CONFIG_IOMMU
+static inline int pci_iommu_register_device(PCIBus *bus, PCIDevice *dev)
+{
+    return iommu_register_device(bus->qbus.iommu, &dev->qdev);
+}
+#else
+static inline int pci_iommu_register_device(PCIBus *bus, PCIDevice *dev)
+{
+    return 0;
+}
+#endif
+
 PCIDevice *pci_register_device(PCIBus *bus, const char *name,
                                int instance_size, int devfn,
                                PCIConfigReadFunc *config_read,
                                PCIConfigWriteFunc *config_write)
 {
     PCIDevice *pci_dev;
+    int err;
 
     pci_dev = qemu_mallocz(instance_size);
     pci_dev = do_pci_register_device(pci_dev, bus, name, devfn,
@@ -747,6 +761,13 @@ PCIDevice *pci_register_device(PCIBus *bus, const char *name,
     if (pci_dev == NULL) {
         hw_error("PCI: can't register device\n");
     }
+
+    err = pci_iommu_register_device(bus, pci_dev);
+    if (err) {
+        hw_error("PCI: can't register device with IOMMU\n");
+        return NULL;
+    }
+
     return pci_dev;
 }
 
Signature · 1 line
1.7.1
#7 lore raw

Re: [Qemu-devel] [RFC PATCH 3/7] pci: call IOMMU hooks

Quoted 48 lines · Eduard - Gabriel Munteanu

Memory accesses must go through the IOMMU layer.

---
 hw/pci.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)
hw/pci.c +14 -0
index 6871728..9c5d706 100644
@@ -22,6 +22,7 @@
  * THE SOFTWARE.
  */
 #include "hw.h"
+#include "iommu.h"
 #include "pci.h"
 #include "monitor.h"
 #include "net.h"
@@ -733,12 +734,25 @@ static void do_pci_unregister_device(PCIDevice *pci_dev)
     pci_config_free(pci_dev);
 }
 
+#ifdef CONFIG_IOMMU
+static inline int pci_iommu_register_device(PCIBus *bus, PCIDevice *dev)
+{
+    return iommu_register_device(bus->qbus.iommu, &dev->qdev);
+}
+#else
+static inline int pci_iommu_register_device(PCIBus *bus, PCIDevice *dev)
+{
+    return 0;
+}
+#endif
+
 PCIDevice *pci_register_device(PCIBus *bus, const char *name,
                                int instance_size, int devfn,
                                PCIConfigReadFunc *config_read,
                                PCIConfigWriteFunc *config_write)
 {
     PCIDevice *pci_dev;
+    int err;
 
     pci_dev = qemu_mallocz(instance_size);
     pci_dev = do_pci_register_device(pci_dev, bus, name, devfn,
@@ -747,6 +761,13 @@ PCIDevice *pci_register_device(PCIBus *bus, const char *name,
     if (pci_dev == NULL) {
         hw_error("PCI: can't register device\n");

hw/pci.c PCIDevice *pci_register_device(PCIBus *bus, const char *name, · line 772

Quoted 10 lines
hw/pci.c +7 -0
     }
+
+    err = pci_iommu_register_device(bus, pci_dev);
+    if (err) {
+        hw_error("PCI: can't register device with IOMMU\n");
+        return NULL;
+    }
+
     return pci_dev;
 }

pci_register_device() is pre-qdev api.
qdev'fied device doesn't call pci_register_device().
So please move the initialization hook into do_pci_register_device() which are commonly used by pci_register_device() and pci_qdev_init().

Signature · 1 line
yamahata
#8 lore raw

Re: [Qemu-devel] [RFC PATCH 3/7] pci: call IOMMU hooks

Quoted 1 line · Isaku Yamahata

On Wed, Jul 14, 2010 at 08:45:03AM +0300, Eduard - Gabriel Munteanu wrote:

[snip]

Quoted 20 lines
Quoted 20 lines

PCIDevice *pci_register_device(PCIBus *bus, const char *name,
int instance_size, int devfn, PCIConfigReadFunc *config_read, PCIConfigWriteFunc *config_write)

 {
     PCIDevice *pci_dev;
hw/pci.c +7 -0
+    int err;
 
     pci_dev = qemu_mallocz(instance_size);
     pci_dev = do_pci_register_device(pci_dev, bus, name, devfn,
@@ -747,6 +761,13 @@ PCIDevice *pci_register_device(PCIBus *bus, const char *name,
     if (pci_dev == NULL) {
         hw_error("PCI: can't register device\n");
     }
+
+    err = pci_iommu_register_device(bus, pci_dev);
+    if (err) {
+        hw_error("PCI: can't register device with IOMMU\n");
+        return NULL;
+    }
Quoted 10 lines

hw/pci.c PCIDevice *pci_register_device(PCIBus *bus, const char *name, · line 772

Quoted 3 lines
hw/pci.c +1 -0
+
     return pci_dev;
 }

pci_register_device() is pre-qdev api.
qdev'fied device doesn't call pci_register_device().
So please move the initialization hook into do_pci_register_device() which are commonly used by pci_register_device() and pci_qdev_init().

Signature · 1 line
yamahata

Thanks, I didn't need the functionality and missed this.

Eduard

#9 lore raw

RFC PATCH 4/7 ide: IOMMU support

In reply to Eduard - Gabriel Munteanu

Memory accesses must go through the IOMMU layer.

---
 hw/ide/core.c |   46 +++++++++++++++++++++++++++++++---------------
 1 files changed, 31 insertions(+), 15 deletions(-)
hw/ide/core.c +31 -15
index 0b3b7c2..7f8f7df 100644
@@ -26,6 +26,7 @@
 #include <hw/pc.h>
 #include <hw/pci.h>
 #include <hw/scsi.h>
+#include <hw/iommu.h>
 #include "qemu-timer.h"
 #include "sysemu.h"
 #include "dma.h"
@@ -433,7 +434,12 @@ static int dma_buf_prepare(BMDMAState *bm, int is_write)
         uint32_t addr;
         uint32_t size;
     } prd;
-    int l, len;
+    int l, len, err, io_len;
+    struct iommu *iommu;
+    DeviceState *dev;
+    target_phys_addr_t io_addr;
+
+    iommu = iommu_get(s->bus->qbus.parent, &dev);
 
     qemu_sglist_init(&s->sg, s->nsector / (IDE_PAGE_SIZE / 512) + 1);
     s->io_buffer_size = 0;
@@ -443,7 +449,7 @@ static int dma_buf_prepare(BMDMAState *bm, int is_write)
             if (bm->cur_prd_last ||
                 (bm->cur_addr - bm->addr) >= IDE_PAGE_SIZE)
                 return s->io_buffer_size != 0;
-            cpu_physical_memory_read(bm->cur_addr, (uint8_t *)&prd, 8);
+            err = iommu_read(iommu, dev, bm->cur_addr, (uint8_t *)&prd, 8);
             bm->cur_addr += 8;
             prd.addr = le32_to_cpu(prd.addr);
             prd.size = le32_to_cpu(prd.size);
@@ -455,11 +461,22 @@ static int dma_buf_prepare(BMDMAState *bm, int is_write)
             bm->cur_prd_last = (prd.size & 0x80000000);
         }
         l = bm->cur_prd_len;
-        if (l > 0) {
-            qemu_sglist_add(&s->sg, bm->cur_prd_addr, l);
-            bm->cur_prd_addr += l;
-            bm->cur_prd_len -= l;
-            s->io_buffer_size += l;
+        while (l > 0) {
+            /*
+             * In case translation / access checking fails no
+             * transfer happens but we pretend it went through.
+             */
+            err = iommu_translate(iommu, dev, bm->cur_prd_addr,
+                                  &io_addr, &io_len, !is_write);
+            if (!err) {
+                if (io_len > l)
+                    io_len = l;
+                qemu_sglist_add(&s->sg, io_addr, io_len);
+            }
+            bm->cur_prd_addr += io_len;
+            bm->cur_prd_len -= io_len;
+            s->io_buffer_size += io_len;
+            l -= io_len;
         }
     }
     return 1;
@@ -516,6 +533,10 @@ static int dma_buf_rw(BMDMAState *bm, int is_write)
         uint32_t size;
     } prd;
     int l, len;
+    struct iommu *iommu;
+    DeviceState *dev;
+
+    iommu = iommu_get(s->bus->qbus.parent, &dev);
 
     for(;;) {
         l = s->io_buffer_size - s->io_buffer_index;
@@ -526,7 +547,7 @@ static int dma_buf_rw(BMDMAState *bm, int is_write)
             if (bm->cur_prd_last ||
                 (bm->cur_addr - bm->addr) >= IDE_PAGE_SIZE)
                 return 0;
-            cpu_physical_memory_read(bm->cur_addr, (uint8_t *)&prd, 8);
+            iommu_read(iommu, dev, bm->cur_addr, (uint8_t *)&prd, 8);
             bm->cur_addr += 8;
             prd.addr = le32_to_cpu(prd.addr);
             prd.size = le32_to_cpu(prd.size);
@@ -540,13 +561,8 @@ static int dma_buf_rw(BMDMAState *bm, int is_write)
         if (l > bm->cur_prd_len)
             l = bm->cur_prd_len;
         if (l > 0) {
-            if (is_write) {
-                cpu_physical_memory_write(bm->cur_prd_addr,
-                                          s->io_buffer + s->io_buffer_index, l);
-            } else {
-                cpu_physical_memory_read(bm->cur_prd_addr,
-                                          s->io_buffer + s->io_buffer_index, l);
-            }
+            iommu_rw(iommu, dev, bm->cur_prd_addr,
+                     s->io_buffer + s->io_buffer_index, l, is_write);
             bm->cur_prd_addr += l;
             bm->cur_prd_len -= l;
             s->io_buffer_index += l;
Signature · 1 line
1.7.1
#10 lore raw

Re: [RFC PATCH 4/7] ide: IOMMU support

Quoted 1 line

Memory accesses must go through the IOMMU layer.

No. Devices should not know or care whether an IOMMU is present.

You should be adding a DeviceState argument to cpu_physical_memory_{rw,map}. This should then handle IOMMU translation transparently.

You also need to accomodate the the case where multiple IOMMU are present.

Paul

#11 lore raw

Re: [RFC PATCH 4/7] ide: IOMMU support

Quoted 3 lines · Paul Brook
Quoted 1 line

Memory accesses must go through the IOMMU layer.

No. Devices should not know or care whether an IOMMU is present.

There are real devices that care very much about an IOMMU. Basically all devices supporting ATS care about that. So I don't see a problem if the device emulation code of qemu also cares about present IOMMUs.

Quoted 2 lines

You should be adding a DeviceState argument to cpu_physical_memory_{rw,map}. This should then handle IOMMU translation transparently.

That's not a good idea imho. With an IOMMU the device no longer accesses cpu physical memory. It accesses device virtual memory. Using cpu_physical_memory* functions in device code becomes misleading when the device virtual address space differs from cpu physical. So different functions for devices make a lot of sense here. Another reason for seperate functions is that we can extend them later to support emulation of ATS devices.

Quoted 1 line

You also need to accomodate the the case where multiple IOMMU are present.

This, indeed, is something transparent to the device. This should be handled inside the iommu emulation code.

Joerg

#12 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

Quoted 9 lines · Paul Brook
Quoted 3 lines
Quoted 1 line

Memory accesses must go through the IOMMU layer.

No. Devices should not know or care whether an IOMMU is present.

There are real devices that care very much about an IOMMU. Basically all devices supporting ATS care about that. So I don't see a problem if the device emulation code of qemu also cares about present IOMMUs.

Quoted 8 lines
Quoted 3 lines

You should be adding a DeviceState argument to cpu_physical_memory_{rw,map}. This should then handle IOMMU translation transparently.

That's not a good idea imho. With an IOMMU the device no longer accesses cpu physical memory. It accesses device virtual memory. Using cpu_physical_memory* functions in device code becomes misleading when the device virtual address space differs from cpu physical.

Well, ok, the function name needs fixing too. However I think the only thing missing from the current API is that it does not provide a way to determine which device is performing the access.

Depending how the we decide to handle IOMMU invalidation, it may also be necessary to augment the memory_map API to allow the system to request a mapping be revoked. However this issue is not specific to the IOMMU implementation. Such bugs are already present on any system that allows dynamic reconfiguration of the address space, e.g. by changing PCI BARs.

Quoted 4 lines

So different
functions for devices make a lot of sense here. Another reason for seperate functions is that we can extend them later to support emulation of ATS devices.

I disagree. ATS should be an independent feature, and is inherently bus specific. As usual the PCI spec is not publicly available, but based on the AMD IOMMU docs I'd say that ATS is completely independent of memory accesses - the convention being that you trust an ATS capable device to DTRT, and configure the bus IOMMU to apply a flat mapping for accesses from such devices.

Quoted 5 lines
Quoted 2 lines

You also need to accomodate the the case where multiple IOMMU are present.

This, indeed, is something transparent to the device. This should be handled inside the iommu emulation code.

I think you've got the abstraction boundaries all wrong.

A device performs a memory access on its local bus. It has no knowledge of how that access is routed to its destination. The device should not be aware of any IOMMUs, in the same way that it doesn't know whether it happens to be accessing RAM or memory mapped peripherals on another device.

Each IOMMU is fundamentally part of a bus bridge. For example the bridge between a PCI bus and the system bus. It provides a address mapping from one bus to another.

There should be no direct interaction between an IOMMU and a device (ignoring ATS, which is effectively a separate data channel). Everything should be done via the cpu_phsycial_memory_* code. Likewise on a system with multiple nested IOMMUs there should be no direct interatcion between these. cpu_physical_memory_* should walk the device/bus tree to determine where the access terminates, applying mappings appropriately.

Paul

#13 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

Quoted 15 lines · Paul Brook
Quoted 15 lines · Paul Brook
Quoted 5 lines
Quoted 3 lines

Memory accesses must go through the IOMMU layer.

No. Devices should not know or care whether an IOMMU is present.

There are real devices that care very much about an IOMMU. Basically all devices supporting ATS care about that. So I don't see a problem if the device emulation code of qemu also cares about present IOMMUs.

Quoted 4 lines

You should be adding a DeviceState argument to cpu_physical_memory_{rw,map}. This should then handle IOMMU translation transparently.

Quoted 9 lines
Quoted 5 lines

That's not a good idea imho. With an IOMMU the device no longer accesses cpu physical memory. It accesses device virtual memory. Using cpu_physical_memory* functions in device code becomes misleading when the device virtual address space differs from cpu physical.

Well, ok, the function name needs fixing too. However I think the only thing missing from the current API is that it does not provide a way to determine which device is performing the access.

I agree with Paul.

The right approach IMHO is to convert devices to use bus-specific functions to access memory. The bus specific functions should have a device argument as the first parameter.

For PCI-based IOMMUs, the implementation exists solely within the PCI bus. For platforms (like SPARC) that have lower level IOMMUs, we would need to probably introduce a sysbus memory access layer and then provide a hook to implement an IOMMU there.

Quoted 6 lines

Depending how the we decide to handle IOMMU invalidation, it may also be necessary to augment the memory_map API to allow the system to request a mapping be revoked. However this issue is not specific to the IOMMU implementation. Such bugs are already present on any system that allows dynamic reconfiguration of the address space, e.g. by changing PCI BARs.

That's why the memory_map API today does not allow mappings to persist after trips back to the main loop.

Regards,

Anthony Liguori

Quoted 43 lines
Quoted 5 lines

So different
functions for devices make a lot of sense here. Another reason for seperate functions is that we can extend them later to support emulation of ATS devices.

I disagree. ATS should be an independent feature, and is inherently bus specific. As usual the PCI spec is not publicly available, but based on the AMD IOMMU docs I'd say that ATS is completely independent of memory accesses - the convention being that you trust an ATS capable device to DTRT, and configure the bus IOMMU to apply a flat mapping for accesses from such devices.

Quoted 6 lines
Quoted 3 lines

You also need to accomodate the the case where multiple IOMMU are present.

This, indeed, is something transparent to the device. This should be handled inside the iommu emulation code.

I think you've got the abstraction boundaries all wrong.

A device performs a memory access on its local bus. It has no knowledge of how that access is routed to its destination. The device should not be aware of any IOMMUs, in the same way that it doesn't know whether it happens to be accessing RAM or memory mapped peripherals on another device.

Each IOMMU is fundamentally part of a bus bridge. For example the bridge between a PCI bus and the system bus. It provides a address mapping from one bus to another.

There should be no direct interaction between an IOMMU and a device (ignoring ATS, which is effectively a separate data channel). Everything should be done via the cpu_phsycial_memory_* code. Likewise on a system with multiple nested IOMMUs there should be no direct interatcion between these. cpu_physical_memory_* should walk the device/bus tree to determine where the access terminates, applying mappings appropriately.

Paul
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#14 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

Quoted 15 lines · * Anthony Liguori ([email protected]) wrote:
Quoted 14 lines · Paul Brook
Quoted 14 lines · Paul Brook
Quoted 2 lines
Quoted 1 line

Memory accesses must go through the IOMMU layer.

No. Devices should not know or care whether an IOMMU is present.

There are real devices that care very much about an IOMMU. Basically all devices supporting ATS care about that. So I don't see a problem if the device emulation code of qemu also cares about present IOMMUs.

Quoted 3 lines

You should be adding a DeviceState argument to cpu_physical_memory_{rw,map}. This should then handle IOMMU translation transparently.

That's not a good idea imho. With an IOMMU the device no longer accesses cpu physical memory. It accesses device virtual memory. Using cpu_physical_memory* functions in device code becomes misleading when the device virtual address space differs from cpu physical.

Quoted 5 lines
Quoted 3 lines

Well, ok, the function name needs fixing too. However I think the only thing missing from the current API is that it does not provide a way to determine which device is performing the access.

I agree with Paul.

I do too.

Quoted 3 lines

The right approach IMHO is to convert devices to use bus-specific functions to access memory. The bus specific functions should have a device argument as the first parameter.

As for ATS, the internal api to handle the device's dma reqeust needs a notion of a translated vs. an untranslated request. IOW, if qemu ever had a device with ATS support, the device would use its local cache to translate the dma address and then submit a translated request to the pci bus (effectively doing a raw cpu physical memory* in that case).

thanks,
-chris

#15 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

Quoted 9 lines
Quoted 3 lines

The right approach IMHO is to convert devices to use bus-specific functions to access memory. The bus specific functions should have a device argument as the first parameter.

As for ATS, the internal api to handle the device's dma reqeust needs a notion of a translated vs. an untranslated request. IOW, if qemu ever had a device with ATS support, the device would use its local cache to translate the dma address and then submit a translated request to the pci bus (effectively doing a raw cpu physical memory* in that case).

Really? Can you provide an documentation to support this claim? My impression is that there is no difference between translated and untranslated devices, and the translation is explicitly disabled by software.

Paul

#16 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

Quoted 10 lines · * Paul Brook ([email protected]) wrote:
Quoted 9 lines
Quoted 3 lines

The right approach IMHO is to convert devices to use bus-specific functions to access memory. The bus specific functions should have a device argument as the first parameter.

As for ATS, the internal api to handle the device's dma reqeust needs a notion of a translated vs. an untranslated request. IOW, if qemu ever had a device with ATS support, the device would use its local cache to translate the dma address and then submit a translated request to the pci bus (effectively doing a raw cpu physical memory* in that case).

Quoted 3 lines

Really? Can you provide an documentation to support this claim? My impression is that there is no difference between translated and untranslated devices, and the translation is explicitly disabled by software.

ATS allows an I/O device to request a translation from the IOMMU. The device can then cache that translation and use the translated address in a PCIe memory transaction. PCIe uses a couple of previously reserved bits in the transaction layer packet header to describe the address type for memory transactions. The default (00) maps to legacy PCIe and describes the memory address as untranslated. This is the normal mode, and could then incur a translation if an IOMMU is present and programmed w/ page tables, etc. as is passes through the host bridge.

Another type is simply a transaction requesting a translation. This is new, and allows a device to request (and cache) a translation from the IOMMU for subsequent use.

The third type is a memory transaction tagged as already translated. This is the type of transaction an ATS capable I/O device will generate when it was able to translate the memory address from its own cache.

Of course, there's also an invalidation request that the IOMMU can send to ATS capable I/O devices to invalidate the cached translation.

thanks,
-chris

#17 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

Quoted 18 lines · Chris Wright
Quoted 5 lines

Really? Can you provide an documentation to support this claim? My impression is that there is no difference between translated and untranslated devices, and the translation is explicitly disabled by software.

ATS allows an I/O device to request a translation from the IOMMU. The device can then cache that translation and use the translated address in a PCIe memory transaction. PCIe uses a couple of previously reserved bits in the transaction layer packet header to describe the address type for memory transactions. The default (00) maps to legacy PCIe and describes the memory address as untranslated. This is the normal mode, and could then incur a translation if an IOMMU is present and programmed w/ page tables, etc. as is passes through the host bridge.

Another type is simply a transaction requesting a translation. This is new, and allows a device to request (and cache) a translation from the IOMMU for subsequent use.

Quoted 7 lines

The third type is a memory transaction tagged as already translated. This is the type of transaction an ATS capable I/O device will generate when it was able to translate the memory address from its own cache.

Of course, there's also an invalidation request that the IOMMU can send to ATS capable I/O devices to invalidate the cached translation.

For emulated device, it seems like we can ignore ATS completely, no?

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
#18 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

Quoted 18 lines · * Avi Kivity ([email protected]) wrote:
Quoted 17 lines · Chris Wright
Quoted 4 lines

Really? Can you provide an documentation to support this claim? My impression is that there is no difference between translated and untranslated devices, and the translation is explicitly disabled by software.

ATS allows an I/O device to request a translation from the IOMMU. The device can then cache that translation and use the translated address in a PCIe memory transaction. PCIe uses a couple of previously reserved bits in the transaction layer packet header to describe the address type for memory transactions. The default (00) maps to legacy PCIe and describes the memory address as untranslated. This is the normal mode, and could then incur a translation if an IOMMU is present and programmed w/ page tables, etc. as is passes through the host bridge.

Another type is simply a transaction requesting a translation. This is new, and allows a device to request (and cache) a translation from the IOMMU for subsequent use.

Quoted 8 lines
Quoted 6 lines

The third type is a memory transaction tagged as already translated. This is the type of transaction an ATS capable I/O device will generate when it was able to translate the memory address from its own cache.

Of course, there's also an invalidation request that the IOMMU can send to ATS capable I/O devices to invalidate the cached translation.

For emulated device, it seems like we can ignore ATS completely, no?

Not if you want to emulate an ATS capable device ;)

Eariler upthread I said:

IOW, if qemu ever had a device with ATS support...

So, that should've been a much bigger _IF_

thanks,
-chris

#19 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

Quoted 5 lines · Chris Wright
Quoted 3 lines

For emulated device, it seems like we can ignore ATS completely, no?

Not if you want to emulate an ATS capable device ;)

What I meant was that the whole request translation, invalidate, dma using a translated address thing is invisible to software. We can emulate an ATS capable device by going through the iommu every time.

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
#20 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

Quoted 8 lines · * Avi Kivity ([email protected]) wrote:
Quoted 3 lines · Chris Wright
Quoted 2 lines

For emulated device, it seems like we can ignore ATS completely, no?

Not if you want to emulate an ATS capable device ;)

What I meant was that the whole request translation, invalidate, dma using a translated address thing is invisible to software. We can emulate an ATS capable device by going through the iommu every time.

Well, I don't see any reason to completely ignore it. It'd be really useful for testing (I'd use it that way). Esp to verify the invalidation of the device IOTLBs.

But I think it's not a difficult thing to emulate once we have a proper api encapsulating a device's dma request.

thanks,
-chris

#21 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

In reply to Chris Wright
Quoted 6 lines · Chris Wright
Quoted 2 lines · * Avi Kivity ([email protected]) wrote:

For emulated device, it seems like we can ignore ATS completely, no?

Not if you want to emulate an ATS capable device ;)

Quoted 8 lines

Eariler upthread I said:

IOW, if qemu ever had a device with ATS support...

So, that should've been a much bigger _IF_

thanks,
-chris

I think we can augment some devices with ATS capability if there are performance gains in doing so. This doesn't seem to be a detail the actual guest OS would be interested in, so we can do it even for devices that existed long before the AMD IOMMU came into existence. But I'm not really sure about this, it's just a thought.

Linux seems to be issuing IOTLB invalidation commands anyway.

Eduard

#22 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

In reply to Avi Kivity
Quoted 1 line · Avi Kivity

For emulated device, it seems like we can ignore ATS completely, no?

An important use-case for emulation is software testing and caching of iommu's is an important part that needs to be handled in software. For this purpose it makes sense to emulate the behavior of caches too. So we probably should not ignore the possibility of an emulated ATS device completly.

Joerg

#23 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

In reply to Chris Wright
Quoted 5 lines · * Chris Wright ([email protected]) wrote:
Quoted 4 lines · * Paul Brook ([email protected]) wrote:
Quoted 4 lines
Quoted 3 lines

The right approach IMHO is to convert devices to use bus-specific functions to access memory. The bus specific functions should have a device argument as the first parameter.

Quoted 7 lines
Quoted 7 lines
Quoted 5 lines

As for ATS, the internal api to handle the device's dma reqeust needs a notion of a translated vs. an untranslated request. IOW, if qemu ever had a device with ATS support, the device would use its local cache to translate the dma address and then submit a translated request to the pci bus (effectively doing a raw cpu physical memory* in that case).

Really? Can you provide an documentation to support this claim?

Wow...color me surprised...there's actually some apparently public "training" docs that might help give a more complete view:

http://www.pcisig.com/developers/main/training_materials/get_document?doc_id=0ab681ba7001e40cdb297ddaf279a8de82a7dc40

ATS discussion starts on slide 23.

Quoted 2 lines
Quoted 2 lines

My impression is that there is no difference between translated and untranslated devices, and the translation is explicitly disabled by software.

And now that I re-read that sentence, I see what you are talking about. Yes, there is the above notion as well.

A device can live in a 1:1 mapping of device address space to physical memory. This could be achieved in a few ways (all done by the OS software programming the IOMMU).

One is to simply create a set of page tables that map 1:1 all of device memory to physical memory. Another is to somehow mark the device as special (either omit translation tables or mark the translation entry as effectively "do not translate"). This is often referred to as Pass Through mode. But this is not the same as ATS.

Pass Through mode is the functional equivalent of disabling the translation/isolation capabilities of the IOMMU. It's typically used when an OS wants to keep a device for itself and isn't interested in the isolation properties of the IOMMU. It then only creates isolating translation tables for devices it's giving to unprivileged software (e.g. Linux/KVM giving a device to a guest, Linux giving a device to user space process, etc.)

Quoted 13 lines

ATS allows an I/O device to request a translation from the IOMMU. The device can then cache that translation and use the translated address in a PCIe memory transaction. PCIe uses a couple of previously reserved bits in the transaction layer packet header to describe the address type for memory transactions. The default (00) maps to legacy PCIe and describes the memory address as untranslated. This is the normal mode, and could then incur a translation if an IOMMU is present and programmed w/ page tables, etc. as is passes through the host bridge.

Another type is simply a transaction requesting a translation. This is new, and allows a device to request (and cache) a translation from the IOMMU for subsequent use.

Quoted 6 lines

The third type is a memory transaction tagged as already translated. This is the type of transaction an ATS capable I/O device will generate when it was able to translate the memory address from its own cache.

Of course, there's also an invalidation request that the IOMMU can send to ATS capable I/O devices to invalidate the cached translation.

thanks,
-chris

#24 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

In reply to Anthony Liguori
Quoted 10 lines · Anthony Liguori
Quoted 3 lines · Paul Brook

Well, ok, the function name needs fixing too. However I think the only thing missing from the current API is that it does not provide a way to determine which device is performing the access.

I agree with Paul.

The right approach IMHO is to convert devices to use bus-specific functions to access memory. The bus specific functions should have a device argument as the first parameter.

If this means a seperate interface for device dma accesses and not fold that functionality into the cpu_physical_memory* interface I agree too :-)

Joerg

#25 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

Quoted 8 lines · Joerg Roedel
Quoted 6 lines · Anthony Liguori
Quoted 4 lines · Paul Brook

Well, ok, the function name needs fixing too. However I think the only thing missing from the current API is that it does not provide a way to determine which device is performing the access.

Quoted 9 lines
Quoted 6 lines

I agree with Paul.

The right approach IMHO is to convert devices to use bus-specific functions to access memory. The bus specific functions should have a device argument as the first parameter.

If this means a seperate interface for device dma accesses and not fold that functionality into the cpu_physical_memory* interface I agree too :-)

No. PCI devices should never call cpu_physical_memory*.

PCI devices should call pci_memory*.

ISA devices should call isa_memory*.

All device memory accesses should go through their respective buses. There can be multiple IOMMUs at different levels of the device hierarchy. If you don't provide bus-level memory access functions that chain through the hierarchy, it's extremely difficult to implement all the necessary hooks to perform the translations at different places.

Regards,

Anthony Liguori

Quoted 3 lines

Joerg

#26 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

Quoted 6 lines · Anthony Liguori
Quoted 3 lines · Joerg Roedel

If this means a seperate interface for device dma accesses and not fold that functionality into the cpu_physical_memory* interface I agree too :-)

No. PCI devices should never call cpu_physical_memory*.

Fully agreed.

Quoted 3 lines

PCI devices should call pci_memory*.

ISA devices should call isa_memory*.

This is a seperate interface. I like the idea and as you stated below it has clear advantages, so lets go this way.

Quoted 5 lines

All device memory accesses should go through their respective buses. There can be multiple IOMMUs at different levels of the device hierarchy. If you don't provide bus-level memory access functions that chain through the hierarchy, it's extremely difficult to implement all the necessary hooks to perform the translations at different places.

Joerg

#27 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

In reply to Anthony Liguori
Quoted 13 lines · Anthony Liguori

No. PCI devices should never call cpu_physical_memory*.

PCI devices should call pci_memory*.

ISA devices should call isa_memory*.

All device memory accesses should go through their respective buses. There can be multiple IOMMUs at different levels of the device hierarchy. If you don't provide bus-level memory access functions that chain through the hierarchy, it's extremely difficult to implement all the necessary hooks to perform the translations at different places.

Quoted 4 lines

Regards,

Anthony Liguori

I liked Paul's initial approach more, at least if I understood him correctly. Basically I'm suggesting a single memory_* function that simply asks the bus for I/O and translation. Say you have something like this:

+ Bus 1
|
---- Memory 1
|
---+ Bus 2 bridge
|
---- Memory 2
|
---+ Bus 3 bridge

      |
      ---- Device

Say Device wants to write to memory. If we have the DeviceState we needn't concern whether this is a BusOneDevice or BusTwoDevice from device code itself. We would just call

memory_rw(dev_state, addr, buf, size, is_write);

which simply recurses through DeviceState's and BusState's through their parent pointers. The actual bus can set up those to provide identification information and perhaps hooks for translation and access checking. So memory_rw() looks like this (pseudocode):

static void memory_rw(DeviceState *dev,

                      target_phys_addr_t addr,
		      uint8_t *buf,
		      int size,
		      int is_write)
{
	BusState *bus = get_parent_bus_of_dev(dev);
	DeviceState *pdev = get_parent_dev(dev);
	target_phys_addr_t taddr;

	if (!bus) {
		/* This shouldn't happen. */
		assert(0);
	}

	if (bus->responsible_for(addr)) {
		raw_physical_memory_rw(addr, buf, size, is_write);
		return;
	}

	taddr = bus->translate(dev, addr);
	memory_rw(pdev, taddr, buf, size, is_write);
}

If we do this, it seems there's no need to provide separate functions. The actual buses must instead initialize those hooks properly. Translation here is something inherent to the bus, that handles arbitration between possibly multiple IOMMUs. Our memory would normally reside on / belong to the top-level bus.

What do you think? (Naming could be better though.)

Eduard

#28 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

Quoted 36 lines · Eduard - Gabriel Munteanu
Quoted 12 lines · Anthony Liguori

No. PCI devices should never call cpu_physical_memory*.

PCI devices should call pci_memory*.

ISA devices should call isa_memory*.

All device memory accesses should go through their respective buses. There can be multiple IOMMUs at different levels of the device hierarchy. If you don't provide bus-level memory access functions that chain through the hierarchy, it's extremely difficult to implement all the necessary hooks to perform the translations at different places.

Quoted 5 lines

Regards,

Anthony Liguori

I liked Paul's initial approach more, at least if I understood him correctly. Basically I'm suggesting a single memory_* function that simply asks the bus for I/O and translation. Say you have something like this:

+ Bus 1
|
---- Memory 1
|
---+ Bus 2 bridge

    |
    ---- Memory 2
    |
    ---+ Bus 3 bridge
       |
       ---- Device
Quoted 6 lines

Say Device wants to write to memory. If we have the DeviceState we needn't concern whether this is a BusOneDevice or BusTwoDevice from device code itself. We would just call

memory_rw(dev_state, addr, buf, size, is_write);

I dislike this API for a few reasons:

1) buses have different types of addresses with different address ranges. this api would have to take a generic address type.
2) dev_state would be the qdev device state. this means qdev needs to have memory hook mechanisms that's chainable. I think it's unnecessary at the qdev level
3) users have upcasted device states, so it's more natural to pass PCIDevice than DeviceState.
4) there's an assumption that all devices can get to DeviceState. that's not always true today.

Quoted 20 lines

which simply recurses through DeviceState's and BusState's through their parent pointers. The actual bus can set up those to provide identification information and perhaps hooks for translation and access checking. So memory_rw() looks like this (pseudocode):

static void memory_rw(DeviceState *dev,

                       target_phys_addr_t addr,
		      uint8_t *buf,
		      int size,
		      int is_write)
{
	BusState *bus = get_parent_bus_of_dev(dev);
	DeviceState *pdev = get_parent_dev(dev);
	target_phys_addr_t taddr;

	if (!bus) {
		/* This shouldn't happen. */
		assert(0);
	}
Quoted 8 lines
	if (bus->responsible_for(addr)) {
		raw_physical_memory_rw(addr, buf, size, is_write);
		return;
	}

	taddr = bus->translate(dev, addr);
	memory_rw(pdev, taddr, buf, size, is_write);

This is too simplistic because you sometimes have layering that doesn't fit into the bus model. For instance, virtio + pci.

We really want a virtio_memory_rw that calls either syborg_memory_rw or pci_memory_rw based on the transport. In your proposal, we would have to model virtio-pci as a bus with a single device which appears awkward to me.

Regards,

Anthony Liguori

Quoted 14 lines

}

If we do this, it seems there's no need to provide separate functions. The actual buses must instead initialize those hooks properly. Translation here is something inherent to the bus, that handles arbitration between possibly multiple IOMMUs. Our memory would normally reside on / belong to the top-level bus.

What do you think? (Naming could be better though.)

Eduard

#29 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

In reply to Anthony Liguori
Quoted 8 lines
Quoted 5 lines

Depending how the we decide to handle IOMMU invalidation, it may also be necessary to augment the memory_map API to allow the system to request a mapping be revoked. However this issue is not specific to the IOMMU implementation. Such bugs are already present on any system that allows dynamic reconfiguration of the address space, e.g. by changing PCI BARs.

That's why the memory_map API today does not allow mappings to persist after trips back to the main loop.

Sure it does. If you can't combine zero-copy memory access with asynchronous IO then IMO it's fairly useless. See e.g. dma-helpers.c

Paul

#30 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

Quoted 6 lines · Paul Brook
Quoted 6 lines
Quoted 6 lines

Depending how the we decide to handle IOMMU invalidation, it may also be necessary to augment the memory_map API to allow the system to request a mapping be revoked. However this issue is not specific to the IOMMU implementation. Such bugs are already present on any system that allows dynamic reconfiguration of the address space, e.g. by changing PCI BARs.

Quoted 6 lines
Quoted 3 lines

That's why the memory_map API today does not allow mappings to persist after trips back to the main loop.

Sure it does. If you can't combine zero-copy memory access with asynchronous IO then IMO it's fairly useless. See e.g. dma-helpers.c

DMA's a very special case. DMA is performed asynchronously to the execution of the CPU so you generally can't make any guarantees about what state the transaction is in until it's completed. That gives us a fair bit of wiggle room when dealing with a DMA operation to a region of physical memory where the physical memory mapping is altered in some way during the transaction.

However, that is not true in the general case.

Regards,

Anthony Liguori

Quoted 2 lines

Paul

#31 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines

Depending how the we decide to handle IOMMU invalidation, it may also be necessary to augment the memory_map API to allow the system to request a mapping be revoked. However this issue is not specific to the IOMMU implementation. Such bugs are already present on any system that allows dynamic reconfiguration of the address space, e.g. by changing PCI BARs.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

That's why the memory_map API today does not allow mappings to persist after trips back to the main loop.

Sure it does. If you can't combine zero-copy memory access with asynchronous IO then IMO it's fairly useless. See e.g. dma-helpers.c

DMA's a very special case.

Special compared to what? The whole purpose of this API is to provide DMA.

Quoted 6 lines

DMA is performed asynchronously to the
execution of the CPU so you generally can't make any guarantees about what state the transaction is in until it's completed. That gives us a fair bit of wiggle room when dealing with a DMA operation to a region of physical memory where the physical memory mapping is altered in some way during the transaction.

You do have ordering constraints though. While it may not be possible to directly determine whether the DMA completed before or after the remapping, and you might not be able to make any assumptions about the atomicity of the transaction as a whole, it is reasonable to assume that any writes to the old mapping will occur before the remapping operation completes.

While things like store buffers potentially allows reordering and deferral of accesses, there are generally fairly tight constraints on this. For example a PCI hast bridge may buffer CPU writes. However it will guarantee that those writes have been flushed out before a subsequent read operation completes.

Consider the case where the hypervisor allows passthough of a device, using the IOMMU to support DMA from that device into virtual machine RAM. When that virtual machine is destroyed the IOMMU mapping for that device will be invalidated. Once the invalidation has completed that RAM can be reused by the hypervisor for other purposes. This may happen before the device is reset. We probably don't really care what happens to the device in this case, but we do need to prevent the device stomping on ram it no longer owns.

There are two ways this can be handled:

If your address translation mechanism allows updates to be deferred indefinitely then we can stall until all relevant DMA transactions have completed. This is probably sufficient for well behaved guests, but potentially opens up a significant window for DoS attacks.

If you need the remapping to occur in a finite timeframe (in the PCI BAR case this is probably before the next CPU access to that bus) then you need some mechanism for revoking the host mapping provided by cpu_physical_memory_map.

Note that a QEMU DMA transaction typically encompasses a whole block of data. The transaction is started when the AIO request is issued, and remains live until the transfer completes. This includes the time taken to fetch the data from external media/devices.

On real hardware a DMA transaction typically only covers a single burst memory write (maybe 16 bytes). This will generally not start until the device has buffered sufficient data to satisfy the burst (or has sufficient buffer space to receive the whole burst).

Paul

#32 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

In reply to Paul Brook
Quoted 9 lines · Paul Brook

Well, ok, the function name needs fixing too. However I think the only thing missing from the current API is that it does not provide a way to determine which device is performing the access.

Depending how the we decide to handle IOMMU invalidation, it may also be necessary to augment the memory_map API to allow the system to request a mapping be revoked. However this issue is not specific to the IOMMU implementation. Such bugs are already present on any system that allows dynamic reconfiguration of the address space, e.g. by changing PCI BARs.

Yeah, having a way to alter existing maps would be good. Basically it's the only place where we truly care about the existence of an IOMMU, and that's due to AIO.

But it's really tricky to do, unfortunately. I also think having an abort_doing_io_on_map() kind of notifier would be sufficient. It should notify back when I/O has been completely stopped.

This should be enough, since we don't expect the results of IDE-like DMA to be recoverable in case a mapping change occurs, even on real hardware.

Quoted 6 lines

I disagree. ATS should be an independent feature, and is inherently bus specific. As usual the PCI spec is not publicly available, but based on the AMD IOMMU docs I'd say that ATS is completely independent of memory accesses - the convention being that you trust an ATS capable device to DTRT, and configure the bus IOMMU to apply a flat mapping for accesses from such devices.

ATS is documented in the Hypertransport specs which are publicly available.

[snip]

Quoted 9 lines

A device performs a memory access on its local bus. It has no knowledge of how that access is routed to its destination. The device should not be aware of any IOMMUs, in the same way that it doesn't know whether it happens to be accessing RAM or memory mapped peripherals on another device.

Each IOMMU is fundamentally part of a bus bridge. For example the bridge between a PCI bus and the system bus. It provides a address mapping from one bus to another.

Quoted 8 lines

There should be no direct interaction between an IOMMU and a device (ignoring ATS, which is effectively a separate data channel). Everything should be done via the cpu_phsycial_memory_* code. Likewise on a system with multiple nested IOMMUs there should be no direct interatcion between these. cpu_physical_memory_* should walk the device/bus tree to determine where the access terminates, applying mappings appropriately.

Paul

Admittedly I could make __iommu_rw() repeateadly call itself instead of doing cpu_physical_memory_rw(). That's what I actually intended, and it should handle IOMMU nesting. It's a trivial change to do so.

Note that emulating hardware realistically defeats some performance purposes. It'd make AIO impossible, if we imagine some sort of message passing scenario (which would be just like the real thing, but a lot slower).

Eduard

#33 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

In reply to Paul Brook
Quoted 4 lines · Paul Brook

A device performs a memory access on its local bus. It has no knowledge of how that access is routed to its destination. The device should not be aware of any IOMMUs, in the same way that it doesn't know whether it happens to be accessing RAM or memory mapped peripherals on another device.

Right.

Quoted 3 lines

Each IOMMU is fundamentally part of a bus bridge. For example the bridge between a PCI bus and the system bus. It provides a address mapping from one bus to another.

An IOMMU is not necessarily part of a bus bridge. By concept an IOMMU can also be implemented on a plugin-card translating only that card. Real implementations that I am aware of always implement the IOMMU in the PCI root bridge, though.

Quoted 6 lines

There should be no direct interaction between an IOMMU and a device (ignoring ATS, which is effectively a separate data channel). Everything should be done via the cpu_phsycial_memory_* code. Likewise on a system with multiple nested IOMMUs there should be no direct interatcion between these. cpu_physical_memory_* should walk the device/bus tree to determine where the access terminates, applying mappings appropriately.

Thats the point where I disagree. I think there should be a seperate set of functions independent from cpu_physical_memory_* to handle device memory accesses. This would keep the changes small and non-intrusive. Beside that, real memory controlers can also handle cpu memory accesses different from device memory accesses. The AMD northbridge GART uses this to decide whether it needs to remap a request or not. The GART can be configured to translate cpu and device accesses seperatly.

Joerg

#34 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

Quoted 9 lines · Paul Brook
Quoted 5 lines

A device performs a memory access on its local bus. It has no knowledge of how that access is routed to its destination. The device should not be aware of any IOMMUs, in the same way that it doesn't know whether it happens to be accessing RAM or memory mapped peripherals on another device.

Right.

Quoted 8 lines
Quoted 3 lines

Each IOMMU is fundamentally part of a bus bridge. For example the bridge between a PCI bus and the system bus. It provides a address mapping from one bus to another.

An IOMMU is not necessarily part of a bus bridge. By concept an IOMMU can also be implemented on a plugin-card translating only that card. Real implementations that I am aware of always implement the IOMMU in the PCI root bridge, though.

If the IOMMU is implemented on the card, then it isn't an interesting case. It's effectively just a complex form of scatter-gather.

If the on-card MMU can delegate pagetable walks to an external device then IMO that's also an unrelated feature, and requires an additional data channel.

Quoted 8 lines
Quoted 7 lines

There should be no direct interaction between an IOMMU and a device (ignoring ATS, which is effectively a separate data channel). Everything should be done via the cpu_phsycial_memory_* code. Likewise on a system with multiple nested IOMMUs there should be no direct interatcion between these.
cpu_physical_memory_* should walk the device/bus tree to determine where the access terminates, applying mappings appropriately.

Quoted 7 lines

Thats the point where I disagree. I think there should be a seperate set of functions independent from cpu_physical_memory_* to handle device memory accesses. This would keep the changes small and non-intrusive. Beside that, real memory controlers can also handle cpu memory accesses different from device memory accesses. The AMD northbridge GART uses this to decide whether it needs to remap a request or not. The GART can be configured to translate cpu and device accesses seperatly.

My point still stands. You should not be pushing the IOMMU handling into device specific code. All you need to do is make the memory access routines aware of which device caused the access.

The fact that the GART can translate CPU accesses proves my point. If you have separate code for CPU and devices, then you need to duplicate the GART handling code.

Paul

#35 lore raw

Re: [Qemu-devel] Re: [RFC PATCH 4/7] ide: IOMMU support

Quoted 10 lines · Paul Brook
Quoted 4 lines

An IOMMU is not necessarily part of a bus bridge. By concept an IOMMU can also be implemented on a plugin-card translating only that card. Real implementations that I am aware of always implement the IOMMU in the PCI root bridge, though.

If the IOMMU is implemented on the card, then it isn't an interesting case. It's effectively just a complex form of scatter-gather.

If the on-card MMU can delegate pagetable walks to an external device then IMO that's also an unrelated feature, and requires an additional data channel.

But that would be handled by the same IOMMU emulation code, so the hooks need to be usable there too.

Quoted 3 lines

My point still stands. You should not be pushing the IOMMU handling into device specific code. All you need to do is make the memory access routines aware of which device caused the access.

Right, the device does not need to know too much about the IOMMU in the general case. The iommu_get/iommu_read/iommu_write interface should replaced by the pci_memory* functions like suggested by Anthony.

Quoted 3 lines

The fact that the GART can translate CPU accesses proves my point. If you have separate code for CPU and devices, then you need to duplicate the GART handling code.

You can configure the GART to translate device accesses only, cpu accesses only, or to translate both. This is hard to handle if cpu and device emulation use the same memory access functions.

Joerg

#36 lore raw

Re: [RFC PATCH 4/7] ide: IOMMU support

In reply to Paul Brook
Quoted 3 lines · Paul Brook
Quoted 1 line

Memory accesses must go through the IOMMU layer.

No. Devices should not know or care whether an IOMMU is present.

They don't really care. iommu_get() et al. are convenience functions which can and do return NULL when there's no IOMMU and device code can pass that NULL around without checking. I could've probably made the r/w functions take only the DeviceState in addition to normal args, but wanted to avoid looking up the related structures on each I/O operation.

Quoted 6 lines

You should be adding a DeviceState argument to cpu_physical_memory_{rw,map}. This should then handle IOMMU translation transparently.

You also need to accomodate the the case where multiple IOMMU are present.

Paul

We don't assume there's a single IOMMU in the generic layer. The callbacks within 'struct iommu' could very well dispatch the request to one of multiple, coexisting IOMMUs.

Eduard

#37 lore raw

Re: [RFC PATCH 4/7] ide: IOMMU support

Quoted 8 lines · Paul Brook
Quoted 3 lines
Quoted 1 line

Memory accesses must go through the IOMMU layer.

No. Devices should not know or care whether an IOMMU is present.

They don't really care. iommu_get() et al. are convenience functions which can and do return NULL when there's no IOMMU and device code can pass that NULL around without checking.

Devices should not need to know any of this. You're introducing a significant amount of duplication and complexity into every device.

The assumption that all accesses will go through the same IOMMU is also false. Accesses to devices on the same bus will not be translated by the IOMMU. Currently there are probably also other things that will break in this case, but your API seems fundamentally incapable of handling this.

Quoted 3 lines

I could've probably made the r/w
functions take only the DeviceState in addition to normal args, but wanted to avoid looking up the related structures on each I/O operation.

That's exactly what you should be doing. If this is inefficient then there are much better ways of fixing this. e.g. by not having the device perform so many accesses, or by adding some sort of translation cache.

Quoted 10 lines
Quoted 6 lines

You should be adding a DeviceState argument to cpu_physical_memory_{rw,map}. This should then handle IOMMU translation transparently.

You also need to accomodate the the case where multiple IOMMU are present.

We don't assume there's a single IOMMU in the generic layer. The callbacks within 'struct iommu' could very well dispatch the request to one of multiple, coexisting IOMMUs.

So you've now introduced yet another copy of the translation code. Not only does every device have to be IOMMU aware, but every IOMMU also has to be aware of nested IOMMUs.

Paul

#38 lore raw

RFC PATCH 5/7 rtl8139: IOMMU support

In reply to Eduard - Gabriel Munteanu

Memory accesses must go through the IOMMU layer.

---
 hw/rtl8139.c |   98 ++++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 62 insertions(+), 36 deletions(-)
hw/rtl8139.c +62 -36
index 72e2242..0f78a69 100644
@@ -48,6 +48,7 @@
  */
 
 #include "hw.h"
+#include "iommu.h"
 #include "pci.h"
 #include "qemu-timer.h"
 #include "net.h"
@@ -416,7 +417,7 @@ typedef struct RTL8139TallyCounters
 static void RTL8139TallyCounters_clear(RTL8139TallyCounters* counters);
 
 /* Writes tally counters to specified physical memory address */
-static void RTL8139TallyCounters_physical_memory_write(target_phys_addr_t tc_addr, RTL8139TallyCounters* counters);
+static void RTL8139TallyCounters_physical_memory_write(DeviceState *qdev, target_phys_addr_t tc_addr, RTL8139TallyCounters* counters);
 
 typedef struct RTL8139State {
     PCIDevice dev;
@@ -746,6 +747,11 @@ static int rtl8139_cp_transmitter_enabled(RTL8139State *s)
 
 static void rtl8139_write_buffer(RTL8139State *s, const void *buf, int size)
 {
+    struct iommu *iommu;
+    DeviceState *dev;
+
+    iommu = iommu_get(&s->dev.qdev, &dev);
+
     if (s->RxBufAddr + size > s->RxBufferSize)
     {
         int wrapped = MOD2(s->RxBufAddr + size, s->RxBufferSize);
@@ -757,15 +763,15 @@ static void rtl8139_write_buffer(RTL8139State *s, const void *buf, int size)
 
             if (size > wrapped)
             {
-                cpu_physical_memory_write( s->RxBuf + s->RxBufAddr,
-                                           buf, size-wrapped );
+                iommu_write(iommu, dev, s->RxBuf + s->RxBufAddr,
+                            buf, size - wrapped);
             }
 
             /* reset buffer pointer */
             s->RxBufAddr = 0;
 
-            cpu_physical_memory_write( s->RxBuf + s->RxBufAddr,
-                                       buf + (size-wrapped), wrapped );
+            iommu_write(iommu, dev, s->RxBuf + s->RxBufAddr,
+                        buf + (size - wrapped), wrapped);
 
             s->RxBufAddr = wrapped;
 
@@ -774,7 +780,7 @@ static void rtl8139_write_buffer(RTL8139State *s, const void *buf, int size)
     }
 
     /* non-wrapping path or overwrapping enabled */
-    cpu_physical_memory_write( s->RxBuf + s->RxBufAddr, buf, size );
+    iommu_write(iommu, dev, s->RxBuf + s->RxBufAddr, buf, size);
 
     s->RxBufAddr += size;
 }
@@ -822,6 +828,11 @@ static ssize_t rtl8139_do_receive(VLANClientState *nc, const uint8_t *buf, size_
     static const uint8_t broadcast_macaddr[6] =
         { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
 
+    struct iommu *iommu;
+    DeviceState *dev;
+
+    iommu = iommu_get(&s->dev.qdev, &dev);
+
     DEBUG_PRINT((">>> RTL8139: received len=%d\n", size));
 
     /* test if board clock is stopped */
@@ -968,13 +979,13 @@ static ssize_t rtl8139_do_receive(VLANClientState *nc, const uint8_t *buf, size_
 
         uint32_t val, rxdw0,rxdw1,rxbufLO,rxbufHI;
 
-        cpu_physical_memory_read(cplus_rx_ring_desc,    (uint8_t *)&val, 4);
+        iommu_read(iommu, dev, cplus_rx_ring_desc, (uint8_t *) &val, 4);
         rxdw0 = le32_to_cpu(val);
-        cpu_physical_memory_read(cplus_rx_ring_desc+4,  (uint8_t *)&val, 4);
+        iommu_read(iommu, dev, cplus_rx_ring_desc + 4, (uint8_t *) &val, 4);
         rxdw1 = le32_to_cpu(val);
-        cpu_physical_memory_read(cplus_rx_ring_desc+8,  (uint8_t *)&val, 4);
+        iommu_read(iommu, dev, cplus_rx_ring_desc + 8,  (uint8_t *) &val, 4);
         rxbufLO = le32_to_cpu(val);
-        cpu_physical_memory_read(cplus_rx_ring_desc+12, (uint8_t *)&val, 4);
+        iommu_read(iommu, dev, cplus_rx_ring_desc + 12, (uint8_t *) &val, 4);
         rxbufHI = le32_to_cpu(val);
 
         DEBUG_PRINT(("RTL8139: +++ C+ mode RX descriptor %d %08x %08x %08x %08x\n",
@@ -1019,7 +1030,7 @@ static ssize_t rtl8139_do_receive(VLANClientState *nc, const uint8_t *buf, size_
         target_phys_addr_t rx_addr = rtl8139_addr64(rxbufLO, rxbufHI);
 
         /* receive/copy to target memory */
-        cpu_physical_memory_write( rx_addr, buf, size );
+        iommu_write(iommu, dev, rx_addr, buf, size);
 
         if (s->CpCmd & CPlusRxChkSum)
         {
@@ -1032,7 +1043,7 @@ static ssize_t rtl8139_do_receive(VLANClientState *nc, const uint8_t *buf, size_
 #else
         val = 0;
 #endif
-        cpu_physical_memory_write( rx_addr+size, (uint8_t *)&val, 4);
+        iommu_write(iommu, dev, rx_addr + size, (uint8_t *) &val, 4);
 
 /* first segment of received packet flag */
 #define CP_RX_STATUS_FS (1<<29)
@@ -1081,9 +1092,9 @@ static ssize_t rtl8139_do_receive(VLANClientState *nc, const uint8_t *buf, size_
 
         /* update ring data */
         val = cpu_to_le32(rxdw0);
-        cpu_physical_memory_write(cplus_rx_ring_desc,    (uint8_t *)&val, 4);
+        iommu_write(iommu, dev, cplus_rx_ring_desc, (uint8_t *) &val, 4);
         val = cpu_to_le32(rxdw1);
-        cpu_physical_memory_write(cplus_rx_ring_desc+4,  (uint8_t *)&val, 4);
+        iommu_write(iommu, dev, cplus_rx_ring_desc + 4, (uint8_t *) &val, 4);
 
         /* update tally counter */
         ++s->tally_counters.RxOk;
@@ -1279,50 +1290,54 @@ static void RTL8139TallyCounters_clear(RTL8139TallyCounters* counters)
     counters->TxUndrn = 0;
 }
 
-static void RTL8139TallyCounters_physical_memory_write(target_phys_addr_t tc_addr, RTL8139TallyCounters* tally_counters)
+static void RTL8139TallyCounters_physical_memory_write(DeviceState *qdev, target_phys_addr_t tc_addr, RTL8139TallyCounters* tally_counters)
 {
+    struct iommu *iommu;
+    DeviceState *dev;
     uint16_t val16;
     uint32_t val32;
     uint64_t val64;
 
+    iommu = iommu_get(qdev, &dev);
+
     val64 = cpu_to_le64(tally_counters->TxOk);
-    cpu_physical_memory_write(tc_addr + 0,    (uint8_t *)&val64, 8);
+    iommu_write(iommu, dev, tc_addr + 0,     (uint8_t *) &val64, 8);
 
     val64 = cpu_to_le64(tally_counters->RxOk);
-    cpu_physical_memory_write(tc_addr + 8,    (uint8_t *)&val64, 8);
+    iommu_write(iommu, dev, tc_addr + 8,     (uint8_t *) &val64, 8);
 
     val64 = cpu_to_le64(tally_counters->TxERR);
-    cpu_physical_memory_write(tc_addr + 16,    (uint8_t *)&val64, 8);
+    iommu_write(iommu, dev, tc_addr + 16,    (uint8_t *) &val64, 8);
 
     val32 = cpu_to_le32(tally_counters->RxERR);
-    cpu_physical_memory_write(tc_addr + 24,    (uint8_t *)&val32, 4);
+    iommu_write(iommu, dev, tc_addr + 24,    (uint8_t *) &val32, 4);
 
     val16 = cpu_to_le16(tally_counters->MissPkt);
-    cpu_physical_memory_write(tc_addr + 28,    (uint8_t *)&val16, 2);
+    iommu_write(iommu, dev, tc_addr + 28,    (uint8_t *) &val16, 2);
 
     val16 = cpu_to_le16(tally_counters->FAE);
-    cpu_physical_memory_write(tc_addr + 30,    (uint8_t *)&val16, 2);
+    iommu_write(iommu, dev, tc_addr + 30,    (uint8_t *) &val16, 2);
 
     val32 = cpu_to_le32(tally_counters->Tx1Col);
-    cpu_physical_memory_write(tc_addr + 32,    (uint8_t *)&val32, 4);
+    iommu_write(iommu, dev, tc_addr + 32,    (uint8_t *) &val32, 4);
 
     val32 = cpu_to_le32(tally_counters->TxMCol);
-    cpu_physical_memory_write(tc_addr + 36,    (uint8_t *)&val32, 4);
+    iommu_write(iommu, dev, tc_addr + 36,    (uint8_t *) &val32, 4);
 
     val64 = cpu_to_le64(tally_counters->RxOkPhy);
-    cpu_physical_memory_write(tc_addr + 40,    (uint8_t *)&val64, 8);
+    iommu_write(iommu, dev, tc_addr + 40,    (uint8_t *) &val64, 8);
 
     val64 = cpu_to_le64(tally_counters->RxOkBrd);
-    cpu_physical_memory_write(tc_addr + 48,    (uint8_t *)&val64, 8);
+    iommu_write(iommu, dev, tc_addr + 48,    (uint8_t *) &val64, 8);
 
     val32 = cpu_to_le32(tally_counters->RxOkMul);
-    cpu_physical_memory_write(tc_addr + 56,    (uint8_t *)&val32, 4);
+    iommu_write(iommu, dev, tc_addr + 56,    (uint8_t *) &val32, 4);
 
     val16 = cpu_to_le16(tally_counters->TxAbt);
-    cpu_physical_memory_write(tc_addr + 60,    (uint8_t *)&val16, 2);
+    iommu_write(iommu, dev, tc_addr + 60,    (uint8_t *) &val16, 2);
 
     val16 = cpu_to_le16(tally_counters->TxUndrn);
-    cpu_physical_memory_write(tc_addr + 62,    (uint8_t *)&val16, 2);
+    iommu_write(iommu, dev, tc_addr + 62,    (uint8_t *) &val16, 2);
 }
 
 /* Loads values of tally counters from VM state file */
@@ -1758,6 +1773,11 @@ static void rtl8139_transfer_frame(RTL8139State *s, const uint8_t *buf, int size
 
 static int rtl8139_transmit_one(RTL8139State *s, int descriptor)
 {
+    struct iommu *iommu;
+    DeviceState *dev;
+
+    iommu = iommu_get(&s->dev.qdev, &dev);
+
     if (!rtl8139_transmitter_enabled(s))
     {
         DEBUG_PRINT(("RTL8139: +++ cannot transmit from descriptor %d: transmitter disabled\n",
@@ -1780,7 +1800,7 @@ static int rtl8139_transmit_one(RTL8139State *s, int descriptor)
     DEBUG_PRINT(("RTL8139: +++ transmit reading %d bytes from host memory at 0x%08x\n",
                  txsize, s->TxAddr[descriptor]));
 
-    cpu_physical_memory_read(s->TxAddr[descriptor], txbuffer, txsize);
+    iommu_read(iommu, dev, s->TxAddr[descriptor], txbuffer, txsize);
 
     /* Mark descriptor as transferred */
     s->TxStatus[descriptor] |= TxHostOwns;
@@ -1886,6 +1906,11 @@ static uint16_t ip_checksum(void *data, size_t len)
 
 static int rtl8139_cplus_transmit_one(RTL8139State *s)
 {
+    struct iommu *iommu;
+    DeviceState *dev;
+
+    iommu = iommu_get(&s->dev.qdev, &dev);
+
     if (!rtl8139_transmitter_enabled(s))
     {
         DEBUG_PRINT(("RTL8139: +++ C+ mode: transmitter disabled\n"));
@@ -1911,14 +1936,14 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
 
     uint32_t val, txdw0,txdw1,txbufLO,txbufHI;
 
-    cpu_physical_memory_read(cplus_tx_ring_desc,    (uint8_t *)&val, 4);
+    iommu_read(iommu, dev, cplus_tx_ring_desc,    (uint8_t *) &val, 4);
     txdw0 = le32_to_cpu(val);
     /* TODO: implement VLAN tagging support, VLAN tag data is read to txdw1 */
-    cpu_physical_memory_read(cplus_tx_ring_desc+4,  (uint8_t *)&val, 4);
+    iommu_read(iommu, dev, cplus_tx_ring_desc+4,  (uint8_t *) &val, 4);
     txdw1 = le32_to_cpu(val);
-    cpu_physical_memory_read(cplus_tx_ring_desc+8,  (uint8_t *)&val, 4);
+    iommu_read(iommu, dev, cplus_tx_ring_desc+8,  (uint8_t *) &val, 4);
     txbufLO = le32_to_cpu(val);
-    cpu_physical_memory_read(cplus_tx_ring_desc+12, (uint8_t *)&val, 4);
+    iommu_read(iommu, dev, cplus_tx_ring_desc+12, (uint8_t *) &val, 4);
     txbufHI = le32_to_cpu(val);
 
     DEBUG_PRINT(("RTL8139: +++ C+ mode TX descriptor %d %08x %08x %08x %08x\n",
@@ -2025,7 +2050,8 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
     DEBUG_PRINT(("RTL8139: +++ C+ mode transmit reading %d bytes from host memory at %016" PRIx64 " to offset %d\n",
                  txsize, (uint64_t)tx_addr, s->cplus_txbuffer_offset));
 
-    cpu_physical_memory_read(tx_addr, s->cplus_txbuffer + s->cplus_txbuffer_offset, txsize);
+    iommu_read(iommu, dev, tx_addr,
+               s->cplus_txbuffer + s->cplus_txbuffer_offset, txsize);
     s->cplus_txbuffer_offset += txsize;
 
     /* seek to next Rx descriptor */
@@ -2052,7 +2078,7 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
 
     /* update ring data */
     val = cpu_to_le32(txdw0);
-    cpu_physical_memory_write(cplus_tx_ring_desc,    (uint8_t *)&val, 4);
+    iommu_write(iommu, dev, cplus_tx_ring_desc, (uint8_t *) &val, 4);
     /* TODO: implement VLAN tagging support, VLAN tag data is read to txdw1 */
 //    val = cpu_to_le32(txdw1);
 //    cpu_physical_memory_write(cplus_tx_ring_desc+4,  &val, 4);
@@ -2381,7 +2407,7 @@ static void rtl8139_TxStatus_write(RTL8139State *s, uint32_t txRegOffset, uint32
             target_phys_addr_t tc_addr = rtl8139_addr64(s->TxStatus[0] & ~0x3f, s->TxStatus[1]);
 
             /* dump tally counters to specified memory location */
-            RTL8139TallyCounters_physical_memory_write( tc_addr, &s->tally_counters);
+            RTL8139TallyCounters_physical_memory_write(&s->dev.qdev, tc_addr, &s->tally_counters);
 
             /* mark dump completed */
             s->TxStatus[0] &= ~0x8;
Signature · 1 line
1.7.1
#39 lore raw

RFC PATCH 6/7 eepro100: IOMMU support

In reply to Eduard - Gabriel Munteanu

Memory accesses must go through the IOMMU layer.

---
 hw/eepro100.c |  141 +++++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 101 insertions(+), 40 deletions(-)
hw/eepro100.c +101 -40
index 97afa2c..74e1d15 100644
@@ -43,6 +43,7 @@
 
 #include <stddef.h>             /* offsetof */
 #include "hw.h"
+#include "iommu.h"
 #include "pci.h"
 #include "net.h"
 #include "eeprom93xx.h"
@@ -306,10 +307,13 @@ static const uint16_t eepro100_mdi_mask[] = {
 };
 
 /* XXX: optimize */
-static void stl_le_phys(target_phys_addr_t addr, uint32_t val)
+static void stl_le_phys(struct iommu *iommu,
+                        DeviceState *dev,
+                        target_phys_addr_t addr,
+                        uint32_t val)
 {
     val = cpu_to_le32(val);
-    cpu_physical_memory_write(addr, (const uint8_t *)&val, sizeof(val));
+    iommu_write(iommu, dev, addr, (const uint8_t *)&val, sizeof(val));
 }
 
 #define POLYNOMIAL 0x04c11db6
@@ -687,17 +691,25 @@ static void set_ru_state(EEPRO100State * s, ru_state_t state)
 
 static void dump_statistics(EEPRO100State * s)
 {
+    struct iommu *iommu;
+    DeviceState *dev;
+    int err;
+    
+    iommu = iommu_get(&s->dev.qdev, &dev);
+
     /* Dump statistical data. Most data is never changed by the emulation
      * and always 0, so we first just copy the whole block and then those
      * values which really matter.
      * Number of data should check configuration!!!
      */
-    cpu_physical_memory_write(s->statsaddr,
-                              (uint8_t *) & s->statistics, s->stats_size);
-    stl_le_phys(s->statsaddr + 0, s->statistics.tx_good_frames);
-    stl_le_phys(s->statsaddr + 36, s->statistics.rx_good_frames);
-    stl_le_phys(s->statsaddr + 48, s->statistics.rx_resource_errors);
-    stl_le_phys(s->statsaddr + 60, s->statistics.rx_short_frame_errors);
+    err = iommu_write(iommu, dev, s->statsaddr,
+                      (uint8_t *) &s->statistics, s->stats_size);
+    stl_le_phys(iommu, dev, s->statsaddr + 0, s->statistics.tx_good_frames);
+    stl_le_phys(iommu, dev, s->statsaddr + 36, s->statistics.rx_good_frames);
+    stl_le_phys(iommu, dev,
+                s->statsaddr + 48, s->statistics.rx_resource_errors);
+    stl_le_phys(iommu, dev,
+                s->statsaddr + 60, s->statistics.rx_short_frame_errors);
 #if 0
     stw_le_phys(s->statsaddr + 76, s->statistics.xmt_tco_frames);
     stw_le_phys(s->statsaddr + 78, s->statistics.rcv_tco_frames);
@@ -707,7 +719,13 @@ static void dump_statistics(EEPRO100State * s)
 
 static void read_cb(EEPRO100State *s)
 {
-    cpu_physical_memory_read(s->cb_address, (uint8_t *) &s->tx, sizeof(s->tx));
+    struct iommu *iommu;
+    DeviceState *dev;
+    int err;
+
+    iommu = iommu_get(&s->dev.qdev, &dev);
+
+    err = iommu_read(iommu, dev, s->cb_address, (uint8_t *) &s->tx, sizeof(s->tx));
     s->tx.status = le16_to_cpu(s->tx.status);
     s->tx.command = le16_to_cpu(s->tx.command);
     s->tx.link = le32_to_cpu(s->tx.link);
@@ -723,6 +741,12 @@ static void tx_command(EEPRO100State *s)
     uint8_t buf[2600];
     uint16_t size = 0;
     uint32_t tbd_address = s->cb_address + 0x10;
+    struct iommu *iommu;
+    DeviceState *dev;
+    int err;
+
+    iommu = iommu_get(&s->dev.qdev, &dev);
+
     TRACE(RXTX, logout
         ("transmit, TBD array address 0x%08x, TCB byte count 0x%04x, TBD count %u\n",
          tbd_array, tcb_bytes, s->tx.tbd_count));
@@ -737,18 +761,18 @@ static void tx_command(EEPRO100State *s)
     }
     assert(tcb_bytes <= sizeof(buf));
     while (size < tcb_bytes) {
-        uint32_t tx_buffer_address = ldl_phys(tbd_address);
-        uint16_t tx_buffer_size = lduw_phys(tbd_address + 4);
+        uint32_t tx_buffer_address = iommu_ldl(iommu, dev, tbd_address);
+        uint16_t tx_buffer_size = iommu_lduw(iommu, dev, tbd_address + 4);
 #if 0
-        uint16_t tx_buffer_el = lduw_phys(tbd_address + 6);
+        uint16_t tx_buffer_el = iommu_lduw(tbd_address + 6);
 #endif
         tbd_address += 8;
         TRACE(RXTX, logout
             ("TBD (simplified mode): buffer address 0x%08x, size 0x%04x\n",
              tx_buffer_address, tx_buffer_size));
         tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
-        cpu_physical_memory_read(tx_buffer_address, &buf[size],
-                                 tx_buffer_size);
+        err = iommu_read(iommu, dev,
+                         tx_buffer_address, &buf[size], tx_buffer_size);
         size += tx_buffer_size;
     }
     if (tbd_array == 0xffffffff) {
@@ -759,16 +783,18 @@ static void tx_command(EEPRO100State *s)
         if (s->has_extended_tcb_support && !(s->configuration[6] & BIT(4))) {
             /* Extended Flexible TCB. */
             for (; tbd_count < 2; tbd_count++) {
-                uint32_t tx_buffer_address = ldl_phys(tbd_address);
-                uint16_t tx_buffer_size = lduw_phys(tbd_address + 4);
-                uint16_t tx_buffer_el = lduw_phys(tbd_address + 6);
+                uint32_t tx_buffer_address = iommu_ldl(iommu, dev,
+                                                       tbd_address);
+                uint16_t tx_buffer_size = iommu_lduw(iommu, dev,
+                                                     tbd_address + 4);
+                uint16_t tx_buffer_el = iommu_lduw(iommu, dev, tbd_address + 6);
                 tbd_address += 8;
                 TRACE(RXTX, logout
                     ("TBD (extended flexible mode): buffer address 0x%08x, size 0x%04x\n",
                      tx_buffer_address, tx_buffer_size));
                 tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
-                cpu_physical_memory_read(tx_buffer_address, &buf[size],
-                                         tx_buffer_size);
+                err = iommu_read(iommu, dev, tx_buffer_address,
+                                 &buf[size], tx_buffer_size);
                 size += tx_buffer_size;
                 if (tx_buffer_el & 1) {
                     break;
@@ -777,16 +803,16 @@ static void tx_command(EEPRO100State *s)
         }
         tbd_address = tbd_array;
         for (; tbd_count < s->tx.tbd_count; tbd_count++) {
-            uint32_t tx_buffer_address = ldl_phys(tbd_address);
-            uint16_t tx_buffer_size = lduw_phys(tbd_address + 4);
-            uint16_t tx_buffer_el = lduw_phys(tbd_address + 6);
+            uint32_t tx_buffer_address = iommu_ldl(iommu, dev, tbd_address);
+            uint16_t tx_buffer_size = iommu_lduw(iommu, dev, tbd_address + 4);
+            uint16_t tx_buffer_el = iommu_lduw(iommu, dev, tbd_address + 6);
             tbd_address += 8;
             TRACE(RXTX, logout
                 ("TBD (flexible mode): buffer address 0x%08x, size 0x%04x\n",
                  tx_buffer_address, tx_buffer_size));
             tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
-            cpu_physical_memory_read(tx_buffer_address, &buf[size],
-                                     tx_buffer_size);
+            err = iommu_read(iommu, dev,
+                             tx_buffer_address, &buf[size], tx_buffer_size);
             size += tx_buffer_size;
             if (tx_buffer_el & 1) {
                 break;
@@ -807,11 +833,17 @@ static void set_multicast_list(EEPRO100State *s)
 {
     uint16_t multicast_count = s->tx.tbd_array_addr & BITS(13, 0);
     uint16_t i;
+    struct iommu *iommu;
+    DeviceState *dev;
+    int err;
+
+    iommu = iommu_get(&s->dev.qdev, &dev);
+
     memset(&s->mult[0], 0, sizeof(s->mult));
     TRACE(OTHER, logout("multicast list, multicast count = %u\n", multicast_count));
     for (i = 0; i < multicast_count; i += 6) {
         uint8_t multicast_addr[6];
-        cpu_physical_memory_read(s->cb_address + 10 + i, multicast_addr, 6);
+        err = iommu_read(iommu, dev, s->cb_address + 10 + i, multicast_addr, 6);
         TRACE(OTHER, logout("multicast entry %s\n", nic_dump(multicast_addr, 6)));
         unsigned mcast_idx = compute_mcast_idx(multicast_addr);
         assert(mcast_idx < 64);
@@ -821,6 +853,11 @@ static void set_multicast_list(EEPRO100State *s)
 
 static void action_command(EEPRO100State *s)
 {
+    struct iommu *iommu;
+    DeviceState *dev;
+
+    iommu = iommu_get(&s->dev.qdev, &dev);
+
     for (;;) {
         bool bit_el;
         bool bit_s;
@@ -845,12 +882,14 @@ static void action_command(EEPRO100State *s)
             /* Do nothing. */
             break;
         case CmdIASetup:
-            cpu_physical_memory_read(s->cb_address + 8, &s->conf.macaddr.a[0], 6);
+            iommu_read(iommu, dev,
+                       s->cb_address + 8, &s->conf.macaddr.a[0], 6);
             TRACE(OTHER, logout("macaddr: %s\n", nic_dump(&s->conf.macaddr.a[0], 6)));
             break;
         case CmdConfigure:
-            cpu_physical_memory_read(s->cb_address + 8, &s->configuration[0],
-                                     sizeof(s->configuration));
+            iommu_read(iommu, dev,
+                       s->cb_address + 8, &s->configuration[0],
+                       sizeof(s->configuration));
             TRACE(OTHER, logout("configuration: %s\n", nic_dump(&s->configuration[0], 16)));
             break;
         case CmdMulticastList:
@@ -880,7 +919,8 @@ static void action_command(EEPRO100State *s)
             break;
         }
         /* Write new status. */
-        stw_phys(s->cb_address, s->tx.status | ok_status | STATUS_C);
+        iommu_stw(iommu, dev,
+                  s->cb_address, s->tx.status | ok_status | STATUS_C);
         if (bit_i) {
             /* CU completed action. */
             eepro100_cx_interrupt(s);
@@ -907,6 +947,11 @@ static void action_command(EEPRO100State *s)
 static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
 {
     cu_state_t cu_state;
+    struct iommu *iommu;
+    DeviceState *dev;
+
+    iommu = iommu_get(&s->dev.qdev, &dev);
+
     switch (val) {
     case CU_NOP:
         /* No operation. */
@@ -947,7 +992,7 @@ static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
         /* Dump statistical counters. */
         TRACE(OTHER, logout("val=0x%02x (dump stats)\n", val));
         dump_statistics(s);
-        stl_le_phys(s->statsaddr + s->stats_size, 0xa005);
+        stl_le_phys(iommu, dev, s->statsaddr + s->stats_size, 0xa005);
         break;
     case CU_CMD_BASE:
         /* Load CU base. */
@@ -958,7 +1003,7 @@ static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
         /* Dump and reset statistical counters. */
         TRACE(OTHER, logout("val=0x%02x (dump stats and reset)\n", val));
         dump_statistics(s);
-        stl_le_phys(s->statsaddr + s->stats_size, 0xa007);
+        stl_le_phys(iommu, dev, s->statsaddr + s->stats_size, 0xa007);
         memset(&s->statistics, 0, sizeof(s->statistics));
         break;
     case CU_SRESUME:
@@ -1252,6 +1297,11 @@ static void eepro100_write_port(EEPRO100State * s, uint32_t val)
     val = le32_to_cpu(val);
     uint32_t address = (val & ~PORT_SELECTION_MASK);
     uint8_t selection = (val & PORT_SELECTION_MASK);
+    struct iommu *iommu;
+    DeviceState *dev;
+
+    iommu = iommu_get(&s->dev.qdev, &dev);
+
     switch (selection) {
     case PORT_SOFTWARE_RESET:
         nic_reset(s);
@@ -1259,10 +1309,12 @@ static void eepro100_write_port(EEPRO100State * s, uint32_t val)
     case PORT_SELFTEST:
         TRACE(OTHER, logout("selftest address=0x%08x\n", address));
         eepro100_selftest_t data;
-        cpu_physical_memory_read(address, (uint8_t *) & data, sizeof(data));
+        iommu_read(iommu, dev,
+                   address, (uint8_t *) &data, sizeof(data));
         data.st_sign = 0xffffffff;
         data.st_result = 0;
-        cpu_physical_memory_write(address, (uint8_t *) & data, sizeof(data));
+        iommu_write(iommu, dev,
+                    address, (uint8_t *) &data, sizeof(data));
         break;
     case PORT_SELECTIVE_RESET:
         TRACE(OTHER, logout("selective reset, selftest address=0x%08x\n", address));
@@ -1646,6 +1698,10 @@ static ssize_t nic_receive(VLANClientState *nc, const uint8_t * buf, size_t size
     uint16_t rfd_status = 0xa000;
     static const uint8_t broadcast_macaddr[6] =
         { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
+    struct iommu *iommu;
+    DeviceState *dev;
+
+    iommu = iommu_get(&s->dev.qdev, &dev);
 
     /* TODO: check multiple IA bit. */
     if (s->configuration[20] & BIT(6)) {
@@ -1721,8 +1777,9 @@ static ssize_t nic_receive(VLANClientState *nc, const uint8_t * buf, size_t size
     }
     /* !!! */
     eepro100_rx_t rx;
-    cpu_physical_memory_read(s->ru_base + s->ru_offset, (uint8_t *) & rx,
-                             offsetof(eepro100_rx_t, packet));
+    iommu_read(iommu, dev,
+               s->ru_base + s->ru_offset, (uint8_t *) & rx,
+               offsetof(eepro100_rx_t, packet));
     uint16_t rfd_command = le16_to_cpu(rx.command);
     uint16_t rfd_size = le16_to_cpu(rx.size);
 
@@ -1736,9 +1793,12 @@ static ssize_t nic_receive(VLANClientState *nc, const uint8_t * buf, size_t size
     }
     TRACE(OTHER, logout("command 0x%04x, link 0x%08x, addr 0x%08x, size %u\n",
           rfd_command, rx.link, rx.rx_buf_addr, rfd_size));
-    stw_phys(s->ru_base + s->ru_offset + offsetof(eepro100_rx_t, status),
-             rfd_status);
-    stw_phys(s->ru_base + s->ru_offset + offsetof(eepro100_rx_t, count), size);
+    iommu_stw(iommu, dev,
+              s->ru_base + s->ru_offset + offsetof(eepro100_rx_t, status),
+              rfd_status);
+    iommu_stw(iommu, dev,
+              s->ru_base + s->ru_offset + offsetof(eepro100_rx_t, count),
+              size);
     /* Early receive interrupt not supported. */
 #if 0
     eepro100_er_interrupt(s);
@@ -1752,8 +1812,9 @@ static ssize_t nic_receive(VLANClientState *nc, const uint8_t * buf, size_t size
 #if 0
     assert(!(s->configuration[17] & BIT(0)));
 #endif
-    cpu_physical_memory_write(s->ru_base + s->ru_offset +
-                              offsetof(eepro100_rx_t, packet), buf, size);
+    iommu_write(iommu, dev,
+                s->ru_base + s->ru_offset +
+                offsetof(eepro100_rx_t, packet), buf, size);
     s->statistics.rx_good_frames++;
     eepro100_fr_interrupt(s);
     s->ru_offset = le32_to_cpu(rx.link);
Signature · 1 line
1.7.1
#40 lore raw

RFC PATCH 7/7 ac97: IOMMU support

In reply to Eduard - Gabriel Munteanu

Memory accesses must go through the IOMMU layer.

---
 hw/ac97.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)
hw/ac97.c +17 -3
index 4319bc8..0e30d80 100644
@@ -15,6 +15,7 @@
  */
 
 #include "hw.h"
+#include "iommu.h"
 #include "audiodev.h"
 #include "audio/audio.h"
 #include "pci.h"
@@ -221,9 +222,13 @@ static void cold_reset (AC97LinkState * s)
 
 static void fetch_bd (AC97LinkState *s, AC97BusMasterRegs *r)
 {
+    struct iommu *iommu;
+    DeviceState *dev;
     uint8_t b[8];
 
-    cpu_physical_memory_read (r->bdbar + r->civ * 8, b, 8);
+    iommu = iommu_get(&s->dev.qdev, &dev);
+
+    iommu_read (iommu, dev, r->bdbar + r->civ * 8, b, 8);
     r->bd_valid = 1;
     r->bd.addr = le32_to_cpu (*(uint32_t *) &b[0]) & ~3;
     r->bd.ctl_len = le32_to_cpu (*(uint32_t *) &b[4]);
@@ -962,6 +967,9 @@ static int write_audio (AC97LinkState *s, AC97BusMasterRegs *r,
     uint32_t temp = r->picb << 1;
     uint32_t written = 0;
     int to_copy = 0;
+    struct iommu *iommu;
+    DeviceState *dev;
+
     temp = audio_MIN (temp, max);
 
     if (!temp) {
@@ -969,10 +977,12 @@ static int write_audio (AC97LinkState *s, AC97BusMasterRegs *r,
         return 0;
     }
 
+    iommu = iommu_get(&s->dev.qdev, &dev);
+
     while (temp) {
         int copied;
         to_copy = audio_MIN (temp, sizeof (tmpbuf));
-        cpu_physical_memory_read (addr, tmpbuf, to_copy);
+        iommu_read (iommu, dev, addr, tmpbuf, to_copy);
         copied = AUD_write (s->voice_po, tmpbuf, to_copy);
         dolog ("write_audio max=%x to_copy=%x copied=%x\n",
                max, to_copy, copied);
@@ -1040,6 +1050,10 @@ static int read_audio (AC97LinkState *s, AC97BusMasterRegs *r,
     uint32_t nread = 0;
     int to_copy = 0;
     SWVoiceIn *voice = (r - s->bm_regs) == MC_INDEX ? s->voice_mc : s->voice_pi;
+    struct iommu *iommu;
+    DeviceState *dev;
+
+    iommu = iommu_get(&s->dev.qdev, &dev);
 
     temp = audio_MIN (temp, max);
 
@@ -1056,7 +1070,7 @@ static int read_audio (AC97LinkState *s, AC97BusMasterRegs *r,
             *stop = 1;
             break;
         }
-        cpu_physical_memory_write (addr, tmpbuf, acquired);
+        iommu_write (iommu, dev, addr, tmpbuf, acquired);
         temp -= acquired;
         addr += acquired;
         nread += acquired;
Signature · 1 line
1.7.1