| Message ID | 1502860344-84425-1-git-send-email-chao.gao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
on wed, aug 16, 2017 at 01:12:24pm +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. AFAIK, extended functions simply remove the slot and extend the function number to [0, 255], so it seems correct to expect that the VT-d unit search should be done using the bus and extended function parameters, and assume slot is 0. Is this some kind of limitation of VT-d? > The original code tried to tell apart Extended > Function and non-Extended Function 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. > > This patch introduces a new field, pf_is_extfn, in struct > pci_dev_info, to indicate whether the physical function is an extended > function. The new field helps to generate correct BDF to search VT-d > unit. [...] > diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h > index 59b6e8a..9e76aa0 100644 > --- a/xen/include/xen/pci.h > +++ b/xen/include/xen/pci.h > @@ -40,6 +40,7 @@ > > struct pci_dev_info { > bool_t is_extfn; > + bool_t pf_is_extfn; /* Only valid for virtual function */ Can't you just re-use is_virtfn and is_extfn, and when both are true it means the pf where this vf belongs is an extended function? > bool_t is_virtfn; > struct { > u8 bus; > -- > 1.8.3.1 >
On Wed, Aug 16, 2017 at 09:17:46AM +0100, Roger Pau Monné wrote: >on wed, aug 16, 2017 at 01:12:24pm +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. > >AFAIK, extended functions simply remove the slot and extend the >function number to [0, 255], so it seems correct to expect that the >VT-d unit search should be done using the bus and extended function >parameters, and assume slot is 0. Is this some kind of limitation of >VT-d? VT-d spec makes such provision for VT-d unit search without any explaination. But I think it isn't. Whether we can find the right VT-d unit depends on DMAR. So I would rather regard it as firmware doesn't prepare entries for extended functions in DMAR. > >> The original code tried to tell apart Extended >> Function and non-Extended Function 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. >> >> This patch introduces a new field, pf_is_extfn, in struct >> pci_dev_info, to indicate whether the physical function is an extended >> function. The new field helps to generate correct BDF to search VT-d >> unit. > >[...] >> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h >> index 59b6e8a..9e76aa0 100644 >> --- a/xen/include/xen/pci.h >> +++ b/xen/include/xen/pci.h >> @@ -40,6 +40,7 @@ >> >> struct pci_dev_info { >> bool_t is_extfn; >> + bool_t pf_is_extfn; /* Only valid for virtual function */ > >Can't you just re-use is_virtfn and is_extfn, and when both are true >it means the pf where this vf belongs is an extended function? Yes. Reuse vf's is_extfn field is possible. Thanks Chao
On Wed, Aug 16, 2017 at 04:50:51PM +0800, Chao Gao wrote: > On Wed, Aug 16, 2017 at 09:17:46AM +0100, Roger Pau Monné wrote: > >on wed, aug 16, 2017 at 01:12:24pm +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. > > > >AFAIK, extended functions simply remove the slot and extend the > >function number to [0, 255], so it seems correct to expect that the > >VT-d unit search should be done using the bus and extended function > >parameters, and assume slot is 0. Is this some kind of limitation of > >VT-d? > > VT-d spec makes such provision for VT-d unit search without any > explaination. But I think it isn't. Whether we can find the right VT-d unit > depends on DMAR. So I would rather regard it as firmware doesn't prepare > entries for extended functions in DMAR. Looking again at the proposed changes in acpi_find_matched_drhd_unit, it seems fine. If the VF belongs to a PF that uses extended functions just use 0 as devfn, which is the devfn of the PF itself unless I'm mistaken. It's just that I find the commit log very hard to parse/understand. Roger.
> From: Gao, Chao > Sent: Wednesday, August 16, 2017 1:12 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 Extended > Function and non-Extended Function 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. > > This patch introduces a new field, pf_is_extfn, in struct > pci_dev_info, to indicate whether the physical function is an extended > function. The new field helps to generate correct BDF to search VT-d > unit. > > Reported-by: Crawford, Eric R <Eric.R.Crawford@intel.com> > Tested-by: Crawford, Eric R <Eric.R.Crawford@intel.com> > Signed-off-by: Chao Gao <chao.gao@intel.com> > --- > xen/drivers/passthrough/pci.c | 6 +++++- > xen/drivers/passthrough/vtd/dmar.c | 2 +- > xen/include/xen/pci.h | 1 + > 3 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index 27bdb71..8c2ba33 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -599,6 +599,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, > unsigned int slot = PCI_SLOT(devfn), func = PCI_FUNC(devfn); > const char *pdev_type; > int ret; > + bool pf_is_extfn = false; > > if (!info) > pdev_type = "device"; > @@ -609,7 +610,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, > pcidevs_lock(); > pdev = pci_get_pdev(seg, info->physfn.bus, info->physfn.devfn); > pcidevs_unlock(); > - if ( !pdev ) > + if ( pdev ) > + pf_is_extfn = pdev->info.is_extfn; besides Roger's comment, can you move above 2 lines inside lock protection? > + else > pci_add_device(seg, info->physfn.bus, info->physfn.devfn, > NULL, node); > pdev_type = "virtual function"; > @@ -707,6 +710,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, > seg, bus, slot, func, ctrl); > } > > + pdev->info.pf_is_extfn = pf_is_extfn; > check_pdev(pdev); > > ret = 0; > diff --git a/xen/drivers/passthrough/vtd/dmar.c > b/xen/drivers/passthrough/vtd/dmar.c > index 82040dd..a96558f 100644 > --- a/xen/drivers/passthrough/vtd/dmar.c > +++ b/xen/drivers/passthrough/vtd/dmar.c > @@ -219,7 +219,7 @@ struct acpi_drhd_unit > *acpi_find_matched_drhd_unit(const struct pci_dev *pdev) > else if ( pdev->info.is_virtfn ) > { > bus = pdev->info.physfn.bus; > - devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : pdev- > >info.physfn.devfn; > + devfn = pdev->info.pf_is_extfn ? 0 : pdev->info.physfn.devfn; > } > else > { > diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h > index 59b6e8a..9e76aa0 100644 > --- a/xen/include/xen/pci.h > +++ b/xen/include/xen/pci.h > @@ -40,6 +40,7 @@ > > struct pci_dev_info { > bool_t is_extfn; > + bool_t pf_is_extfn; /* Only valid for virtual function */ > bool_t is_virtfn; > struct { > u8 bus; > -- > 1.8.3.1
On Thu, Aug 17, 2017 at 03:43:07PM +0800, Tian, Kevin wrote: >> From: Gao, Chao >> Sent: Wednesday, August 16, 2017 1:12 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 Extended >> Function and non-Extended Function 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. >> >> This patch introduces a new field, pf_is_extfn, in struct >> pci_dev_info, to indicate whether the physical function is an extended >> function. The new field helps to generate correct BDF to search VT-d >> unit. >> >> Reported-by: Crawford, Eric R <Eric.R.Crawford@intel.com> >> Tested-by: Crawford, Eric R <Eric.R.Crawford@intel.com> >> Signed-off-by: Chao Gao <chao.gao@intel.com> >> --- >> xen/drivers/passthrough/pci.c | 6 +++++- >> xen/drivers/passthrough/vtd/dmar.c | 2 +- >> xen/include/xen/pci.h | 1 + >> 3 files changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c >> index 27bdb71..8c2ba33 100644 >> --- a/xen/drivers/passthrough/pci.c >> +++ b/xen/drivers/passthrough/pci.c >> @@ -599,6 +599,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, >> unsigned int slot = PCI_SLOT(devfn), func = PCI_FUNC(devfn); >> const char *pdev_type; >> int ret; >> + bool pf_is_extfn = false; >> >> if (!info) >> pdev_type = "device"; >> @@ -609,7 +610,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, >> pcidevs_lock(); >> pdev = pci_get_pdev(seg, info->physfn.bus, info->physfn.devfn); >> pcidevs_unlock(); >> - if ( !pdev ) >> + if ( pdev ) >> + pf_is_extfn = pdev->info.is_extfn; > >besides Roger's comment, can you move above 2 lines inside lock >protection? > Hi, Kevin and Roger. I sent out a new version recently. The new version adopts all your suggestions. Please review it. Thanks Chao
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index 27bdb71..8c2ba33 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -599,6 +599,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, unsigned int slot = PCI_SLOT(devfn), func = PCI_FUNC(devfn); const char *pdev_type; int ret; + bool pf_is_extfn = false; if (!info) pdev_type = "device"; @@ -609,7 +610,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, pcidevs_lock(); pdev = pci_get_pdev(seg, info->physfn.bus, info->physfn.devfn); pcidevs_unlock(); - if ( !pdev ) + if ( pdev ) + pf_is_extfn = pdev->info.is_extfn; + else pci_add_device(seg, info->physfn.bus, info->physfn.devfn, NULL, node); pdev_type = "virtual function"; @@ -707,6 +710,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, seg, bus, slot, func, ctrl); } + pdev->info.pf_is_extfn = pf_is_extfn; check_pdev(pdev); ret = 0; diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c index 82040dd..a96558f 100644 --- a/xen/drivers/passthrough/vtd/dmar.c +++ b/xen/drivers/passthrough/vtd/dmar.c @@ -219,7 +219,7 @@ struct acpi_drhd_unit *acpi_find_matched_drhd_unit(const struct pci_dev *pdev) else if ( pdev->info.is_virtfn ) { bus = pdev->info.physfn.bus; - devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : pdev->info.physfn.devfn; + devfn = pdev->info.pf_is_extfn ? 0 : pdev->info.physfn.devfn; } else { diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h index 59b6e8a..9e76aa0 100644 --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -40,6 +40,7 @@ struct pci_dev_info { bool_t is_extfn; + bool_t pf_is_extfn; /* Only valid for virtual function */ bool_t is_virtfn; struct { u8 bus;