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
