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

On Mon, Aug 17, 2026 at 05:27:34PM +0000, Michael Kelley wrote:
> From: Yu Zhang <[email protected]> Sent: Monday, August 17, 2026 3:38 AM
> > 
> > On Fri, Aug 14, 2026 at 03:31:27PM +0000, Michael Kelley wrote:
> > > From: Yu Zhang <[email protected]> Sent: Tuesday, August 11, 2026 8:50 AM
> > > >
> 
> [snip]
> 
> > > > +#ifdef CONFIG_HYPERV_PVIOMMU
> > >
> > > Making this code in hv_common.c conditional on HYPERV_PVIOMMU
> > > assumes that the only consumer is the pvIOMMU driver. But my
> > > understanding is that root partition code is also expected to be a
> > > consumer, and I would guess that code is independent of a guest
> > > pvIOMMU. So the #ifdef might need to become more complicated
> > > to also allow the root partition case. I'm thinking it makes sense to
> > > always build this code as part of hv_common.c. Even when building
> > > for a guest only with no root partition code, HYPERV_PVIOMMU
> > > defaults to HYPERV so the pvIOMMU driver will be built anytime
> > > Hyper-V code is included, unless someone goes out of their way to
> > > disable it. Getting rid of all the #ifdef'ery would make the code a bit
> > > cleaner, and I'm looking for an excuse to do so!
> > >
> > > I'd also note that CONFIG_HYPERV_PVIOMMU isn't defined
> > > until Patch 4 of this series. This patch will still build without the
> > > remaining patches, so bisect isn't broken, but it would be with
> > > the equivalent of CONFIG_HYPERV_PVIOMMU=n. It's a little bit
> > > weird to be using a CONFIG_* value before it is created, but maybe
> > > the ordering among the patches is complex and not easily sorted.
> > >
> > 
> > Thank you, Michael!
> > 
> > That makes sense. The trade-off is that a Hyper-V guest without
> > pvIOMMU support will still allocate and populate the registry for
> > its PCI buses. If that small overhead is acceptable, building it
> > unconditionally does make the design simpler. And we do not need
> > to worry about the chicken-and-egg problem when ordering this patch
> > and patch 4. :)
> 
> To me, the small overhead is acceptable. vPCI add/remove
> operations are relatively infrequent, and are not a "fast" path. So
> the overhead is of no real consequence.

Yes. Let's use rhashtable!

> 
> [snip]
> 
> > >
> > > The comment above is written as if the pvIOMMU driver is the only
> > > consumer, which isn't accurate assuming the root partition use case
> > > materializes as expected.  And that also means having "iommu" in the
> > > function name is overly specific. Same with the other function names
> > > with "iommu".  There's really nothing here that is specific to pvIOMMUs.
> > >
> > 
> > Agreed. Will update the comment.
> > 
> > For the function names, I would like to avoid using sth. like
> > hv_register_pci_bus() since that sounds like to register the
> > PCI bus itself. Two options I'm having in mind are:
> > 
> > 	hv_pci_busdata_register()
> > 	hv_pci_busdata_unregister()
> > 	hv_pci_busdata_lookup()
> > 
> > or some more descriptive yet legthy ones, e.g.,
> > 
> > 	hv_pci_bus_register_logical_dev_id_prefix()
> > 	hv_pci_bus_unregister_logical_dev_id_prefix()
> > 	hv_pci_bus_lookup_logical_dev_id_prefix()
> > 
> > Do you have a preference, or any suggestion?
> 
> Or maybe just hv_pci_register_dev_id()? To me, it's
> better to put the verb ("register") first followed by the
> object. "logical_dev_id_prefix" as the object is a bit
> too wordy for my preference.  Just "dev_id" or "busdata"
> is better, even if neither is fully precise.
> 

I would prefer hv_pci_register_dev_id(). "busdata" is kinda
vague and "logical_dev_id_prefix" is too wordy. :)

> [snip]
> 
> > 
> > > Separately, it seems like the existing calls from the Hyper-V vPCI driver should
> > > never duplicate an existing mapping. Your code disallows mapping a domain_nr
> > > to a different logical_dev_id_prefix, but silently allows entering a mapping
> > > that already exists as an exact match. I wonder if that really should be
> > > allowed, as it seems likely such would be followed later by twice removing
> > > the duplicate entry, with the second removal failing. Removing a non-existent
> > > entry is also silently allowed in the unregister function below, with the same
> > > question as to whether that should be allowed.
> > >
> > 
> > Actually, I don't think an exact duplicate registration is a valid case
> > either. So how about we use WARN_ONCE() and return -EEXIST when an entry
> > for the PCI domain is already registered(regardless it is a exact match
> > or not)?
> > 
> > And for unregister, how about we initialize the result to -ENOENT, do the
> > removal if lookup finds the entry, and WARN_ONCE if no entry is really
> > removed?
> 
> Works for me.
> 
> > 
> > > > +}
> > > > +EXPORT_SYMBOL_FOR_MODULES(hv_iommu_register_pci_bus, "pci-hyperv");
> > >
> > > This may be overly restrictive if the root partition use case code is in the
> > > mshv_root module.
> > >
> > 
> > Well, I'm not sure. IMHO, the register/unregister side is owned by
> > pci-hyperv, which creates and removes the Hyper-V PCI bus. The guest
> > pvIOMMU or the root driver are only cumsumers of the  lookup.
> > 
> > So maybe the register/unregister exports should remain restricted to
> > pci-hyperv, and if a future module needs the lookup helper, an export
> > can be added for the lookup helper by then (we do not need to export
> > lookup right now for pvIOMMU, which is built-in)?
> > 
> > Am I missing anything?
> 
> You are correct and didn't miss anything. I didn't fully think it through,
> so my comment is bogus. What you propose is fine.
> 

Glad to know that. Thanks!

B.R.
Yu
> Michael
>