Re: [PATCH v2 2/4] Drivers: hv: Add logical device ID registry for vPCI devices

[Yu Zhang <zhangyu1@linux.microsoft.com>]

On Wed, Jul 08, 2026 at 01:28:42PM +0000, Michael Kelley wrote:
> From: Yu Zhang <zhangyu1@linux.microsoft.com> Sent: Wednesday, July 8, 2026 6:09 AM
> > 
> > On Wed, Jul 08, 2026 at 02:52:43AM +0000, Michael Kelley wrote:
> > > From: Yu Zhang <zhangyu1@linux.microsoft.com> Sent: Thursday, July 2, 2026 9:05 AM
> > > >
> > > > From: Easwar Hariharan <easwar.hariharan@linux.microsoft.com>
> > > >
> > > > Hyper-V identifies each PCI pass-thru device by a logical device ID in
> > > > its hypercall interface. This ID consists of a per-bus prefix, derived
> > > > from the VMBus device instance GUID, combined with the PCI function
> > > > number of the endpoint device.
> > > >
> > > > Add a small registry in hv_common.c that maps a PCI domain number to its
> > > > logical device ID prefix. The vPCI bus driver (pci-hyperv) registers the
> > > > prefix when a bus is probed and unregisters it when the bus is removed.
> > > > Consumers such as the para-virtualized IOMMU driver look up the prefix
> > > > by PCI domain number and combine it with the function number to form the
> > > > complete logical device ID for hypercalls.
> > > >
> > > > The prefix construction is shared via hv_build_logical_dev_id_prefix() so
> > > > that pci-hyperv's interrupt retargeting path and the registry use exactly
> > > > the same byte layout. It is derived on demand from the constant hv_device
> > > > instance GUID rather than cached in struct hv_pcibus_device, which is
> > > > private to the pci-hyperv module; this keeps the interface narrow and
> > > > avoids depending on pci-hyperv internals.
> > > >
> > > > Co-developed-by: Yu Zhang <zhangyu1@linux.microsoft.com>
> > > > Signed-off-by: Yu Zhang <zhangyu1@linux.microsoft.com>
> > > > Signed-off-by: Easwar Hariharan <easwar.hariharan@linux.microsoft.com>
> > > > ---
> > > >  drivers/hv/hv_common.c              | 95 +++++++++++++++++++++++++++++
> > > >  drivers/pci/controller/pci-hyperv.c | 21 +++++--
> > > >  include/asm-generic/mshyperv.h      | 13 ++++
> > > >  include/linux/hyperv.h              |  8 +++
> > > >  4 files changed, 132 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> > > > index 6b67ac616789..53493f8d14dc 100644
> > > > --- a/drivers/hv/hv_common.c
> > > > +++ b/drivers/hv/hv_common.c
> > > > @@ -26,6 +26,8 @@
> > > >  #include <linux/kmsg_dump.h>
> > > >  #include <linux/sizes.h>
> > > >  #include <linux/slab.h>
> > > > +#include <linux/list.h>
> > > > +#include <linux/spinlock.h>
> > > >  #include <linux/dma-map-ops.h>
> > > >  #include <linux/set_memory.h>
> > > >  #include <hyperv/hvhdk.h>
> > > > @@ -863,3 +865,96 @@ const char *hv_result_to_string(u64 status)
> > > >  	return "Unknown";
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(hv_result_to_string);
> > > > +
> > > > +#ifdef CONFIG_HYPERV_PVIOMMU
> > > > +/*
> > > > + * Logical device ID registry shared between the vPCI bus driver
> > > > + * (pci-hyperv) and the para-virtualized IOMMU driver. The vPCI driver
> > > > + * registers the per-bus logical device ID prefix at bus probe time, and
> > > > + * the pvIOMMU driver looks it up to build the full logical device ID used
> > > > + * in IOMMU hypercalls.
> > > > + */
> > > > +struct hv_pci_busdata {
> > > > +	int		 pci_domain_nr;
> > > > +	u32		 logical_dev_id_prefix;
> > > > +	struct list_head list;
> > > > +};
> > > > +
> > > > +static LIST_HEAD(hv_pci_bus_list);
> > > > +static DEFINE_SPINLOCK(hv_pci_bus_lock);
> > > > +
> > > > +int hv_iommu_register_pci_bus(int pci_domain_nr, u32 logical_dev_id_prefix)
> > > > +{
> > > > +	struct hv_pci_busdata *bus, *new;
> > > > +	int ret = 0;
> > > > +
> > > > +	new = kzalloc_obj(*new, GFP_KERNEL);
> > > > +	if (!new)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	spin_lock(&hv_pci_bus_lock);
> > > > +	list_for_each_entry(bus, &hv_pci_bus_list, list) {
> > > > +		if (bus->pci_domain_nr != pci_domain_nr)
> > > > +			continue;
> > > > +
> > > > +		if (bus->logical_dev_id_prefix != logical_dev_id_prefix) {
> > > > +			pr_err("stale registration for PCI domain %d (old prefix 0x%08x, new 0x%08x)\n",
> > > > +			       pci_domain_nr, bus->logical_dev_id_prefix,
> > > > +			       logical_dev_id_prefix);
> > > > +			ret = -EEXIST;
> > > > +		}
> > > > +
> > > > +		goto out_free;
> > > > +	}
> > > > +
> > > > +	new->pci_domain_nr = pci_domain_nr;
> > > > +	new->logical_dev_id_prefix = logical_dev_id_prefix;
> > > > +	list_add(&new->list, &hv_pci_bus_list);
> > > > +	spin_unlock(&hv_pci_bus_lock);
> > > > +	return 0;
> > > > +
> > > > +out_free:
> > > > +	spin_unlock(&hv_pci_bus_lock);
> > > > +	kfree(new);
> > > > +	return ret;
> > > > +}
> > > > +EXPORT_SYMBOL_FOR_MODULES(hv_iommu_register_pci_bus, "pci-hyperv");
> > > > +
> > > > +void hv_iommu_unregister_pci_bus(int pci_domain_nr)
> > > > +{
> > > > +	struct hv_pci_busdata *bus, *tmp;
> > > > +
> > > > +	spin_lock(&hv_pci_bus_lock);
> > > > +	list_for_each_entry_safe(bus, tmp, &hv_pci_bus_list, list) {
> > > > +		if (bus->pci_domain_nr == pci_domain_nr) {
> > > > +			list_del(&bus->list);
> > > > +			kfree(bus);
> > > > +			break;
> > > > +		}
> > > > +	}
> > > > +	spin_unlock(&hv_pci_bus_lock);
> > > > +}
> > > > +EXPORT_SYMBOL_FOR_MODULES(hv_iommu_unregister_pci_bus, "pci-hyperv");
> > > > +
> > > > +/*
> > > > + * Look up the logical device ID prefix registered for @pci_domain_nr.
> > > > + * Returns 0 on success with *prefix filled in; -ENODEV if no entry is
> > > > + * registered for that PCI domain.
> > > > + */
> > > > +int hv_iommu_lookup_logical_dev_id(int pci_domain_nr, u32 *prefix)
> > > > +{
> > > > +	struct hv_pci_busdata *bus;
> > > > +	int ret = -ENODEV;
> > > > +
> > > > +	spin_lock(&hv_pci_bus_lock);
> > > > +	list_for_each_entry(bus, &hv_pci_bus_list, list) {
> > > > +		if (bus->pci_domain_nr == pci_domain_nr) {
> > > > +			*prefix = bus->logical_dev_id_prefix;
> > > > +			ret = 0;
> > > > +			break;
> > > > +		}
> > > > +	}
> > > > +	spin_unlock(&hv_pci_bus_lock);
> > > > +	return ret;
> > > > +}
> > >
> > > I started thinking about the mechanism here because it's somewhat
> > > annoying that it takes 77 lines of code (sans comments) to manage
> > > this simple little mapping. I also started thinking about how many entries
> > > are likely to be in the mapping. A guest VM probably has fewer than 10
> > > entries unless it has multiple NICs and maybe some GPUs. But this code
> > > is also intended to be used by the Linux-as-root code, and I'm thinking
> > > that the number of PCI devices managed by the root could easily be a
> > > hundred or more if the root is managing a couple dozen VMs on a large
> > > physical server. Searching a linked list with 100 or more entries could be
> > > a bit slow.
> > >
> > > If only the guest scenario were needed, you could declare a static
> > > array with 64 entries (64 is an arbitrary upper bound), and just search
> > > through the array instead of having to allocate memory, deal with
> > > allocation failures, and deal with linked lists. But a fixed size array
> > > would need to be much bigger for the root scenario, and you would
> > > still be doing a linear search.
> > >
> > > A better alternative to consider is rhashtable, which is an existing
> > > Linux kernel facility. Based on what an AI bot generated for me, the
> > > code for setting up and using rhashtable is straightforward, and
> > > would probably result in far fewer than 77 lines of code. Lookups
> > > would also be faster than a linear search, at least for the root case
> > > with more than just a few entries. I'd suggest looking at rhashtable
> > > to see whether you like how the resulting code comes out for this
> > > use case, and whether it really is simpler than a roll-your-own linked
> > > list.
> > >
> > 
> > Thank you so much for thinking this through, Michael! That is really
> > a valid concern.
> > 
> > How about using XArray? It might be more lightweight compared with
> > rhashtable. Using pci_domain_nr as the key and prefix as the value.
> > Maybe sth. like:
> >                                                                                                                                                         ┃
> > 	static DEFINE_XARRAY(hv_pci_bus_xa);
> > 
> > 	int hv_iommu_register_pci_bus(int domain_nr, u32 prefix)
> > 	{
> > 		return xa_insert(&hv_pci_bus_xa, domain_nr,
> > 				xa_mk_value(prefix), GFP_KERNEL);
> > 	}                                                                                                                                                ┃
> > 
> > 	void hv_iommu_unregister_pci_bus(int domain_nr)
> > 	{
> > 		xa_erase(&hv_pci_bus_xa, domain_nr);
> > 	}
> > 
> > 	int hv_iommu_lookup_logical_dev_id(int domain_nr, u32 *prefix)
> > 	{
> > 		void *entry = xa_load(&hv_pci_bus_xa, domain_nr);
> > 		if (!entry)
> > ┃
> > 			return -ENODEV;
> > 		*prefix = xa_to_value(entry);
> > 		return 0;
> > ┃
> > 	}
> > 
> 
> xarray is best where the keys are dense or mostly dense integers. In
> this use case, the pci_domain_nr keys are essentially random 16-bit
> values, which doesn't fit xarray as well. It would work, but wouldn't
> be very efficient. See the 2nd paragraph of the documentation here:
> 
> https://www.kernel.org/doc/html/latest/core-api/xarray.html#xarray
> 

Thanks for pointing out the documentation. You're right. With sparse
16-bit keys, a hash-based data structure makes more sense than a
radix-based one. I'll try reworking it with rhashtable and let you
know if it doesn't work out.

B.R.
Yu
> Michael