Re: [PATCH] PCI: hv: support reporting serial number as slot information

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 19 Jul 2018 17:58:59 +0100
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:

> On Fri, Jul 13, 2018 at 02:38:19PM -0700, Stephen Hemminger wrote:
> > On Wed, 11 Jul 2018 08:49:08 -0500
> > Bjorn Helgaas <helgaas@kernel.org> wrote:
> >  =20
> > > On Wed, Jul 04, 2018 at 05:08:34PM +0100, Lorenzo Pieralisi wrote: =20
> > > > On Tue, Jun 12, 2018 at 09:40:37AM -0700, Stephen Hemminger wrote: =
  =20
> > > > > The Hyper-V host API for PCI provides a unique "serial number" wh=
ich
> > > > > can be used as the basis for sysfs PCI slot table. This can be us=
eful
> > > > > for cases where userspace wants to find the PCI device based on
> > > > > serial number.
> > > > >=20
> > > > > 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, b=
ut
> > > > > 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.   =20
> > >=20
> > > Is this essentially a way to expose the serial number to userspace?
> > >=20
> > > 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.,
> > >=20
> > >   /sys/devices/pci0000:00/0000:00:00.0/serial
> > >=20
> > > 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?
> > >=20
> > > Or is there something you need from the slot infrastructure in
> > > addition to the serial number exposure?
> > >=20
> > > 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().
> > >  =20
> > > > > 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.
> > > > >=20
> > > > > This also shortens the network device names generated by
> > > > > systemd/udev. The new names use slot (ens2) rather than
> > > > > PCI address (enP2p0s2).   =20
> > >=20
> > > 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?
> > >  =20
> > > > Hi Stephen,
> > > >=20
> > > > 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.
> > > >=20
> > > > I would also ask Bjorn's opinion on this since he has more
> > > > insights into the slot interface history.
> > > >=20
> > > > Thanks,
> > > > Lorenzo
> > > >    =20
> > > > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> =20
> >=20
> > Yes, this is a way to expose serial number to userspace as well as a
> > way to help udev with naming. =20
>=20
> When you say "to help" it means "to fix" or to bring it on a par
> with KVM ? This change is Hyper-V specific so I am not really against
> it, still, I am not a big fan of making a serial number look like
> a slot number to please udev either.

It provides udev with shorter more concise value that gives better result.
Still would like something persistent and repeatable but it doesn't seem
to be guaranteed by the host.

>=20
> > Today, a VF device shows up as:
> > $ lspci -n
> > 0002:00:02.0 0200: 15b3:1014 (rev 80)
> >=20
> >=20
> > And therefore is named: enP2p0s2 based on these rules:
> > See: https://github.com/systemd/systemd/blob/master/src/udev/udev-built=
in-net_id.c#L20
> >=20
> >  * Two character prefixes based on the type of interface:
> >  *   en =E2=80=94 Ethernet
> >  *   sl =E2=80=94 serial line IP (slip)
> >  *   wl =E2=80=94 wlan
> >  *   ww =E2=80=94 wwan
> >  *
> >  * Type of names:
> >=20
> >  *   o<index>[n<phys_port_name>|d<dev_port>]
> >  *                                         =E2=80=94 on-board device in=
dex number
> >  *   s<slot>[f<function>][n<phys_port_name>|d<dev_port>]
> >  *                                         =E2=80=94 hotplug slot index=
 number
> >  *   x<MAC>                                =E2=80=94 MAC address
> >  *   [P<domain>]p<bus>s<slot>[f<function>][n<phys_port_name>|d<dev_port=
>]
> >  *                                         =E2=80=94 PCI geographical l=
ocation
> >=20
> > I looked into several other options:
> >   - put serial number into the device index number
> >=20
> >=20
> > On KVM, the ACPI emulation provides PCI slot information so that is why
> > the devices are named "ens3".
> >=20
> > I looked into several other options (none of which worked well):
> >   * provide sysfs /index file but that wants to be handled by smbios
> >   * use VPD serial number but that is a GUID not an integer
> >   * full hotplug (but Hyper-V doesn't support real PCI hotplug)
> >   * new device_attribute serial, but that is not easy to hook
> >     into existing code, and udev won't know about it.
> >=20
> > Open to any ideas about how to best represent this. =20
>=20
> By trawling PCI patchwork I found this series that was not merged, maybe
> it is a starting point to implement the latter suggestion:
>=20
> https://patchwork.ozlabs.org/patch/264610/
>=20
> udev won't know about it but as I say above that's not a bug
> we are fixing either AFAICS.

Right this isn't a bug.

The PCIe Device serial number (in patchwork) has a different meaning and
can't be used. The PCIe device serial number comes from device config space
and the serial number (i.e it is a property of the board).
The value this patch is addressing is assigned by the host (similar to ACPI=
 slots).

On latest kernels (now that PCI domain is 32 bits), the SR-IOV VF device
is even more ugly (enP37727p0s2) that was one of the motivations for this p=
atch.
With the patch udev will generate ens2.