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
