Re: [RFC v2 1/1] memory: Delete assertion in memory_region_unregister_iommu_notifier

On Mon, Aug 03, 2020 at 06:00:34PM +0200, Eugenio Pérez wrote: > On Fri, 2020-07-03 at 15:24 +0800, Jason Wang wrote: >> On 2020/7/2 下午11:45, Peter Xu wrote: >>> On Thu, Jul 02, 2020 at 11:01:54AM +0800, Jason Wang wrote: >>>> So I think we agree that a new notifier is needed? >>> Good to me, or a new flag should be easier (IOMMU_NOTIFIER_DEV_IOTLB)? >> That should work but I wonder something as following is better. >> >> Instead of introducing new flags, how about carry the type of event in >> the notifier then the device (vhost) can choose the message it want to >> process like: >> >> static vhost_iommu_event(IOMMUNotifier *n, IOMMUTLBEvent *event) >> >> { >> >> switch (event->type) { >> >> case IOMMU_MAP: >> case IOMMU_UNMAP: >> case IOMMU_DEV_IOTLB_UNMAP: >> ... >> >> } >> >> Thanks >> >> > Hi! > > Sorry, I thought I had this clear but now it seems not so clear to me. Do you mean to add that switch to the current > vhost_iommu_unmap_notify, and then the "type" field to the IOMMUTLBEntry? Is that the scope of the changes, or there is > something I'm missing? > > If that is correct, what is the advantage for vhost or other notifiers? I understand that move the IOMMUTLBEntry (addr, > len) -> (iova, mask) split/transformation to the different notifiers implementation could pollute them, but this is even a deeper change and vhost is not insterested in other events but IOMMU_UNMAP, isn't? > > On the other hand, who decide what type of event is? If I follow the backtrace of the assert in > https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg01015.html, it seems to me that it should be > vtd_process_device_iotlb_desc. How do I know if it should be IOMMU_UNMAP or IOMMU_DEV_IOTLB_UNMAP? If I set it in some > function of memory.c, I should decide the type looking the actual notifier, isn't? (Since Jason didn't reply yesterday, I'll try to; Jason, feel free to correct me...) IMHO whether to put the type into the IOMMUTLBEntry is not important. The important change should be that we introduce IOMMU_DEV_IOTLB_UNMAP (or I'd rather call it IOMMU_DEV_IOTLB directly which is shorter and cleaner). With that information we can make the failing assertion conditional for MAP/UNMAP only.

We can also allow dev-iotlb messages to take arbitrary addr_mask (so it becomes a length of address range; imho we can keep using addr_mask for simplicity, but we can comment for addr_mask that for dev-iotlb it can be not a real mask).

Thanks,