On Tue, Sep 03, 2019 at 01:37:11PM +0200, Auger Eric wrote: > Hi Peter, > > On 8/19/19 10:11 AM, Peter Xu wrote: > > On Tue, Jul 30, 2019 at 07:21:30PM +0200, Eric Auger wrote: > > > > [...] > > > >> + mapping = g_tree_lookup(domain->mappings, (gpointer)(&interval)); > >> + > >> + while (mapping) { > >> + viommu_interval current; > >> + uint64_t low = mapping->virt_addr; > >> + uint64_t high = mapping->virt_addr + mapping->size - 1; > >> + > >> + current.low = low; > >> + current.high = high; > >> + > >> + if (low == interval.low && size >= mapping->size) { > >> + g_tree_remove(domain->mappings, (gpointer)(¤t)); > >> + interval.low = high + 1; > >> + trace_virtio_iommu_unmap_left_interval(current.low, > >> current.high, > >> + interval.low, interval.high); > >> + } else if (high == interval.high && size >= mapping->size) { > >> + trace_virtio_iommu_unmap_right_interval(current.low, > >> current.high, > >> + interval.low, interval.high); > >> + g_tree_remove(domain->mappings, (gpointer)(¤t)); > >> + interval.high = low - 1; > >> + } else if (low > interval.low && high < interval.high) { > >> + trace_virtio_iommu_unmap_inc_interval(current.low, > >> current.high); > >> + g_tree_remove(domain->mappings, (gpointer)(¤t)); > >> + } else { > >> + break; > >> + } > >> + if (interval.low >= interval.high) { > >> + return VIRTIO_IOMMU_S_OK; > >> + } else { > >> + mapping = g_tree_lookup(domain->mappings, > >> (gpointer)(&interval)); > >> + } > >> + } > >> + > >> + if (mapping) { > >> + qemu_log_mask(LOG_GUEST_ERROR, > >> + "****** %s: Unmap 0x%"PRIx64" size=0x%"PRIx64 > >> + " from 0x%"PRIx64" size=0x%"PRIx64" is not > >> supported\n", > >> + __func__, interval.low, size, > >> + mapping->virt_addr, mapping->size); > >> + } else { > >> + return VIRTIO_IOMMU_S_OK; > >> + } > >> + > >> + return VIRTIO_IOMMU_S_INVAL; > > > > Could the above chunk be simplified as something like below? > > > > while ((mapping = g_tree_lookup(domain->mappings, &interval))) { > > g_tree_remove(domain->mappings, mapping); > > } > Indeed the code could be simplified. I only need to make sure I don't > split an existing mapping.
Hmm... Do we need to still split an existing mapping if necessary? For example when with this mapping: iova=0x1000, size=0x2000, phys=ADDR1, flags=FLAGS1 And if we want to unmap the range (iova=0, size=0x2000), then we should split the existing mappping and leave this one: iova=0x2000, size=0x1000, phys=(ADDR1+0x1000), flags=FLAGS1 Right? > > Also I needed to use g_tree_lookup_extended to retrieve the actual key > to remove. The usage of g_tree_lookup_extended() allows me to remove the > virt_addr and size fields from the mapping value value struct as those > info can be retrieved from the key. True. Thanks, -- Peter Xu