| Message ID | 1499327857-68032-1-git-send-email-chao.gao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> From: Gao, Chao > Sent: Thursday, July 6, 2017 3:58 PM > > The problem is for a VF of RC integrated PF (e.g. PF's BDF is 00:02.0), > we would wrongly use 00:00.0 to search VT-d unit. > > If a PF is an extended function, the BDF of a traditional function within the > same device should be used to search VT-d unit. Otherwise, the real BDF of > PF > should be used. According PCI-e spec, an extended function is a function > within an ARI device and Function Number is greater than 7. The original > code > tried to tell apart them through checking PCI_SLOT(), missing counterpart of > pci_ari_enabled() (this function exists in linux kernel) compared to linux > kernel. Without checking whether ARI is enabled, it incurs a RC integrated PF > with PCI_SLOT() >0 is wrongly classified to an extended function. Note that a > RC integrated function isn't within an ARI device and thus cannot be > extended > function and in this case the real BDF should be used. > > Considering 'is_extfn' field of struct pci_dev has been passed down from > Domain0 to indicate whether the function is an extended function, this patch > just looks up the 'is_extfn' field of PF's struct pci_dev and set 'devfn' to 0 > when 'is_extfn' is true. > > Reported-by: Crawford, Eric R <Eric.R.Crawford@intel.com> > Signed-off-by: Chao Gao <chao.gao@intel.com> Acked-by: Kevin Tian <kevin.tian@intel.com>
On Thu, Jul 06, 2017 at 03:57:37PM +0800, Chao Gao wrote: > The problem is for a VF of RC integrated PF (e.g. PF's BDF is 00:02.0), > we would wrongly use 00:00.0 to search VT-d unit. > > If a PF is an extended function, the BDF of a traditional function within the > same device should be used to search VT-d unit. Otherwise, the real BDF of PF > should be used. According PCI-e spec, an extended function is a function > within an ARI device and Function Number is greater than 7. The original code > tried to tell apart them through checking PCI_SLOT(), missing counterpart of > pci_ari_enabled() (this function exists in linux kernel) compared to linux > kernel. Without checking whether ARI is enabled, it incurs a RC integrated PF > with PCI_SLOT() >0 is wrongly classified to an extended function. Note that a > RC integrated function isn't within an ARI device and thus cannot be extended > function and in this case the real BDF should be used. > > Considering 'is_extfn' field of struct pci_dev has been passed down from > Domain0 to indicate whether the function is an extended function, this patch > just looks up the 'is_extfn' field of PF's struct pci_dev and set 'devfn' to 0 > when 'is_extfn' is true. > > Reported-by: Crawford, Eric R <Eric.R.Crawford@intel.com> > Signed-off-by: Chao Gao <chao.gao@intel.com> > --- > v5: > - Commit description change. > v4: > - access pf's struct pci_pdev between pcidevs_lock() and pcidevs_unlock() > > --- > xen/drivers/passthrough/vtd/dmar.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c > index 82040dd..8724f0a 100644 > --- a/xen/drivers/passthrough/vtd/dmar.c > +++ b/xen/drivers/passthrough/vtd/dmar.c > @@ -218,8 +218,17 @@ struct acpi_drhd_unit *acpi_find_matched_drhd_unit(const struct pci_dev *pdev) > } > else if ( pdev->info.is_virtfn ) > { > + struct pci_dev *physfn; > + > bus = pdev->info.physfn.bus; > - devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : pdev->info.physfn.devfn; > + /* > + * Use 0 as 'devfn' to search VT-d unit when the physical function > + * is an Extended Function. > + */ > + pcidevs_lock(); > + physfn = pci_get_pdev(pdev->seg, bus, pdev->info.physfn.devfn); > + devfn = (physfn && physfn->info.is_extfn) ? 0 : pdev->info.physfn.devfn; > + pcidevs_unlock(); Just as a note (not that I intend you to fix this), but AFAICT this function should be called holding the pcidevs lock, or else there's the risk that the pdev argument is freed while poking at it. Roger.
>>> On 07.07.17 at 10:13, <roger.pau@citrix.com> wrote: > On Thu, Jul 06, 2017 at 03:57:37PM +0800, Chao Gao wrote: >> --- a/xen/drivers/passthrough/vtd/dmar.c >> +++ b/xen/drivers/passthrough/vtd/dmar.c >> @@ -218,8 +218,17 @@ struct acpi_drhd_unit *acpi_find_matched_drhd_unit(const struct pci_dev *pdev) >> } >> else if ( pdev->info.is_virtfn ) >> { >> + struct pci_dev *physfn; >> + >> bus = pdev->info.physfn.bus; >> - devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : pdev->info.physfn.devfn; >> + /* >> + * Use 0 as 'devfn' to search VT-d unit when the physical function >> + * is an Extended Function. >> + */ >> + pcidevs_lock(); >> + physfn = pci_get_pdev(pdev->seg, bus, pdev->info.physfn.devfn); >> + devfn = (physfn && physfn->info.is_extfn) ? 0 : pdev->info.physfn.devfn; >> + pcidevs_unlock(); > > Just as a note (not that I intend you to fix this), but AFAICT this > function should be called holding the pcidevs lock, or else there's > the risk that the pdev argument is freed while poking at it. As pointed out in discussion on (I think) one of your recent patch series, it is well known that we don't take that lock in all the places we should, and we really should ref-count struct pci_dev instances. I don't think dealing with the issue in individual places would be very useful - if anything, we'd have to audit the entire code base. Jan
diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c index 82040dd..8724f0a 100644 --- a/xen/drivers/passthrough/vtd/dmar.c +++ b/xen/drivers/passthrough/vtd/dmar.c @@ -218,8 +218,17 @@ struct acpi_drhd_unit *acpi_find_matched_drhd_unit(const struct pci_dev *pdev) } else if ( pdev->info.is_virtfn ) { + struct pci_dev *physfn; + bus = pdev->info.physfn.bus; - devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : pdev->info.physfn.devfn; + /* + * Use 0 as 'devfn' to search VT-d unit when the physical function + * is an Extended Function. + */ + pcidevs_lock(); + physfn = pci_get_pdev(pdev->seg, bus, pdev->info.physfn.devfn); + devfn = (physfn && physfn->info.is_extfn) ? 0 : pdev->info.physfn.devfn; + pcidevs_unlock(); } else {
The problem is for a VF of RC integrated PF (e.g. PF's BDF is 00:02.0), we would wrongly use 00:00.0 to search VT-d unit. If a PF is an extended function, the BDF of a traditional function within the same device should be used to search VT-d unit. Otherwise, the real BDF of PF should be used. According PCI-e spec, an extended function is a function within an ARI device and Function Number is greater than 7. The original code tried to tell apart them through checking PCI_SLOT(), missing counterpart of pci_ari_enabled() (this function exists in linux kernel) compared to linux kernel. Without checking whether ARI is enabled, it incurs a RC integrated PF with PCI_SLOT() >0 is wrongly classified to an extended function. Note that a RC integrated function isn't within an ARI device and thus cannot be extended function and in this case the real BDF should be used. Considering 'is_extfn' field of struct pci_dev has been passed down from Domain0 to indicate whether the function is an extended function, this patch just looks up the 'is_extfn' field of PF's struct pci_dev and set 'devfn' to 0 when 'is_extfn' is true. Reported-by: Crawford, Eric R <Eric.R.Crawford@intel.com> Signed-off-by: Chao Gao <chao.gao@intel.com> --- v5: - Commit description change. v4: - access pf's struct pci_pdev between pcidevs_lock() and pcidevs_unlock() --- xen/drivers/passthrough/vtd/dmar.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)