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.au...@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 > -- Peter Xu