Re: [PATCH v5 5/7] iommu: Add virtio-iommu driver

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Nov 27, 2018 at 05:55:20PM +0000, Jean-Philippe Brucker wrote:
> On 23/11/2018 21:56, Michael S. Tsirkin wrote:
> >> +config VIRTIO_IOMMU
> >> +	bool "Virtio IOMMU driver"
> >> +	depends on VIRTIO=y
> >> +	select IOMMU_API
> >> +	select INTERVAL_TREE
> >> +	select ARM_DMA_USE_IOMMU if ARM
> >> +	help
> >> +	  Para-virtualised IOMMU driver with virtio.
> >> +
> >> +	  Say Y here if you intend to run this kernel as a guest.
> >> +
> > 
> > Given it is arm specific right now, shouldn't this depend on ARM?
> > E.g. there's a hack for x86 right now.
> 
> Sure, I'll make it depend on ARM64 for now
> 
> [..]
> >> +static int viommu_probe(struct virtio_device *vdev)
> >> +{
> >> +	struct device *parent_dev = vdev->dev.parent;
> >> +	struct viommu_dev *viommu = NULL;
> >> +	struct device *dev = &vdev->dev;
> >> +	u64 input_start = 0;
> >> +	u64 input_end = -1UL;
> >> +	int ret;
> >> +
> >> +	if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1) ||
> >> +	    !virtio_has_feature(vdev, VIRTIO_IOMMU_F_MAP_UNMAP))
> > 
> > Why bother with a feature bit for this then btw?
> 
> We'll need a new feature bit for sharing page tables with the hardware,
> because they require different requests (attach_table/invalidate instead
> of map/unmap.) A future device supporting page table sharing won't
> necessarily need to support map/unmap.
> 
> Thanks,
> Jean

I don't see virtio iommu being extended to support ARM specific
requests. This just won't scale, too many different
descriptor formats out there.

If you want to go that way down the road, you should avoid
virtio iommu, instead emulate and share code with the ARM SMMU (probably
with a different vendor id so you can implement the
report on map for devices without PRI).

Others on the TC might feel differently.

If someone's looking into adding virtio iommu support in hardware,
that's a different matter. Which is it?

-- 
MST