On Wed, Jul 04, 2018 at 05:08:34PM +0100, Lorenzo Pieralisi wrote:
> On Tue, Jun 12, 2018 at 09:40:37AM -0700, Stephen Hemminger wrote:
> > The Hyper-V host API for PCI provides a unique "serial number" which
> > can be used as the basis for sysfs PCI slot table. This can be useful
> > for cases where userspace wants to find the PCI device based on
> > serial number.
> >
> > When an SR-IOV NIC is added, the host sends an attach message
> > with a serial number. The kernel doesn't use the serial number, but
> > it is useful when doing the same thing in a userspace driver such
> > as the DPDK. By having /sys/bus/pci/slots/N it provides a direct
> > way to find the matching PCI device.
Is this essentially a way to expose the serial number to userspace?
I confess I'm not super familiar with what the slot infrastructure
gives us, and I'm trying to figure out if it's the right way to do
this. Would a file directly in the device directory, e.g.,
/sys/devices/pci0000:00/0000:00:00.0/serial
be a possibility? There is an optional PCIe Device Serial Number
capability and also an optional serial number item in the VPD. We
don't support either in the core yet, but maybe they could all be
exposed similarly?
Or is there something you need from the slot infrastructure in
addition to the serial number exposure?
You mentioned that this patch is along the lines of what KVM does.
Can you connect the dots a little bit for me as far as how this is
implemented for KVM? I don't see KVM-related calls to
pci_create_slot() or pci_hp_register().
> > There may be some cases where the serial number is not unique such
> > as when using GPU's. But the PCI slot infrastructure will handle
> > that.
> >
> > This also shortens the network device names generated by
> > systemd/udev. The new names use slot (ens2) rather than
> > PCI address (enP2p0s2).
I don't recognize "enP2p0s2" as a PCI address. Is that related to a
domain/bus/device/function address somehow, or is this some sort of
Windows-specific address?
> Hi Stephen,
>
> I wanted to apply this patch but wanted to make sure all HV
> maintainers are in agreement first since this looks like
> a significant user-space ABI change.
>
> I would also ask Bjorn's opinion on this since he has more
> insights into the slot interface history.
>
> Thanks,
> Lorenzo
>
> > Signed-off-by: Stephen Hemminger <sthemmin@xxxxxxxxxxxxx>
> > ---
> > v2
> > - retarget for current filenames in PCI next
> > - remove debug log message
> >
> > drivers/pci/controller/pci-hyperv.c | 30 +++++++++++++++++++++++++++++
> > 1 file changed, 30 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> > index 6cc5036ac83c..4e3575716ced 100644
> > --- a/drivers/pci/controller/pci-hyperv.c
> > +++ b/drivers/pci/controller/pci-hyperv.c
> > @@ -88,6 +88,8 @@ static enum pci_protocol_version_t pci_protocol_version;
> >
> > #define STATUS_REVISION_MISMATCH 0xC0000059
> >
> > +#define SLOT_NAME_SIZE 21
> > +
> > /*
> > * Message Types
> > */
> > @@ -493,6 +495,7 @@ struct hv_pci_dev {
> > struct list_head list_entry;
> > refcount_t refs;
> > enum hv_pcichild_state state;
> > + struct pci_slot *pci_slot;
> > struct pci_function_description desc;
> > bool reported_missing;
> > struct hv_pcibus_device *hbus;
> > @@ -1454,6 +1457,28 @@ static void prepopulate_bars(struct hv_pcibus_device *hbus)
> > spin_unlock_irqrestore(&hbus->device_list_lock, flags);
> > }
> >
> > +static void hv_pci_assign_slots(struct hv_pcibus_device *hbus)
> > +{
> > + struct hv_pci_dev *hpdev;
> > + char name[SLOT_NAME_SIZE];
> > + unsigned long flags;
> > + int slot_nr;
> > +
> > + spin_lock_irqsave(&hbus->device_list_lock, flags);
> > + list_for_each_entry(hpdev, &hbus->children, list_entry) {
> > + if (hpdev->pci_slot)
> > + continue;
> > +
> > + slot_nr = PCI_SLOT(wslot_to_devfn(hpdev->desc.win_slot.slot));
> > + snprintf(name, SLOT_NAME_SIZE, "%u", hpdev->desc.ser);
> > + hpdev->pci_slot = pci_create_slot(hbus->pci_bus, slot_nr,
> > + name, NULL);
> > + if (!hpdev->pci_slot)
> > + pr_warn("pci_create slot %s failed\n", name);
Is there any way this could be a dev_warn() so it's connected with
something? Does hpdev->hbus->hdev->device make sense?
> > + }
> > + spin_unlock_irqrestore(&hbus->device_list_lock, flags);
> > +}
> > +
> > /**
> > * create_root_hv_pci_bus() - Expose a new root PCI bus
> > * @hbus: Root PCI bus, as understood by this driver
> > @@ -1477,6 +1502,7 @@ static int create_root_hv_pci_bus(struct hv_pcibus_device *hbus)
> > pci_lock_rescan_remove();
> > pci_scan_child_bus(hbus->pci_bus);
> > pci_bus_assign_resources(hbus->pci_bus);
> > + hv_pci_assign_slots(hbus);
> > pci_bus_add_devices(hbus->pci_bus);
> > pci_unlock_rescan_remove();
> > hbus->state = hv_pcibus_installed;
> > @@ -1739,6 +1765,7 @@ static void pci_devices_present_work(struct work_struct *work)
> > */
> > pci_lock_rescan_remove();
> > pci_scan_child_bus(hbus->pci_bus);
> > + hv_pci_assign_slots(hbus);
> > pci_unlock_rescan_remove();
> > break;
> >
> > @@ -1855,6 +1882,9 @@ static void hv_eject_device_work(struct work_struct *work)
> > list_del(&hpdev->list_entry);
> > spin_unlock_irqrestore(&hpdev->hbus->device_list_lock, flags);
> >
> > + if (hpdev->pci_slot)
> > + pci_destroy_slot(hpdev->pci_slot);
> > +
> > memset(&ctxt, 0, sizeof(ctxt));
> > ejct_pkt = (struct pci_eject_response *)&ctxt.pkt.message;
> > ejct_pkt->message_type.type = PCI_EJECTION_COMPLETE;
> > --
> > 2.17.1
> >
