Re: [PATCH v3] event/dlb2: fix disable PASID for kernel 6.2

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Nov 01, 2023 at 12:12:02AM +0530, Jerin Jacob wrote:
> On Tue, Oct 31, 2023 at 10:45 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Tue, Oct 31, 2023 at 10:36:04PM +0530, Jerin Jacob wrote:
> > > On Tue, Oct 31, 2023 at 8:43 PM Sevincer, Abdullah
> > > <abdullah.sevincer@intel.com> wrote:
> > > >
> > > >
> > > > > +This patch can be splited as two, +1) Generic PCIe function to
> > > > > enable/disable PASID +2) Call generic function to disable PASID
> > > > > in drivers/event/dlb2/. Also mention which Linux kernel commit is
> > > > > introducing this issue in the git commit log.
> > > >
> > > > Hi Jerrin, I think I need to provide more information here, then we
> > > > can decide which way we will go would be good for now. I agree to
> > > > having 2 functions in pci common code to enable/disable PASID, but
> > > > we need to have hardcoded PASID cap offset inside these functions
> > > > as well since PASID capability is not exposed to user. Hence, to be
> > > > more specific main reason to have hardcoded PASID is,
> > > > rte_pci_find_ext_capability() function to retrieve the offset
> > > > returns '0' since PASID is not exposed to user yet.
> > > >
> > > > We can see this is vfio_pci_config.c in kernel code where PASID is
> > > > not exposed to user.  [PCI_EXT_CAP_ID_PASID]  =       0,      /*
> > > > not yet */
> > > >
> > > > So if it is okay to go with hardcoded offset now in these functions
> > > > I will move the implementation to pci_common file.
> > >
> > > I would suggest, add argument option to API whether to probe the
> > > capability or not? - 0 means probe and- non zero means specific PASID
> > > cap offset till Linux VFIO is exposing it.
> >
> > That doesn't seem particularly useful to me. The calling-API in the
> > DPDK PMD (assuming it's PMD who use this), is no more likely to know
> > whether probing will work. Therefore, I think we just hard-code the
> > offset for now.
> 
> I think, there are three things here 1) Whether to have common API for
> dealing with generic function like enabling PASID or not? - I think, we
> are in agreement to have common public function(Implementation could be
> hard-coded or probe) 2) Since it is public PCIe API, I thought of adding
> probing in API as it is just LINUX limitation now. No strong opinion on
> inclusion of probe in on this as Linux is main EAL which supports PCIe
> now.  3) Since it is PCIe capability, In my understanding the offset will
> change based on the number of capabilities available in PCIe config space
> for a given device. _if so_, an additional argument for the offset needs
> to be passed from PMD to common PCI API(I.e it can not be hard-coded in
> common PCI code)
> 

Given these constraints and how late we are in the release cycle, I
therefore suggest we take the driver-specific bug fix for now. DLB seems
the only driver affected right now (and I can confirm the issue having
encountered it myself when running DPDK on Ubuntu 23.04).

I think a general function is a good thing to have, but it seems that such
a general function should wait until we really can make it generic in the
future.

Just my 2c. at this point.

/Bruce