diff mbox series

[v12,09/13] virtio-iommu: Implement fault reporting

Message ID 20200109144319.15912-10-eric.auger@redhat.com
State New
Headers show
Series VIRTIO-IOMMU device | expand

Commit Message

Eric Auger Jan. 9, 2020, 2:43 p.m. UTC 
The event queue allows to report asynchronous errors.
The translate function now injects faults when relevant.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v11 -> v12:
- reporting the addr associated with the fault and set the
  VIRTIO_IOMMU_FAULT_F_ADDRESS flag.
- added cpu_to_le<n>

v10 -> v11:
- change a virtio_error into an error_report_once
  (no buffer available for output faults)
---
 hw/virtio/trace-events   |  1 +
 hw/virtio/virtio-iommu.c | 73 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 69 insertions(+), 5 deletions(-)

Comments

Peter Xu Jan. 14, 2020, 7:04 p.m. UTC | #1 
On Thu, Jan 09, 2020 at 03:43:15PM +0100, Eric Auger wrote:
> The event queue allows to report asynchronous errors.
> The translate function now injects faults when relevant.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v11 -> v12:
> - reporting the addr associated with the fault and set the
>   VIRTIO_IOMMU_FAULT_F_ADDRESS flag.
> - added cpu_to_le<n>
> 
> v10 -> v11:
> - change a virtio_error into an error_report_once
>   (no buffer available for output faults)
> ---
>  hw/virtio/trace-events   |  1 +
>  hw/virtio/virtio-iommu.c | 73 +++++++++++++++++++++++++++++++++++++---
>  2 files changed, 69 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index 095aa8b509..e83500bee9 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -72,3 +72,4 @@ virtio_iommu_put_endpoint(uint32_t ep_id) "Free endpoint=%d"
>  virtio_iommu_get_domain(uint32_t domain_id) "Alloc domain=%d"
>  virtio_iommu_put_domain(uint32_t domain_id) "Free domain=%d"
>  virtio_iommu_translate_out(uint64_t virt_addr, uint64_t phys_addr, uint32_t sid) "0x%"PRIx64" -> 0x%"PRIx64 " for sid=%d"
> +virtio_iommu_report_fault(uint8_t reason, uint32_t flags, uint32_t endpoint, uint64_t addr) "FAULT reason=%d flags=%d endpoint=%d address =0x%"PRIx64
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index d192bcb505..09193970ee 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -477,6 +477,51 @@ out:
>      }
>  }
>  
> +static void virtio_iommu_report_fault(VirtIOIOMMU *viommu, uint8_t reason,
> +                                      int flags, uint32_t endpoint,
> +                                      uint64_t address)
> +{
> +    VirtIODevice *vdev = &viommu->parent_obj;
> +    VirtQueue *vq = viommu->event_vq;
> +    struct virtio_iommu_fault fault;
> +    VirtQueueElement *elem;
> +    size_t sz;
> +
> +    memset(&fault, 0, sizeof(fault));
> +    fault.reason = reason;
> +    fault.flags = cpu_to_le32(flags);
> +    fault.endpoint = cpu_to_le32(endpoint);
> +    fault.address = cpu_to_le64(address);
> +
> +    for (;;) {
> +        elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> +
> +        if (!elem) {
> +            error_report_once(
> +                "no buffer available in event queue to report event");

(Should this also be a guest issue?  IIRC you are still using a
 mixture of both qemu_log_mask and error_report()... I'll stop
 commenting on this, assuming that you prefer both ways to be used...)

> +            return;
> +        }
> +
> +        if (iov_size(elem->in_sg, elem->in_num) < sizeof(fault)) {
> +            virtio_error(vdev, "error buffer of wrong size");
> +            virtqueue_detach_element(vq, elem, 0);
> +            g_free(elem);
> +            continue;

If virtio_error(), should we stop rather than continue?

> +        }
> +        break;
> +    }
> +    /* we have a buffer to fill in */
> +    sz = iov_from_buf(elem->in_sg, elem->in_num, 0,
> +                      &fault, sizeof(fault));
> +    assert(sz == sizeof(fault));
> +
> +    trace_virtio_iommu_report_fault(reason, flags, endpoint, address);
> +    virtqueue_push(vq, elem, sz);
> +    virtio_notify(vdev, vq);
> +    g_free(elem);
> +
> +}
> +
>  static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>                                              IOMMUAccessFlags flag,
>                                              int iommu_idx)
> @@ -485,9 +530,10 @@ static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>      VirtIOIOMMUInterval interval, *mapping_key;
>      VirtIOIOMMUMapping *mapping_value;
>      VirtIOIOMMU *s = sdev->viommu;
> +    bool read_fault, write_fault;
>      VirtIOIOMMUEndpoint *ep;
> +    uint32_t sid, flags;
>      bool bypass_allowed;
> -    uint32_t sid;
>      bool found;
>  
>      interval.low = addr;
> @@ -513,6 +559,9 @@ static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>      if (!ep) {
>          if (!bypass_allowed) {
>              error_report_once("%s sid=%d is not known!!", __func__, sid);
> +            virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_UNKNOWN,
> +                                      VIRTIO_IOMMU_FAULT_F_ADDRESS,
> +                                      sid, addr);
>          } else {
>              entry.perm = flag;
>          }
> @@ -524,6 +573,9 @@ static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>              error_report_once("%s %02x:%02x.%01x not attached to any domain",
>                                __func__, PCI_BUS_NUM(sid),
>                                PCI_SLOT(sid), PCI_FUNC(sid));
> +            virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_DOMAIN,
> +                                      VIRTIO_IOMMU_FAULT_F_ADDRESS,
> +                                      sid, addr);
>          } else {
>              entry.perm = flag;
>          }
> @@ -536,15 +588,26 @@ static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>      if (!found) {
>          error_report_once("%s no mapping for 0x%"PRIx64" for sid=%d",
>                            __func__, addr, sid);
> +        virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_MAPPING,
> +                                  VIRTIO_IOMMU_FAULT_F_ADDRESS,
> +                                  sid, addr);
>          goto unlock;
>      }
>  
> -    if (((flag & IOMMU_RO) &&
> -            !(mapping_value->flags & VIRTIO_IOMMU_MAP_F_READ)) ||
> -        ((flag & IOMMU_WO) &&
> -            !(mapping_value->flags & VIRTIO_IOMMU_MAP_F_WRITE))) {
> +    read_fault = (flag & IOMMU_RO) &&
> +                    !(mapping_value->flags & VIRTIO_IOMMU_MAP_F_READ);
> +    write_fault = (flag & IOMMU_WO) &&
> +                    !(mapping_value->flags & VIRTIO_IOMMU_MAP_F_WRITE);
> +
> +    flags = read_fault ? VIRTIO_IOMMU_FAULT_F_READ : 0;
> +    flags |= write_fault ? VIRTIO_IOMMU_FAULT_F_WRITE : 0;
> +    if (flags) {
>          error_report_once("%s permission error on 0x%"PRIx64"(%d): allowed=%d",
>                            __func__, addr, flag, mapping_value->flags);
> +        flags |= VIRTIO_IOMMU_FAULT_F_ADDRESS;
> +        virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_MAPPING,
> +                                  flags | VIRTIO_IOMMU_FAULT_F_ADDRESS,
> +                                  sid, addr);
>          goto unlock;
>      }
>      entry.translated_addr = addr - mapping_key->low + mapping_value->phys_addr;
> -- 
> 2.20.1
>
Eric Auger Jan. 15, 2020, 1:12 p.m. UTC | #2 
Hi Peter,

On 1/14/20 8:04 PM, Peter Xu wrote:
> On Thu, Jan 09, 2020 at 03:43:15PM +0100, Eric Auger wrote:
>> The event queue allows to report asynchronous errors.
>> The translate function now injects faults when relevant.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> v11 -> v12:
>> - reporting the addr associated with the fault and set the
>>   VIRTIO_IOMMU_FAULT_F_ADDRESS flag.
>> - added cpu_to_le<n>
>>
>> v10 -> v11:
>> - change a virtio_error into an error_report_once
>>   (no buffer available for output faults)
>> ---
>>  hw/virtio/trace-events   |  1 +
>>  hw/virtio/virtio-iommu.c | 73 +++++++++++++++++++++++++++++++++++++---
>>  2 files changed, 69 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
>> index 095aa8b509..e83500bee9 100644
>> --- a/hw/virtio/trace-events
>> +++ b/hw/virtio/trace-events
>> @@ -72,3 +72,4 @@ virtio_iommu_put_endpoint(uint32_t ep_id) "Free endpoint=%d"
>>  virtio_iommu_get_domain(uint32_t domain_id) "Alloc domain=%d"
>>  virtio_iommu_put_domain(uint32_t domain_id) "Free domain=%d"
>>  virtio_iommu_translate_out(uint64_t virt_addr, uint64_t phys_addr, uint32_t sid) "0x%"PRIx64" -> 0x%"PRIx64 " for sid=%d"
>> +virtio_iommu_report_fault(uint8_t reason, uint32_t flags, uint32_t endpoint, uint64_t addr) "FAULT reason=%d flags=%d endpoint=%d address =0x%"PRIx64
>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>> index d192bcb505..09193970ee 100644
>> --- a/hw/virtio/virtio-iommu.c
>> +++ b/hw/virtio/virtio-iommu.c
>> @@ -477,6 +477,51 @@ out:
>>      }
>>  }
>>  
>> +static void virtio_iommu_report_fault(VirtIOIOMMU *viommu, uint8_t reason,
>> +                                      int flags, uint32_t endpoint,
>> +                                      uint64_t address)
>> +{
>> +    VirtIODevice *vdev = &viommu->parent_obj;
>> +    VirtQueue *vq = viommu->event_vq;
>> +    struct virtio_iommu_fault fault;
>> +    VirtQueueElement *elem;
>> +    size_t sz;
>> +
>> +    memset(&fault, 0, sizeof(fault));
>> +    fault.reason = reason;
>> +    fault.flags = cpu_to_le32(flags);
>> +    fault.endpoint = cpu_to_le32(endpoint);
>> +    fault.address = cpu_to_le64(address);
>> +
>> +    for (;;) {
>> +        elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
>> +
>> +        if (!elem) {
>> +            error_report_once(
>> +                "no buffer available in event queue to report event");
> 
> (Should this also be a guest issue?  IIRC you are still using a
>  mixture of both qemu_log_mask and error_report()... I'll stop
>  commenting on this, assuming that you prefer both ways to be used...)

I've just removed the qemu_log_mask in virtio_iommu_unmap(). So now you
should not find any qemu_log_mask anymore. Sorry for the oversight.
> 
>> +            return;
>> +        }
>> +
>> +        if (iov_size(elem->in_sg, elem->in_num) < sizeof(fault)) {
>> +            virtio_error(vdev, "error buffer of wrong size");
>> +            virtqueue_detach_element(vq, elem, 0);
>> +            g_free(elem);
>> +            continue;
> 
> If virtio_error(), should we stop rather than continue?
My understanding is the buffer just popped had a wrong size so it is not
usable. We skip it we try to use another one if any. Does it make sense?

Thanks

Eric
> 
>> +        }
>> +        break;
>> +    }
>> +    /* we have a buffer to fill in */
>> +    sz = iov_from_buf(elem->in_sg, elem->in_num, 0,
>> +                      &fault, sizeof(fault));
>> +    assert(sz == sizeof(fault));
>> +
>> +    trace_virtio_iommu_report_fault(reason, flags, endpoint, address);
>> +    virtqueue_push(vq, elem, sz);
>> +    virtio_notify(vdev, vq);
>> +    g_free(elem);
>> +
>> +}
>> +
>>  static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>>                                              IOMMUAccessFlags flag,
>>                                              int iommu_idx)
>> @@ -485,9 +530,10 @@ static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>>      VirtIOIOMMUInterval interval, *mapping_key;
>>      VirtIOIOMMUMapping *mapping_value;
>>      VirtIOIOMMU *s = sdev->viommu;
>> +    bool read_fault, write_fault;
>>      VirtIOIOMMUEndpoint *ep;
>> +    uint32_t sid, flags;
>>      bool bypass_allowed;
>> -    uint32_t sid;
>>      bool found;
>>  
>>      interval.low = addr;
>> @@ -513,6 +559,9 @@ static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>>      if (!ep) {
>>          if (!bypass_allowed) {
>>              error_report_once("%s sid=%d is not known!!", __func__, sid);
>> +            virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_UNKNOWN,
>> +                                      VIRTIO_IOMMU_FAULT_F_ADDRESS,
>> +                                      sid, addr);
>>          } else {
>>              entry.perm = flag;
>>          }
>> @@ -524,6 +573,9 @@ static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>>              error_report_once("%s %02x:%02x.%01x not attached to any domain",
>>                                __func__, PCI_BUS_NUM(sid),
>>                                PCI_SLOT(sid), PCI_FUNC(sid));
>> +            virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_DOMAIN,
>> +                                      VIRTIO_IOMMU_FAULT_F_ADDRESS,
>> +                                      sid, addr);
>>          } else {
>>              entry.perm = flag;
>>          }
>> @@ -536,15 +588,26 @@ static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>>      if (!found) {
>>          error_report_once("%s no mapping for 0x%"PRIx64" for sid=%d",
>>                            __func__, addr, sid);
>> +        virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_MAPPING,
>> +                                  VIRTIO_IOMMU_FAULT_F_ADDRESS,
>> +                                  sid, addr);
>>          goto unlock;
>>      }
>>  
>> -    if (((flag & IOMMU_RO) &&
>> -            !(mapping_value->flags & VIRTIO_IOMMU_MAP_F_READ)) ||
>> -        ((flag & IOMMU_WO) &&
>> -            !(mapping_value->flags & VIRTIO_IOMMU_MAP_F_WRITE))) {
>> +    read_fault = (flag & IOMMU_RO) &&
>> +                    !(mapping_value->flags & VIRTIO_IOMMU_MAP_F_READ);
>> +    write_fault = (flag & IOMMU_WO) &&
>> +                    !(mapping_value->flags & VIRTIO_IOMMU_MAP_F_WRITE);
>> +
>> +    flags = read_fault ? VIRTIO_IOMMU_FAULT_F_READ : 0;
>> +    flags |= write_fault ? VIRTIO_IOMMU_FAULT_F_WRITE : 0;
>> +    if (flags) {
>>          error_report_once("%s permission error on 0x%"PRIx64"(%d): allowed=%d",
>>                            __func__, addr, flag, mapping_value->flags);
>> +        flags |= VIRTIO_IOMMU_FAULT_F_ADDRESS;
>> +        virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_MAPPING,
>> +                                  flags | VIRTIO_IOMMU_FAULT_F_ADDRESS,
>> +                                  sid, addr);
>>          goto unlock;
>>      }
>>      entry.translated_addr = addr - mapping_key->low + mapping_value->phys_addr;
>> -- 
>> 2.20.1
>>
>
Peter Xu Jan. 15, 2020, 3:04 p.m. UTC | #3 
On Wed, Jan 15, 2020 at 02:12:20PM +0100, Auger Eric wrote:
> >> +static void virtio_iommu_report_fault(VirtIOIOMMU *viommu, uint8_t reason,
> >> +                                      int flags, uint32_t endpoint,
> >> +                                      uint64_t address)
> >> +{

[...]

> >> +        if (iov_size(elem->in_sg, elem->in_num) < sizeof(fault)) {
> >> +            virtio_error(vdev, "error buffer of wrong size");
> >> +            virtqueue_detach_element(vq, elem, 0);
> >> +            g_free(elem);
> >> +            continue;
> > 
> > If virtio_error(), should we stop rather than continue?
> My understanding is the buffer just popped had a wrong size so it is not
> usable. We skip it we try to use another one if any. Does it make sense?

I'm not very familiar to virtio, but I see that virtio_error marks
vdev->broken to true.  If with that iiuc the next virtqueue_pop() will
fail directly (see the first call to virtio_device_disabled in
virtqueue_pop).  Then I don't see why retry any more...

Thanks,
Eric Auger Jan. 15, 2020, 4:36 p.m. UTC | #4 
Hi Peter,
On 1/15/20 4:04 PM, Peter Xu wrote:
> On Wed, Jan 15, 2020 at 02:12:20PM +0100, Auger Eric wrote:
>>>> +static void virtio_iommu_report_fault(VirtIOIOMMU *viommu, uint8_t reason,
>>>> +                                      int flags, uint32_t endpoint,
>>>> +                                      uint64_t address)
>>>> +{
> 
> [...]
> 
>>>> +        if (iov_size(elem->in_sg, elem->in_num) < sizeof(fault)) {
>>>> +            virtio_error(vdev, "error buffer of wrong size");
>>>> +            virtqueue_detach_element(vq, elem, 0);
>>>> +            g_free(elem);
>>>> +            continue;
>>>
>>> If virtio_error(), should we stop rather than continue?
>> My understanding is the buffer just popped had a wrong size so it is not
>> usable. We skip it we try to use another one if any. Does it make sense?
> 
> I'm not very familiar to virtio, but I see that virtio_error marks
> vdev->broken to true.  If with that iiuc the next virtqueue_pop() will
> fail directly (see the first call to virtio_device_disabled in
> virtqueue_pop).  Then I don't see why retry any more...
You're right. I will fix it.

Thanks

Eric
> 
> Thanks,
>
diff mbox series

Patch

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 095aa8b509..e83500bee9 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -72,3 +72,4 @@  virtio_iommu_put_endpoint(uint32_t ep_id) "Free endpoint=%d"
 virtio_iommu_get_domain(uint32_t domain_id) "Alloc domain=%d"
 virtio_iommu_put_domain(uint32_t domain_id) "Free domain=%d"
 virtio_iommu_translate_out(uint64_t virt_addr, uint64_t phys_addr, uint32_t sid) "0x%"PRIx64" -> 0x%"PRIx64 " for sid=%d"
+virtio_iommu_report_fault(uint8_t reason, uint32_t flags, uint32_t endpoint, uint64_t addr) "FAULT reason=%d flags=%d endpoint=%d address =0x%"PRIx64
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index d192bcb505..09193970ee 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -477,6 +477,51 @@  out:
     }
 }
 
+static void virtio_iommu_report_fault(VirtIOIOMMU *viommu, uint8_t reason,
+                                      int flags, uint32_t endpoint,
+                                      uint64_t address)
+{
+    VirtIODevice *vdev = &viommu->parent_obj;
+    VirtQueue *vq = viommu->event_vq;
+    struct virtio_iommu_fault fault;
+    VirtQueueElement *elem;
+    size_t sz;
+
+    memset(&fault, 0, sizeof(fault));
+    fault.reason = reason;
+    fault.flags = cpu_to_le32(flags);
+    fault.endpoint = cpu_to_le32(endpoint);
+    fault.address = cpu_to_le64(address);
+
+    for (;;) {
+        elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
+
+        if (!elem) {
+            error_report_once(
+                "no buffer available in event queue to report event");
+            return;
+        }
+
+        if (iov_size(elem->in_sg, elem->in_num) < sizeof(fault)) {
+            virtio_error(vdev, "error buffer of wrong size");
+            virtqueue_detach_element(vq, elem, 0);
+            g_free(elem);
+            continue;
+        }
+        break;
+    }
+    /* we have a buffer to fill in */
+    sz = iov_from_buf(elem->in_sg, elem->in_num, 0,
+                      &fault, sizeof(fault));
+    assert(sz == sizeof(fault));
+
+    trace_virtio_iommu_report_fault(reason, flags, endpoint, address);
+    virtqueue_push(vq, elem, sz);
+    virtio_notify(vdev, vq);
+    g_free(elem);
+
+}
+
 static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
                                             IOMMUAccessFlags flag,
                                             int iommu_idx)
@@ -485,9 +530,10 @@  static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
     VirtIOIOMMUInterval interval, *mapping_key;
     VirtIOIOMMUMapping *mapping_value;
     VirtIOIOMMU *s = sdev->viommu;
+    bool read_fault, write_fault;
     VirtIOIOMMUEndpoint *ep;
+    uint32_t sid, flags;
     bool bypass_allowed;
-    uint32_t sid;
     bool found;
 
     interval.low = addr;
@@ -513,6 +559,9 @@  static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
     if (!ep) {
         if (!bypass_allowed) {
             error_report_once("%s sid=%d is not known!!", __func__, sid);
+            virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_UNKNOWN,
+                                      VIRTIO_IOMMU_FAULT_F_ADDRESS,
+                                      sid, addr);
         } else {
             entry.perm = flag;
         }
@@ -524,6 +573,9 @@  static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
             error_report_once("%s %02x:%02x.%01x not attached to any domain",
                               __func__, PCI_BUS_NUM(sid),
                               PCI_SLOT(sid), PCI_FUNC(sid));
+            virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_DOMAIN,
+                                      VIRTIO_IOMMU_FAULT_F_ADDRESS,
+                                      sid, addr);
         } else {
             entry.perm = flag;
         }
@@ -536,15 +588,26 @@  static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
     if (!found) {
         error_report_once("%s no mapping for 0x%"PRIx64" for sid=%d",
                           __func__, addr, sid);
+        virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_MAPPING,
+                                  VIRTIO_IOMMU_FAULT_F_ADDRESS,
+                                  sid, addr);
         goto unlock;
     }
 
-    if (((flag & IOMMU_RO) &&
-            !(mapping_value->flags & VIRTIO_IOMMU_MAP_F_READ)) ||
-        ((flag & IOMMU_WO) &&
-            !(mapping_value->flags & VIRTIO_IOMMU_MAP_F_WRITE))) {
+    read_fault = (flag & IOMMU_RO) &&
+                    !(mapping_value->flags & VIRTIO_IOMMU_MAP_F_READ);
+    write_fault = (flag & IOMMU_WO) &&
+                    !(mapping_value->flags & VIRTIO_IOMMU_MAP_F_WRITE);
+
+    flags = read_fault ? VIRTIO_IOMMU_FAULT_F_READ : 0;
+    flags |= write_fault ? VIRTIO_IOMMU_FAULT_F_WRITE : 0;
+    if (flags) {
         error_report_once("%s permission error on 0x%"PRIx64"(%d): allowed=%d",
                           __func__, addr, flag, mapping_value->flags);
+        flags |= VIRTIO_IOMMU_FAULT_F_ADDRESS;
+        virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_MAPPING,
+                                  flags | VIRTIO_IOMMU_FAULT_F_ADDRESS,
+                                  sid, addr);
         goto unlock;
     }
     entry.translated_addr = addr - mapping_key->low + mapping_value->phys_addr;