Patchwork [v2,4/7] memory: provide a MemoryRegion for IOMMUs to log faults

login
register
mail settings
Submitter Avi Kivity
Date Oct. 30, 2012, 11:47 a.m.
Message ID <1351597670-23031-5-git-send-email-avi@redhat.com>
Download mbox | patch
Permalink /patch/195466/
State New
Headers show

Comments

Avi Kivity - Oct. 30, 2012, 11:47 a.m.
Accesses which do not translate will hit the fault region, which
can then log the access.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 memory.c | 9 ++++++---
 memory.h | 4 ++++
 2 files changed, 10 insertions(+), 3 deletions(-)
Blue Swirl - Oct. 30, 2012, 7:14 p.m.
On Tue, Oct 30, 2012 at 11:47 AM, Avi Kivity <avi@redhat.com> wrote:
> Accesses which do not translate will hit the fault region, which
> can then log the access.

Maybe special casing the fault region should be avoided, the IOMMU
could set up suitable fault regions by itself.

>
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  memory.c | 9 ++++++---
>  memory.h | 4 ++++
>  2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/memory.c b/memory.c
> index ba2d4a0..d6b46fd 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -778,7 +778,9 @@ static void memory_region_destructor_rom_device(MemoryRegion *mr)
>  static void memory_region_destructor_iommu(MemoryRegion *mr)
>  {
>      address_space_destroy(mr->iommu_target_as);
> +    address_space_destroy(mr->iommu_fault_as);
>      g_free(mr->iommu_target_as);
> +    g_free(mr->iommu_fault_as);
>  }
>
>  static bool memory_region_wrong_endianness(MemoryRegion *mr)
> @@ -1001,9 +1003,7 @@ static void memory_region_iommu_rw(MemoryRegion *iommu, hwaddr addr,
>              xlat = (tlb.translated_addr & ~tlb.addr_mask) | (addr & tlb.addr_mask);
>              address_space_rw(iommu->iommu_target_as, xlat, buf, clen, is_write);
>          } else {
> -            if (!is_write) {
> -                memset(buf, 0xff, clen);
> -            }
> +            address_space_rw(iommu->iommu_fault_as, addr, buf, clen, is_write);
>          }
>          buf += clen;
>          addr += clen;
> @@ -1068,6 +1068,7 @@ static void memory_region_iommu_write(void *opaque, hwaddr addr,
>  void memory_region_init_iommu(MemoryRegion *mr,
>                                MemoryRegionIOMMUOps *ops,
>                                MemoryRegion *target,
> +                              MemoryRegion *fault,
>                                const char *name,
>                                uint64_t size)
>  {
> @@ -1079,6 +1080,8 @@ void memory_region_init_iommu(MemoryRegion *mr,
>      mr->destructor = memory_region_destructor_iommu;
>      mr->iommu_target_as = g_new(AddressSpace, 1);
>      address_space_init(mr->iommu_target_as, target);
> +    mr->iommu_fault_as = g_new(AddressSpace, 1);
> +    address_space_init(mr->iommu_fault_as, fault);
>  }
>
>  static uint64_t invalid_read(void *opaque, hwaddr addr,
> diff --git a/memory.h b/memory.h
> index 47362c9..6312197 100644
> --- a/memory.h
> +++ b/memory.h
> @@ -162,6 +162,7 @@ struct MemoryRegion {
>      unsigned ioeventfd_nb;
>      MemoryRegionIoeventfd *ioeventfds;
>      struct AddressSpace *iommu_target_as;
> +    struct AddressSpace *iommu_fault_as;
>  };
>
>  struct MemoryRegionPortio {
> @@ -361,12 +362,15 @@ void memory_region_init_reservation(MemoryRegion *mr,
>   * @ops: a function that translates addresses into the @target region
>   * @target: a #MemoryRegion that will be used to satisfy accesses to translated
>   *          addresses
> + * @fault: a #MemoryRegion for servicing failed translations; accessed with
> + *         untranslated addresses
>   * @name: used for debugging; not visible to the user or ABI
>   * @size: size of the region.
>   */
>  void memory_region_init_iommu(MemoryRegion *mr,
>                                MemoryRegionIOMMUOps *ops,
>                                MemoryRegion *target,
> +                              MemoryRegion *fault,
>                                const char *name,
>                                uint64_t size);
>
> --
> 1.7.12
>
Avi Kivity - Oct. 31, 2012, 10:33 a.m.
On 10/30/2012 09:14 PM, Blue Swirl wrote:
> On Tue, Oct 30, 2012 at 11:47 AM, Avi Kivity <avi@redhat.com> wrote:
>> Accesses which do not translate will hit the fault region, which
>> can then log the access.
> 
> Maybe special casing the fault region should be avoided, the IOMMU
> could set up suitable fault regions by itself.

How can it?  Please elaborate.

I'm not too pleased with using a MemoryRegion for faults, perhaps a
MemoryRegionIOMMUOps::fault() callback would be better.

Patch

diff --git a/memory.c b/memory.c
index ba2d4a0..d6b46fd 100644
--- a/memory.c
+++ b/memory.c
@@ -778,7 +778,9 @@  static void memory_region_destructor_rom_device(MemoryRegion *mr)
 static void memory_region_destructor_iommu(MemoryRegion *mr)
 {
     address_space_destroy(mr->iommu_target_as);
+    address_space_destroy(mr->iommu_fault_as);
     g_free(mr->iommu_target_as);
+    g_free(mr->iommu_fault_as);
 }
 
 static bool memory_region_wrong_endianness(MemoryRegion *mr)
@@ -1001,9 +1003,7 @@  static void memory_region_iommu_rw(MemoryRegion *iommu, hwaddr addr,
             xlat = (tlb.translated_addr & ~tlb.addr_mask) | (addr & tlb.addr_mask);
             address_space_rw(iommu->iommu_target_as, xlat, buf, clen, is_write);
         } else {
-            if (!is_write) {
-                memset(buf, 0xff, clen);
-            }
+            address_space_rw(iommu->iommu_fault_as, addr, buf, clen, is_write);
         }
         buf += clen;
         addr += clen;
@@ -1068,6 +1068,7 @@  static void memory_region_iommu_write(void *opaque, hwaddr addr,
 void memory_region_init_iommu(MemoryRegion *mr,
                               MemoryRegionIOMMUOps *ops,
                               MemoryRegion *target,
+                              MemoryRegion *fault,
                               const char *name,
                               uint64_t size)
 {
@@ -1079,6 +1080,8 @@  void memory_region_init_iommu(MemoryRegion *mr,
     mr->destructor = memory_region_destructor_iommu;
     mr->iommu_target_as = g_new(AddressSpace, 1);
     address_space_init(mr->iommu_target_as, target);
+    mr->iommu_fault_as = g_new(AddressSpace, 1);
+    address_space_init(mr->iommu_fault_as, fault);
 }
 
 static uint64_t invalid_read(void *opaque, hwaddr addr,
diff --git a/memory.h b/memory.h
index 47362c9..6312197 100644
--- a/memory.h
+++ b/memory.h
@@ -162,6 +162,7 @@  struct MemoryRegion {
     unsigned ioeventfd_nb;
     MemoryRegionIoeventfd *ioeventfds;
     struct AddressSpace *iommu_target_as;
+    struct AddressSpace *iommu_fault_as;
 };
 
 struct MemoryRegionPortio {
@@ -361,12 +362,15 @@  void memory_region_init_reservation(MemoryRegion *mr,
  * @ops: a function that translates addresses into the @target region
  * @target: a #MemoryRegion that will be used to satisfy accesses to translated
  *          addresses
+ * @fault: a #MemoryRegion for servicing failed translations; accessed with
+ *         untranslated addresses
  * @name: used for debugging; not visible to the user or ABI
  * @size: size of the region.
  */
 void memory_region_init_iommu(MemoryRegion *mr,
                               MemoryRegionIOMMUOps *ops,
                               MemoryRegion *target,
+                              MemoryRegion *fault,
                               const char *name,
                               uint64_t size);