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

[Peter Xu]

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 <address@hidden>
> 
> ---
> 
> 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
> 

-- 
Peter Xu