Re: [PATCH v1 1/5] PCI/IOV: Add support to verify PF/VF spec compliance

From: "Raj, Ashok" <ashok.raj@intel.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: sathyanarayanan kuppuswamy 
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	corbet@lwn.net, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
	Keith Busch <keith.busch@intel.com>,
	alex.williamson@redhat.com, Ashok Raj <ashok.raj@intel.com>
Subject: Re: [PATCH v1 1/5] PCI/IOV: Add support to verify PF/VF spec compliance
Date: Tue, 23 Apr 2019 15:57:43 -0700	[thread overview]
Message-ID: <20190423225743.GA19171@otc-nc-03> (raw)
In-Reply-To: <20190423221134.GI14616@google.com>

On Tue, Apr 23, 2019 at 05:11:34PM -0500, Bjorn Helgaas wrote:
> On Fri, Apr 19, 2019 at 11:41:31AM -0700, sathyanarayanan kuppuswamy wrote:
> > On 4/19/19 10:37 AM, Raj, Ashok wrote:
> > > On Fri, Apr 19, 2019 at 08:59:18AM -0500, Bjorn Helgaas wrote:
> > > > On Wed, Mar 06, 2019 at 02:11:14PM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > > > 
> > > > > PF/VF implementation must comply with PCIe specification as
> > > > > defined in r4.0, sec 9.3.4, 9.3.5, 9.3.6 and 9.3.7. And if
> > > > > it does not comply, return error and skip PF/VF device
> > > > > creation.
> > > > As far as I can tell, this patch basically looks for excuses not to
> > > > enable SR-IOV.  Does this actually solve a problem?  Are there
> > > > non-compliant devices out there that blow up if we enable SR-IOV?
> > > We ran into one case with a future product for INTx value on VF's. We do have
> > > a quirk for it upstream now. We thought it might be good to just run through
> > > some of the basic requirements and see if any device violates it, that would allow
> > > us to catch them early.
> > > 
> > > We are looking into other things like PASID and End-2-End prefix capability for instance.
> > > There is another subtle thing like number of eetlp_prefix_supported. Which i don't
> > > pci core checks today, and we might need to do that to be compliant or find devices
> > > that break that contract.
> > > 
> > > Real intend is to be find such breaks early, it also helps the product guys :-)
> > > 
> > > With upcoming vIOMMU effort, we need to ensure that capabilities are exported properly
> > > depending on if its a SRIOV-PF/VF or a Scalable device.
> > > 
> > > Cheers,
> > > Ashok
> > 
> > I agree with Ashok's comments. It helps us find SRIOV related features
> > enable/disable issues easily.
> > 
> > Also, there is some of level of spec compliance checks in enabling
> > PASID/PRI/ATS already in our existing code.
> > 
> > I am just grouping them together in one place and expanded it for other IOV
> > related features.
> 
> My $0.02:
> 
>   - If there's a problem that actually prevents Linux from using a feature,
>     check for the problem close to the point where we use the feature.
> 
>   - If you want to check for spec compliance, most of that can be done more
>     easily in user-space by examining lspci output.

Certainly we could turn this into a user space tool, but its hard to enforce to find
problems early. Being part of the kernel there is no free pass for not being spec
compliant and you find problems early during bringup.

I'll think about it more and see if we could move these to places before use
as you suggest rather than a big fat catchall as it currently is.

Cheers,
Ashok

> 
> Bjorn