* [PATCH v6] VT-d: fix VF of RC integrated PF matched to wrong VT-d unit
@ 2017-08-16 5:12 Chao Gao
2017-08-16 8:17 ` [patch v6] vt-d: fix vf of rc integrated pf matched to wrong vt-d unit Roger Pau Monné
2017-08-17 7:43 ` [PATCH v6] VT-d: fix VF of RC integrated PF matched to wrong VT-d unit Tian, Kevin
0 siblings, 2 replies; 6+ messages in thread
From: Chao Gao @ 2017-08-16 5:12 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Stefano Stabellini, Wei Liu, George Dunlap,
Ian Jackson, Tim Deegan, Jan Beulich, Andrew Cooper, Chao Gao,
Roger Pau Monné
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;
+ 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [patch v6] vt-d: fix vf of rc integrated pf matched to wrong vt-d unit
2017-08-16 5:12 [PATCH v6] VT-d: fix VF of RC integrated PF matched to wrong VT-d unit Chao Gao
@ 2017-08-16 8:17 ` Roger Pau Monné
2017-08-16 8:50 ` Chao Gao
2017-08-17 7:43 ` [PATCH v6] VT-d: fix VF of RC integrated PF matched to wrong VT-d unit Tian, Kevin
1 sibling, 1 reply; 6+ messages in thread
From: Roger Pau Monné @ 2017-08-16 8:17 UTC (permalink / raw)
To: Chao Gao
Cc: Kevin Tian, stefano stabellini, wei liu, george dunlap,
ian jackson, tim deegan, xen-devel, jan beulich, andrew cooper
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
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch v6] vt-d: fix vf of rc integrated pf matched to wrong vt-d unit
2017-08-16 8:17 ` [patch v6] vt-d: fix vf of rc integrated pf matched to wrong vt-d unit Roger Pau Monné
@ 2017-08-16 8:50 ` Chao Gao
2017-08-16 9:10 ` Roger Pau Monné
0 siblings, 1 reply; 6+ messages in thread
From: Chao Gao @ 2017-08-16 8:50 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Kevin Tian, stefano stabellini, wei liu, george dunlap,
ian jackson, tim deegan, xen-devel, jan beulich, andrew cooper
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch v6] vt-d: fix vf of rc integrated pf matched to wrong vt-d unit
2017-08-16 8:50 ` Chao Gao
@ 2017-08-16 9:10 ` Roger Pau Monné
0 siblings, 0 replies; 6+ messages in thread
From: Roger Pau Monné @ 2017-08-16 9:10 UTC (permalink / raw)
To: xen-devel, Kevin Tian, wei liu, tim deegan, stefano stabellini,
konrad rzeszutek wilk, ian jackson, george dunlap, andrew cooper,
jan beulich
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v6] VT-d: fix VF of RC integrated PF matched to wrong VT-d unit
2017-08-16 5:12 [PATCH v6] VT-d: fix VF of RC integrated PF matched to wrong VT-d unit Chao Gao
2017-08-16 8:17 ` [patch v6] vt-d: fix vf of rc integrated pf matched to wrong vt-d unit Roger Pau Monné
@ 2017-08-17 7:43 ` Tian, Kevin
2017-08-22 5:03 ` Chao Gao
1 sibling, 1 reply; 6+ messages in thread
From: Tian, Kevin @ 2017-08-17 7:43 UTC (permalink / raw)
To: Gao, Chao, xen-devel@lists.xen.org
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Ian Jackson,
Tim Deegan, Jan Beulich, Andrew Cooper, Roger Pau Monné
> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v6] VT-d: fix VF of RC integrated PF matched to wrong VT-d unit
2017-08-17 7:43 ` [PATCH v6] VT-d: fix VF of RC integrated PF matched to wrong VT-d unit Tian, Kevin
@ 2017-08-22 5:03 ` Chao Gao
0 siblings, 0 replies; 6+ messages in thread
From: Chao Gao @ 2017-08-22 5:03 UTC (permalink / raw)
To: Tian, Kevin, Roger Pau Monné
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Ian Jackson,
Tim Deegan, xen-devel@lists.xen.org, Jan Beulich, Andrew Cooper
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-08-22 5:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-16 5:12 [PATCH v6] VT-d: fix VF of RC integrated PF matched to wrong VT-d unit Chao Gao
2017-08-16 8:17 ` [patch v6] vt-d: fix vf of rc integrated pf matched to wrong vt-d unit Roger Pau Monné
2017-08-16 8:50 ` Chao Gao
2017-08-16 9:10 ` Roger Pau Monné
2017-08-17 7:43 ` [PATCH v6] VT-d: fix VF of RC integrated PF matched to wrong VT-d unit Tian, Kevin
2017-08-22 5:03 ` Chao Gao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).