On Fri, Nov 22, 2019 at 07:29:31PM +0100, Eric Auger wrote: > This patch implements the translate callback > > Signed-off-by: Eric Auger <eric.au...@redhat.com> > > --- > > v10 -> v11: > - take into account the new value struct and use > g_tree_lookup_extended > - switched to error_report_once > > v6 -> v7: > - implemented bypass-mode > > v5 -> v6: > - replace error_report by qemu_log_mask > > v4 -> v5: > - check the device domain is not NULL > - s/printf/error_report > - set flags to IOMMU_NONE in case of all translation faults > --- > hw/virtio/trace-events | 1 + > hw/virtio/virtio-iommu.c | 63 +++++++++++++++++++++++++++++++++++++++- > 2 files changed, 63 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events > index f25359cee2..de7cbb3c8f 100644 > --- a/hw/virtio/trace-events > +++ b/hw/virtio/trace-events > @@ -72,3 +72,4 @@ virtio_iommu_get_endpoint(uint32_t ep_id) "Alloc > endpoint=%d" > 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" > diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c > index f0a56833a2..a83666557b 100644 > --- a/hw/virtio/virtio-iommu.c > +++ b/hw/virtio/virtio-iommu.c > @@ -412,19 +412,80 @@ static IOMMUTLBEntry > virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr, > int iommu_idx) > { > IOMMUDevice *sdev = container_of(mr, IOMMUDevice, iommu_mr); > + viommu_interval interval, *mapping_key; > + viommu_mapping *mapping_value; > + VirtIOIOMMU *s = sdev->viommu; > + viommu_endpoint *ep; > + bool bypass_allowed; > uint32_t sid; > + bool found; > + > + interval.low = addr; > + interval.high = addr + 1; > > IOMMUTLBEntry entry = { > .target_as = &address_space_memory, > .iova = addr, > .translated_addr = addr, > - .addr_mask = ~(hwaddr)0, > + .addr_mask = (1 << ctz32(s->config.page_size_mask)) - 1, > .perm = IOMMU_NONE, > }; > > + bypass_allowed = virtio_has_feature(s->acked_features, > + VIRTIO_IOMMU_F_BYPASS); > +
Would it be easier to check bypass_allowed here once and then drop the latter [1] and [2] check? > sid = virtio_iommu_get_sid(sdev); > > trace_virtio_iommu_translate(mr->parent_obj.name, sid, addr, flag); > + qemu_mutex_lock(&s->mutex); > + > + ep = g_tree_lookup(s->endpoints, GUINT_TO_POINTER(sid)); > + if (!ep) { > + if (!bypass_allowed) { [1] > + error_report_once("%s sid=%d is not known!!", __func__, sid); > + } else { > + entry.perm = flag; > + } > + goto unlock; > + } > + > + if (!ep->domain) { > + if (!bypass_allowed) { [2] > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s %02x:%02x.%01x not attached to any domain\n", > + __func__, PCI_BUS_NUM(sid), > + PCI_SLOT(sid), PCI_FUNC(sid)); > + } else { > + entry.perm = flag; > + } > + goto unlock; > + } > + > + found = g_tree_lookup_extended(ep->domain->mappings, > (gpointer)(&interval), > + (void **)&mapping_key, > + (void **)&mapping_value); > + if (!found) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s no mapping for 0x%"PRIx64" for sid=%d\n", > + __func__, addr, sid); I would still suggest that we use the same logging interface (either error_report_once() or qemu_log_mask(), not use them randomly). > + 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))) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "Permission error on 0x%"PRIx64"(%d): allowed=%d\n", > + addr, flag, mapping_value->flags); (Btw, IIUC this may not be a guest error. Say, what if the device is simply broken?) > + goto unlock; > + } > + entry.translated_addr = addr - mapping_key->low + > mapping_value->phys_addr; > + entry.perm = flag; > + trace_virtio_iommu_translate_out(addr, entry.translated_addr, sid); > + > +unlock: > + qemu_mutex_unlock(&s->mutex); > return entry; > } > > -- > 2.20.1 > -- Peter Xu