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

From: Peter Xu <peterx@redhat.com>
To: Eugenio Perez Martin <eperezma@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	Yan Zhao <yan.y.zhao@intel.com>,
	Juan Quintela <quintela@redhat.com>,
	Jason Wang <jasowang@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	qemu-devel@nongnu.org, Eric Auger <eric.auger@redhat.com>,
	Avi Kivity <avi@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [RFC v3 0/1] memory: Delete assertion in memory_region_unregister_iommu_notifier
Date: Tue, 11 Aug 2020 15:27:57 -0400	[thread overview]
Message-ID: <20200811192757.GB6353@xz-x1> (raw)
In-Reply-To: <CAJaqyWf24F8ERAuvYj+0Xv=+cYj4v1MCmvCmQFTqcfLVYYCVuQ@mail.gmail.com>

Hi, Eugenio,

On Tue, Aug 11, 2020 at 08:10:44PM +0200, Eugenio Perez Martin wrote:
> Using this patch as a reference, I'm having problems to understand:
> 
> - I'm not sure that the flag name expresses clearly the notifier capability.

The old code is kind of messed up for dev-iotlb invalidations, by always
sending UNMAP notifications for both iotlb and dev-iotlb invalidations.

Now if we introduce the new DEV_IOTLB type, we can separate the two:

  - We notify IOMMU_NOTIFIER_UNMAP for iotlb invalidations

  - We notify IOMMU_NOTIFIER_DEV_IOTLB for dev-iotlb invalidations

Vhost should always be with ats=on when vIOMMU enabled, so it will enable
device iotlb.  Then it does not need iotlb (UNMAP) invalidation any more
because they'll normally be duplicated (one is to invalidate vIOMMU cache, one
is to invalidate device cache).

Also, we can drop UNMAP type for vhost if we introduce DEV_IOTLB.  It works
just like on the real hardwares - the device won't be able to receive iotlb
invalidation messages, but only device iotlb invalidation messages.  Here
vhost (or, virtio-pci) is the device.

> - What would be the advantages of using another field (NotifierType?)
> in the notifier to express that it accepts arbitrary ranges for
> unmapping? (If I understood correctly Jason's proposal)

(Please refer to above too..)

> - Is it possible (or advisable) to skip all the page splitting in
> vtd_page_walk if the memory range notifier supports these arbitrary
> ranges? What would be the disadvantages? (Maybe in a future patch). It
> seems it is advisable to me, but I would like to double confirm.

vtd_page_walk is not used for dev-iotlb, we don't need to change that.  We also
want to explicitly keep the page granularity of vtd_page_walk for the other
IOMMU notifiers, e.g. vfio.

Though we'll need to modify vtd_process_device_iotlb_desc() to only send
notifications to the notifiers that registered with DEV_IOTLB flag.

Hope it helps..

Thanks,

-- 
Peter Xu