Re: [PATCH V1 3/3] x86/hyperv: Implement root VM IOMMU kernel only driver

[Mukesh R]

On 9/28/26 06:38, Jason Gunthorpe wrote:
> > [ ... 103 lines skipped ... ]
> > +/*
> > + * We will not claim these PCI devices. Eg hypervisor debugger is using it
> > + * for a dynamic debug session. They cannot be enumerated under static ACPI
> > + * device scope.
> > + */
> > +static char *hv_skip_pci_devs;
> > +static int __init hv_iommu_setup_skip(char *str)
> > +{
> > +	hv_skip_pci_devs = str;
> > +	return 1;
> > +}
> > +/* Eg: hv_iommu_skip=(SSSS:BB:DD.F)(SSSS:BB:DD.F) */
> > +__setup("hv_iommu_skip=", hv_iommu_setup_skip);
>
> I don't like this in a driver. This isn't really skipping anything, it
> is just leaving some devices in an identity mode.

Ok. I talked to the original author of that and i can just remove
it. It's mostly for running hyp debugger and we can just carry
the patch internally, at least for now.


> If you have a use case for this as a general command line policy then
> come with a core code enhacmenet so everyone can choose per-device
> their boot time mode.
>
> > [ ... 7 lines skipped ... ]
> > +struct hv_domain {
> > +	struct iommu_domain iommu_dom;
> > +	u32 domid_num;			      /* as opposed to domain_id.type */
> > +	spinlock_t mappings_lock;	      /* protects mappings_tree */
> > +	struct rb_root_cached mappings_tree;  /* iova to pa lookup tree */
>
> This seems basically identical to what virtio-iommu is doing, can you
> consider sharing its code?

yeah, the tree part is somewhat identical, but virtio-iommu has extra
fields that we don't need. overall, i don't think there is enough here
to refactor, just few lines of code around add/remove calling kernel
interval tree apis. moreover, if hyp can provide the lookup in future,
i'd like to just remove it from here.

> > [ ... 26 lines skipped ... ]
> > +static bool hv_special_domain(struct hv_domain *hvdom)
> > +{
> > +	return hvdom == &hv_def_identity_dom || hvdom == &hv_def_blocked_dom;
> > +}
>
> This is only called by hv_iommu_domain_free() which is only linked to
> hv_paging_domain_ops(), so it should be dead code
>
> > [ ... 204 lines skipped ... ]
> > +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);
>
> 'new' is an odd variable name here

'current' is passed as 'old', so we are moving from old to new i
thought. please tell me what would you like it called, thx.

> > [ ... 246 lines skipped ... ]
> > +static struct iommu_group *hv_iommu_device_group(struct device *dev)
> > +{
> > +	return pci_device_group(dev);
> > +}
>
> No need for a wrapper, use the function directly in the ops

it helps with quick debug... just set breakpoint in hv_iommu_device_group
or add a printk here at the cost of one jmp instruction. but whatever..
i can remove it if it helps move this forward.

> > +
> > +static void hv_iommu_get_resv_regions(struct device *dev,
> > +				      struct list_head *head)
> > +{
> > +	struct iommu_resv_region *reg;
> > +
> > +	/* reserve the entire LAPIC region */
> > +	reg = iommu_alloc_resv_region(0xfee00000, SZ_1M, 0, IOMMU_RESV_MSI,
> > +				      GFP_KERNEL);
>
> There was some discussion to make a helper for this, I don't see it
> merged yet..

we are both waiting on each other, whoever goes first will leave
the follower to address it i guess. i cannot test without this and
i am not sure if that series will merge first or this.

> > [ ... 65 lines skipped ... ]
> > +static int __init hv_iommu_init(void)
> > +{
> > +	int rc;
> > +	struct iommu_device *iommup = &hv_virt_iommu;
> > +	struct hv_output_get_iommu_capabilities caps;
> > +
> > +	if (!hv_is_hyperv_initialized())
> > +		return -ENODEV;
> > +
> > +	rc = hv_iommu_get_caps(&caps);
> > +	if (rc)
> > +		return rc;
> > +
> > +	hv_iommu_max_iova = ((ulong)1 << caps.max_iova_width) - 1;
> > +
> > +	rc = iommu_device_sysfs_add(iommup, NULL, NULL, "%s", "hyperv-iommu");
> > +	if (rc) {
> > +		pr_err("Hyper-V: iommu_device_sysfs_add failed: %d\n", rc);
> > +		return rc;
> > +	}
> > +
> > +	/* This must come before iommu_device_register() because the latter
> > +	 * calls into the hooks.
> > +	 */
> > +	hv_initialize_special_domains();
>
> This probably should be before doing anything with sysfs.

ok.

Thanks,
-Mukesh