On Tue, 2015-09-15 at 18:39 +0000, Rustad, Mark D wrote:
> > On Sep 15, 2015, at 11:19 AM, Alex Williamson <alex.williamson@xxxxxxxxxx> wrote:
> >
> > In addition to the (PCI_SLOT() != devfn) issue, I'm concerned about
> > topologies like we see on Skylake. IIRC, the integrated NIC appears at
> > something like 00:1f.6. I don't know if that specific NIC has VPD, nor
> > am I sure it really matter because another example or some future
> > version might. So we'll set the PCI_DEV_FLAGS_VPD_REF_F0 because we do
> > so for all (PCI_FUNC() != 0) Intel NICs, we'll call
> > pci_vpd_f0_dev_check(), which will error because function 0 has a
> > different class code and device ID, so we return error and if VPD exists
> > on the device, it's now inaccessible.
>
> Yes, that is exactly what would happen.
>
> > I thought there was talk about whitelisting anything on the root bus to
> > avoid strange root complex integrated devices (and perhaps avoid the
> > general case for assigned devices within a VM), but I don't see anything
> > like that here.
>
> I hadn't heard that talk, but I'm not on the PCI list and I guess I
> wasn't copied.
>
> > Perhaps instead of failing and hiding VPD we should fail, clear the
> > flag, and allow normal access. Thanks,
>
> Because the purpose of VPD is to hold information about the device, I
> would suggest that VPD should never be provided for an embedded
> network device, but rather for the device as a whole. So while there
> may well be VPD for an SOC, that VPD should not be associated with one
> of its embedded devices, but rather something more appropriate for the
> device as a whole. And attaching VPD to a whole bunch of internal
> devices would just be madness.
FRU-type information is only one of the use cases of VPD, the spec also
defines (PCI rev 3.0, 6.4):
... a mechanism for storing information such as performance and
failure data on the device being monitored.
That information could very much be function specific.
> So I understand the concern, but I don't think that it should really
> happen in real systems. I did think about this case when I was working
> on the patches. A networking device should really only have VPD when
> it is its own physical device, such as a NIC.
>
When I was looking at whether we should provide VPD access of an
assigned device at all, I ran across this interesting statement in the
PCI spec (rev 3.0, I.3.1.1):
CP Extended Capability
This field allows a new capability to be identified in the VPD
area. Since dynamic control/status cannot be placed in VPD, the
data for this field identifies where, in the device’s memory or
I/O address space, the control/status registers for the
capability can be found. Location of the control/status
registers is identified by providing the index (a value between
0 and 5) of the Base Address register that defines the address
range that contains the registers, and the offset within that
Base Address register range where the control/status registers
reside. The data area for this field is four bytes long. The
first byte contains the ID of the extended capability. The
second byte contains the index (zero based) of the Base Address
register used. The next two bytes contain the offset (in
little-endian order) within that address range where the
control/status registers defined for that capability reside.
Again, this sounds like function specific data, and both here and above,
blocking access to VPD could affect the functionality of drivers. It
may be the case that Intel would find this use to be madness, but
there's no PCI spec requirement that separate functions are in any way
similar and we're looking at an interface that may be used by non-Intel
devices as well. Thanks,
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
