Re: [PATCH v2 3/4] iommu/hyperv: Add para-virtualized IOMMU support for Hyper-V guest

On Fri, Jul 03, 2026 at 02:32:48PM -0300, Jason Gunthorpe wrote:
> On Fri, Jul 03, 2026 at 12:05:17AM +0800, Yu Zhang wrote:
> 
> > +static bool hv_iommu_capable(struct device *dev, enum iommu_cap cap)
> > +{
> > +	switch (cap) {
> > +	case IOMMU_CAP_CACHE_COHERENCY:
> > +		return true;
> > +	case IOMMU_CAP_DEFERRED_FLUSH:
> > +		return true;
> 
> This CAP isn't necessary anymore
> 

Right, thanks for pointing this out!

> > +static struct iommu_device *hv_iommu_probe_device(struct device *dev)
> > +{
> > +	struct pci_dev *pdev;
> > +	struct hv_iommu_endpoint *vdev;
> > +	struct hv_output_get_logical_device_property device_iommu_property = {0};
> > +
> > +	if (!dev_is_pci(dev))
> > +		return ERR_PTR(-ENODEV);
> > +
> > +	pdev = to_pci_dev(dev);
> > +
> > +	if (hv_iommu_get_logical_device_property(dev,
> > +						 HV_LOGICAL_DEVICE_PROPERTY_PVIOMMU,
> > +						 &device_iommu_property) ||
> > +	    !(device_iommu_property.device_iommu & HV_DEVICE_IOMMU_ENABLED))
> > +		return ERR_PTR(-ENODEV);
> > +
> > +	vdev = kzalloc_obj(*vdev, GFP_KERNEL);
> > +	if (!vdev)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	vdev->dev = dev;
> > +	vdev->hv_iommu = hv_iommu_device;
> > +	dev_iommu_priv_set(dev, vdev);
> > +
> > +	if (hv_iommu_ats_supported(hv_iommu_device->cap) &&
> > +	    pci_ats_supported(pdev))
> > +		pci_enable_ats(pdev, __ffs(hv_iommu_device->pgsize_bitmap));
> 
> This can probably just be PAGE_SHIFT

Indeed. 

> 
> Also ATS shouldn't be enabled until a translation is installed,
> otherwise the driver cannot participate in the ATS error handling
> Nicolin is working on.
> 

Yes.I'll move ATS enablement into a paging-domain-specific attach wrapper
that calls pci_enable_ats(pdev, PAGE_SHIFT) after the attach hypercall
succeeds. And maybe blocking attach shall also call pci_disable_ats()
before  the hypercall to prevent stale ATC entries from bypassing the
block?

Something like:

    static int hv_iommu_paging_attach_dev(...)
    {
        ret = hv_iommu_attach_dev(domain, dev, old);
        if (ret)
            return ret;
        if (!pdev->ats_enabled && ats_supported)
            pci_enable_ats(pdev, PAGE_SHIFT);
        return 0;
    }

    static int hv_iommu_blocking_attach_dev(...)
    {
        if (pdev->ats_enabled)
            pci_disable_ats(pdev);
        ret = hv_iommu_attach_dev(domain, dev, old);
        ...
    }

Does this look right?

> > +static void hv_iommu_release_device(struct device *dev)
> > +{
> > +	struct hv_iommu_endpoint *vdev = dev_iommu_priv_get(dev);
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +
> > +	if (pdev->ats_enabled)
> > +		pci_disable_ats(pdev);
> > +
> > +	dev_iommu_priv_set(dev, NULL);
> 
> No necessary, the caller does it
> 

Yes. Thanks!

> > +static struct iommu_group *hv_iommu_device_group(struct device *dev)
> > +{
> > +	if (dev_is_pci(dev))
> > +		return pci_device_group(dev);
> > +
> > +	WARN_ON_ONCE(1);
> > +	return generic_device_group(dev);
> 
> I think you can just return failure here instead of WARN_ON ?
> 

Yes, will change to return ERR_PTR(-ENODEV).

> > +static int __init hv_initialize_static_domains(void)
> > +{
> > +	int ret;
> > +	struct hv_iommu_domain *hv_domain;
> > +
> > +	/* Default stage-1 identity domain */
> > +	hv_domain = &hv_identity_domain;
> > +
> > +	ret = hv_create_device_domain(hv_domain, HV_DEVICE_DOMAIN_TYPE_S1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = hv_configure_device_domain(hv_domain, IOMMU_DOMAIN_IDENTITY);
> > +	if (ret)
> > +		goto delete_identity_domain;
> 
> IMHO I would change this around to have a single function that accepts
> a struct hv_input_configure_device_domain as input and does both of
> the hypercalls inside. Then here it is easy to directly construct the
> hv_input_configure_device_domain for blocking and identity.
> 
> I'd be happy if this never touched domain_type, drivers shouldn't be
> touching that.
> 

Good idea. Maybe we can just change hv_configure_device_domain()
to take a "struct hv_device_domain_settings *" directly - that way each
caller constructs the HW settings explicitly and domain_type is not
needed at all. Does that sound right?

> > +static void __init hv_init_iommu_device(struct hv_iommu_dev *hv_iommu,
> > +			struct hv_output_get_iommu_capabilities *hv_iommu_cap)
> > +{
> > +	ida_init(&hv_iommu->domain_ids);
> > +
> > +	hv_iommu->cap = hv_iommu_cap->iommu_cap;
> > +	hv_iommu->max_iova_width = hv_iommu_cap->max_iova_width;
> > +	if (!hv_iommu_5lvl_supported(hv_iommu->cap) &&
> > +	    hv_iommu->max_iova_width > 48) {
> > +		pr_info("5-level paging not supported, limiting iova width to 48.\n");
> > +		hv_iommu->max_iova_width = 48;
> > +	}
> > +
> > +	hv_iommu->geometry = (struct iommu_domain_geometry) {
> > +		.aperture_start = 0,
> > +		.aperture_end = (((u64)1) << hv_iommu->max_iova_width) - 1,
> > +		.force_aperture = true,
> > +	};
> 
> I don't see anything reading this, I don't expect this to be used?
> 
> The max_iova_width has to be passed into the iommupt creation, which
> it does:
> 
>  +	cfg.common.hw_max_vasz_lg2 = hv_iommu_device->max_iova_width;
>  +	cfg.common.hw_max_oasz_lg2 = 52;
>  +	cfg.top_level = (hv_iommu_device->max_iova_width > 48) ? 4 : 3;
>  +	ret = pt_iommu_x86_64_init(&hv_domain->pt_iommu_x86_64, &cfg, GFP_KERNEL);
>  +	if (ret)
> 
> So just delete hv->iommu->geometry.
> 

Right. Will remove.

> Also, VT-D has weirdness where the HW can require a 4 level table but
> only a 3 level worth of IOVA width is being used. This was a
> real-world bug we hit when converting to iommupt. This interaction
> with the HV doesn't seem able to represent that.
> 

Is this the issue fixed by commit d856f9d27885 ("iommupt/vtd: Allow
VT-d to have a larger table top than the vasz requires")? For pvIOMMU
the first-stage table is either 4-level (max_iova_width <= 48) or
5-level (max_iova_width > 48 && 5lvl cap set). Is there a scenario
where this would still be a problem?

> > +	/*
> > +	 * The page table code only maps x86 page sizes (4K/2M/1G); require the
> > +	 * hypervisor to advertise a non-empty subset of exactly those.
> > +	 */
> > +	if (!hv_iommu_cap.pgsize_bitmap ||
> > +	    (hv_iommu_cap.pgsize_bitmap & ~(u64)(SZ_4K | SZ_2M | SZ_1G))) {
> > +		pr_err("unsupported page sizes: pgsize_bitmap=0x%llx\n",
> > +		       hv_iommu_cap.pgsize_bitmap);
> > +		return -ENODEV;
> > +	}
> 
> This can just be
> 
> if (!(hv_iommu_cap.pgsize_bitmap & PAGE_SHIFT)) {
> 		pr_err("unsupported page sizes: pgsize_bitmap=0x%llx\n",
> 		       hv_iommu_cap.pgsize_bitmap);
> }		return -ENODEV;
> 
> Which is all you really need. If the HV doesn't support 1G it is
> perfectly fine, the iommupt page bitmap is already masked by this. 
> 

Good point, it's much simpler.
And I assume you meant PAGE_SIZE / SZ_4K instead of PAGE_SHIFT here. :) 

> > +	ret = iommu_device_register(&hv_iommu->iommu, &hv_iommu_ops, NULL);
> > +	if (ret) {
> > +		pr_err("iommu_device_register failed: %d\n", ret);
> > +		goto err_sysfs_remove;
> > +	}
> > +
> > +	pr_info("successfully initialized\n");
> 
> Don't log someting so vauge?
> 

With pr_fmt defined as "Hyper-V pvIOMMU: ", this shows up as
"Hyper-V pvIOMMU: successfully initialized" in dmesg. I'd like to
keep some indication that pvIOMMU init succeeded at boot. Is this
still too vague? Would it be better if I also print capabilities
like IOVA width and supported page sizes here? Thanks!

B.R.
Yu

> Jason
>