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

[Mukesh R <mrathor@linux.microsoft.com>]

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