Re: [PATCH v5 7/9] x86/hyperv: Implement Hyper-V virtual IOMMU

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Jul 31, 2026 at 03:34:25PM -0700, Mukesh R wrote:
> +struct iommu_domain_geometry default_geometry = (struct iommu_domain_geometry) {
> +	.aperture_start = 0,
> +	.aperture_end = -1UL,
> +	.force_aperture = true,
> +};

This should not exist

> +/*
> + * If the current thread is a VMM thread, return the partition id of the VM it
> + * is managing, else return HV_PARTITION_ID_INVALID.
> + */
> +static u64 hv_get_current_partid(void)
> +{

No, you cannot transparently detect VMMs and link them like this. The
VMM makes it self visible to the iommu driver via the viommu interface
and you get a kvm FD to fish your partid out of. This is hackery not OK.

You should come with VMM support as a followup once you get a basic
kernel-only iommu driver working.

> +static struct iommu_domain *hv_iommu_domain_alloc_paging(struct device *dev)
> +{
> +	struct hv_domain *hvdom;
> +	int rc;
> +	u32 unique_id;
> +	u64 ptid = hv_get_current_partid();
> +
> +	if (ptid == HV_PARTITION_ID_INVALID)
> +		return NULL;
> +
> +	hvdom = kzalloc_obj(struct hv_domain);
> +	if (hvdom == NULL)
> +		return NULL;
> +
> +	spin_lock_init(&hvdom->mappings_lock);
> +	hvdom->mappings_tree = RB_ROOT_CACHED;
> +
> +	unique_id = (u32)atomic_inc_return(&hv_unique_id);
> +	if (unique_id == HV_DEVICE_DOMAIN_ID_S2_NULL)	/* ie, UINTMAX */
> +		goto out_err;
> +
> +	hvdom->domid_num = unique_id;
> +	hvdom->partid = ptid;
> +	hvdom->iommu_dom.geometry = default_geometry;
> +	hvdom->iommu_dom.pgsize_bitmap = HV_IOMMU_PGSIZES;

This is the only place that needs it, and I somehow doubt -1 is the
right end value since that isn't supported by most HW.

> +static int hv_iommu_attach_dev(struct iommu_domain *immdom, struct device *dev,
> +			       struct iommu_domain *old)
> +{
> +	struct pci_dev *pdev;
> +	int rc;
> +	struct hv_domain *hvdom_new = to_hv_domain(immdom);
> +	struct hv_domain *hvdom_prev = to_hv_domain(old);
> +
> +	/* Only allow PCI devices for now */
> +	if (!dev_is_pci(dev))
> +		return -EINVAL;
> +
> +	pdev = to_pci_dev(dev);
> +
> +	/* There are no explicit detach calls, hence check if we need to detach
> +	 * first. Also, in case of guest shutdown, it's the VMM thread that
> +	 * attaches it back to the hv_def_identity_dom, and hvdom_prev will not
> +	 * be null then. It is null during boot.
> +	 */
> +	if (hvdom_prev && !hv_special_domain(hvdom_prev))
> +		hv_iommu_detach_dev(hvdom_prev, dev);

What translation does this set? If it is anything other than blocking
it is security broken for VFIO.

If it is blocking then why does  this:

> +	rc = hv_iommu_att_dev2dom(hvdom_new, pdev);

Attach HV_DEVICE_DOMAIN_ID_S2_NULL ?

> +	if (rc == 0)
> +		dev_iommu_priv_set(dev, hvdom_new);  /* sets "private" field */

The only thing the priv is used for is release_device ?

It would be better to have a 'detach domain' as the
release_domain so you don't need this.

> +static void hv_iommu_probe_finalize(struct device *dev)
> +{
> +	struct iommu_domain *immdom = iommu_get_domain_for_dev(dev);
> +
> +	if (immdom && immdom->type == IOMMU_DOMAIN_DMA)
> +		iommu_setup_dma_ops(dev, immdom);
> +	else
> +		set_dma_ops(dev, NULL);
> +}

I've forgotten now, but I thought we had reached the point of getting
rid of this from most drivers? amd and vtd do not implement this, why
does this need it?

> +static void hv_iommu_release_device(struct device *dev)
> +{
> +	struct hv_domain *hvdom = dev_iommu_priv_get(dev);
> +
> +	/* Need to detach device from device domain if necessary. */
> +	if (hvdom)
> +		hv_iommu_detach_dev(hvdom, dev);

What does "detach" actually do? What translation will be in effect for
the device?

Ideally you should set the release_domain to blocking or identity and
arrange things so that is enough to destroy the iommu attachment. But
I see both blocking and identity do new attaches so IDK what this
trying to do..

> +static int hv_iommu_def_domain_type(struct device *dev)
> +{
> +	/* The hypervisor always creates this by default during boot */
> +	return IOMMU_DOMAIN_IDENTITY;
> +}

That isn't what this does, it overrides the policy set by Linux. Fully
functional HW should not implement this function, please remove it.

> +static struct iommu_ops hv_iommu_ops = {
> +	.capable	    = hv_iommu_capable,
> +	.domain_alloc_paging	= hv_iommu_domain_alloc_paging,
> +	.probe_device	    = hv_iommu_probe_device,
> +	.probe_finalize     = hv_iommu_probe_finalize,
> +	.release_device     = hv_iommu_release_device,
> +	.def_domain_type    = hv_iommu_def_domain_type,
> +	.device_group	    = hv_iommu_device_group,
> +	.default_domain_ops = &(const struct iommu_domain_ops) {
> +		.attach_dev   = hv_iommu_attach_dev,
> +		.map_pages    = hv_iommu_map_pages,
> +		.unmap_pages  = hv_iommu_unmap_pages,
> +		.iova_to_phys = hv_iommu_iova_to_phys,
> +		.free	      = hv_iommu_domain_free,
> +	},

Please don't use default_domain_ops, this should a new struct
hv_paging_domain_ops

> +	.owner		    = THIS_MODULE,
> +	.identity_domain = &hv_def_identity_dom.iommu_dom,
> +	.blocked_domain  = &hv_null_dom.iommu_dom,

Can we call null dom blocked dom please?

> +static void __init hv_initialize_special_domains(void)
> +{
> +	hv_def_identity_dom.iommu_dom.type = IOMMU_DOMAIN_IDENTITY;
> +	hv_def_identity_dom.iommu_dom.ops = &hv_special_domain_ops;
> +	hv_def_identity_dom.iommu_dom.owner = &hv_iommu_ops;
> +	hv_def_identity_dom.iommu_dom.geometry = default_geometry;
> +	hv_def_identity_dom.domid_num = HV_DEVICE_DOMAIN_ID_S2_DEFAULT; /* 0 */
> +
> +	hv_null_dom.iommu_dom.type = IOMMU_DOMAIN_BLOCKED;
> +	hv_null_dom.iommu_dom.ops = &hv_special_domain_ops;
> +	hv_null_dom.iommu_dom.owner = &hv_iommu_ops;
> +	hv_null_dom.iommu_dom.geometry = default_geometry;
> +	hv_null_dom.domid_num = HV_DEVICE_DOMAIN_ID_S2_NULL;  /* INTMAX */

These ones don't use geometry. Didn't I say this once before?

Jason