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

[Jason Wang <jasowang@redhat.com>]

On 2020/8/5 上午4:30, Peter Xu wrote:
> 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.


Or having another dedicated device IOTLB notifier.


>    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).


Yes.

Thanks


>
> Thanks,
>