* [PATCH v10] VT-d: use correct BDF for VF to search VT-d unit
@ 2017-08-28 2:42 Chao Gao
2017-08-28 5:56 ` Tian, Kevin
2017-08-28 8:16 ` Jan Beulich
0 siblings, 2 replies; 5+ messages in thread
From: Chao Gao @ 2017-08-28 2:42 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,
Crawford Eric R, Roger Pau Monné
When SR-IOV is enabled, 'Virtual Functions' of a 'Physical Function'
are under the scope of the same VT-d unit as the 'Physical Function'.
A 'Physical Function' can be a 'Traditional Function' or an ARI
'Extended Function'. And furthermore, 'Extended Functions' on an
endpoint are under the scope of the same VT-d unit as the 'Traditional
Functions' on the endpoint. To search VT-d unit for a VF, if its PF
isn't an extended function, the BDF of PF should be used. Otherwise
the BDF of a traditional function in the same device with the PF
should be used.
Current code uses PCI_SLOT() to recognize an ARI 'Extended Funcion'.
But it is conceptually wrong w/o checking whether PF is an extended
function and would lead to match VFs of a RC integrated PF to a wrong
VT-d unit.
This patch overrides VF 'is_extfn' field and uses this field to
indicate whether the PF of this VF is an extended function. The field
helps to use correct BDF to search VT-d unit.
Reported-by: Crawford, Eric R <Eric.R.Crawford@intel.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
v10:
- move setting vf's is_extfn closer to the place where we set other fields.
- reverse the conditional expression in acpi_find_matched_drhd_unit()
v9:
- check 'is_virtfn' first in pci_add_device() to avoid potential error if
linux side sets VF's 'is_extfn'
- comments changes to make it clear that we use VF's 'is_extfn' intentionally
otherwise the patch seems like a workaround.
v8:
- use "conceptually wrong", instead of "a corner case" in commit message
- check 'is_virtfn' first in acpi_find_matched_drhd_unit()
v7:
- Drop Eric's tested-by
- Change commit message to be clearer
- Re-use VF's is_extfn field
- access PF's is_extfn field in locked area
---
xen/drivers/passthrough/pci.c | 19 +++++++++++++++----
xen/drivers/passthrough/vtd/dmar.c | 12 ++++++------
xen/include/xen/pci.h | 4 ++++
3 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 27bdb71..187a9e7 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -599,21 +599,24 @@ 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)
+ if ( !info )
pdev_type = "device";
- else if (info->is_extfn)
- pdev_type = "extended function";
- else if (info->is_virtfn)
+ else if ( info->is_virtfn )
{
pcidevs_lock();
pdev = pci_get_pdev(seg, info->physfn.bus, info->physfn.devfn);
+ if ( pdev )
+ pf_is_extfn = pdev->info.is_extfn;
pcidevs_unlock();
if ( !pdev )
pci_add_device(seg, info->physfn.bus, info->physfn.devfn,
NULL, node);
pdev_type = "virtual function";
}
+ else if ( info->is_extfn )
+ pdev_type = "extended function";
else
{
info = NULL;
@@ -637,7 +640,15 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
pdev->node = node;
if ( info )
+ {
pdev->info = *info;
+ /*
+ * VF's 'is_extfn' field is used to indicate whether its PF is an
+ * extended function.
+ */
+ if ( pdev->info.is_virtfn )
+ pdev->info.is_extfn = pf_is_extfn;
+ }
else if ( !pdev->vf_rlen[0] )
{
unsigned int pos = pci_find_ext_capability(seg, bus, devfn,
diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index 82040dd..9676471 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -211,15 +211,15 @@ struct acpi_drhd_unit *acpi_find_matched_drhd_unit(const struct pci_dev *pdev)
if ( pdev == NULL )
return NULL;
- if ( pdev->info.is_extfn )
+ if ( pdev->info.is_virtfn )
{
- bus = pdev->bus;
- devfn = 0;
+ bus = pdev->info.physfn.bus;
+ devfn = (!pdev->info.is_extfn) ? pdev->info.physfn.devfn : 0;
}
- else if ( pdev->info.is_virtfn )
+ else if ( pdev->info.is_extfn )
{
- bus = pdev->info.physfn.bus;
- devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : pdev->info.physfn.devfn;
+ bus = pdev->bus;
+ devfn = 0;
}
else
{
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 59b6e8a..da1bd22 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -39,6 +39,10 @@
#define PCI_SBDF3(s,b,df) ((((s) & 0xffff) << 16) | PCI_BDF2(b, df))
struct pci_dev_info {
+ /*
+ * VF's 'is_extfn' field is used to indicate whether its PF is an extended
+ * function.
+ */
bool_t is_extfn;
bool_t is_virtfn;
struct {
--
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] 5+ messages in thread
* Re: [PATCH v10] VT-d: use correct BDF for VF to search VT-d unit
2017-08-28 2:42 [PATCH v10] VT-d: use correct BDF for VF to search VT-d unit Chao Gao
@ 2017-08-28 5:56 ` Tian, Kevin
2017-08-28 8:16 ` Jan Beulich
1 sibling, 0 replies; 5+ messages in thread
From: Tian, Kevin @ 2017-08-28 5:56 UTC (permalink / raw)
To: Gao, Chao, xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Ian Jackson,
Tim Deegan, Jan Beulich, Andrew Cooper, Crawford, Eric R,
Roger Pau Monné
> From: Gao, Chao
> Sent: Monday, August 28, 2017 10:42 AM
>
> When SR-IOV is enabled, 'Virtual Functions' of a 'Physical Function'
> are under the scope of the same VT-d unit as the 'Physical Function'.
> A 'Physical Function' can be a 'Traditional Function' or an ARI
> 'Extended Function'. And furthermore, 'Extended Functions' on an
> endpoint are under the scope of the same VT-d unit as the 'Traditional
> Functions' on the endpoint. To search VT-d unit for a VF, if its PF
> isn't an extended function, the BDF of PF should be used. Otherwise
> the BDF of a traditional function in the same device with the PF
> should be used.
>
> Current code uses PCI_SLOT() to recognize an ARI 'Extended Funcion'.
> But it is conceptually wrong w/o checking whether PF is an extended
> function and would lead to match VFs of a RC integrated PF to a wrong
> VT-d unit.
>
> This patch overrides VF 'is_extfn' field and uses this field to
> indicate whether the PF of this VF is an extended function. The field
> helps to use correct BDF to search VT-d unit.
>
> Reported-by: Crawford, Eric R <Eric.R.Crawford@intel.com>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v10] VT-d: use correct BDF for VF to search VT-d unit
2017-08-28 8:16 ` Jan Beulich
@ 2017-08-28 7:26 ` Chao Gao
2017-08-31 0:51 ` Crawford, Eric R
0 siblings, 1 reply; 5+ messages in thread
From: Chao Gao @ 2017-08-28 7:26 UTC (permalink / raw)
Cc: Tim Deegan, Kevin Tian, Stefano Stabellini, Wei Liu,
George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
Jan Beulich, Crawford Eric R, Roger Pau Monné
On Mon, Aug 28, 2017 at 02:16:18AM -0600, Jan Beulich wrote:
>>>> On 28.08.17 at 04:42, <chao.gao@intel.com> wrote:
>> When SR-IOV is enabled, 'Virtual Functions' of a 'Physical Function'
>> are under the scope of the same VT-d unit as the 'Physical Function'.
>> A 'Physical Function' can be a 'Traditional Function' or an ARI
>> 'Extended Function'. And furthermore, 'Extended Functions' on an
>> endpoint are under the scope of the same VT-d unit as the 'Traditional
>> Functions' on the endpoint. To search VT-d unit for a VF, if its PF
>> isn't an extended function, the BDF of PF should be used. Otherwise
>> the BDF of a traditional function in the same device with the PF
>> should be used.
>>
>> Current code uses PCI_SLOT() to recognize an ARI 'Extended Funcion'.
>> But it is conceptually wrong w/o checking whether PF is an extended
>> function and would lead to match VFs of a RC integrated PF to a wrong
>> VT-d unit.
>>
>> This patch overrides VF 'is_extfn' field and uses this field to
>> indicate whether the PF of this VF is an extended function. The field
>> helps to use correct BDF to search VT-d unit.
>>
>> Reported-by: Crawford, Eric R <Eric.R.Crawford@intel.com>
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>
>Acked-by: Jan Beulich <jbeulich@suse.com>
>albeit ...
>
>> --- a/xen/drivers/passthrough/vtd/dmar.c
>> +++ b/xen/drivers/passthrough/vtd/dmar.c
>> @@ -211,15 +211,15 @@ struct acpi_drhd_unit *acpi_find_matched_drhd_unit(const struct pci_dev *pdev)
>> if ( pdev == NULL )
>> return NULL;
>>
>> - if ( pdev->info.is_extfn )
>> + if ( pdev->info.is_virtfn )
>> {
>> - bus = pdev->bus;
>> - devfn = 0;
>> + bus = pdev->info.physfn.bus;
>> + devfn = (!pdev->info.is_extfn) ? pdev->info.physfn.devfn : 0;
>
>... if I end up committing this and if I don't forget, I'll likely take the
>liberty to remove the pointless parentheses here.
>
Hi, Eric.
Could you test this patch again and give your Tested-by if it fixes the
problem you reported?
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v10] VT-d: use correct BDF for VF to search VT-d unit
2017-08-28 2:42 [PATCH v10] VT-d: use correct BDF for VF to search VT-d unit Chao Gao
2017-08-28 5:56 ` Tian, Kevin
@ 2017-08-28 8:16 ` Jan Beulich
2017-08-28 7:26 ` Chao Gao
1 sibling, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2017-08-28 8:16 UTC (permalink / raw)
To: Chao Gao
Cc: Tim Deegan, Kevin Tian, Stefano Stabellini, Wei Liu,
George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
Crawford Eric R, Roger Pau Monné
>>> On 28.08.17 at 04:42, <chao.gao@intel.com> wrote:
> When SR-IOV is enabled, 'Virtual Functions' of a 'Physical Function'
> are under the scope of the same VT-d unit as the 'Physical Function'.
> A 'Physical Function' can be a 'Traditional Function' or an ARI
> 'Extended Function'. And furthermore, 'Extended Functions' on an
> endpoint are under the scope of the same VT-d unit as the 'Traditional
> Functions' on the endpoint. To search VT-d unit for a VF, if its PF
> isn't an extended function, the BDF of PF should be used. Otherwise
> the BDF of a traditional function in the same device with the PF
> should be used.
>
> Current code uses PCI_SLOT() to recognize an ARI 'Extended Funcion'.
> But it is conceptually wrong w/o checking whether PF is an extended
> function and would lead to match VFs of a RC integrated PF to a wrong
> VT-d unit.
>
> This patch overrides VF 'is_extfn' field and uses this field to
> indicate whether the PF of this VF is an extended function. The field
> helps to use correct BDF to search VT-d unit.
>
> Reported-by: Crawford, Eric R <Eric.R.Crawford@intel.com>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
albeit ...
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -211,15 +211,15 @@ struct acpi_drhd_unit *acpi_find_matched_drhd_unit(const struct pci_dev *pdev)
> if ( pdev == NULL )
> return NULL;
>
> - if ( pdev->info.is_extfn )
> + if ( pdev->info.is_virtfn )
> {
> - bus = pdev->bus;
> - devfn = 0;
> + bus = pdev->info.physfn.bus;
> + devfn = (!pdev->info.is_extfn) ? pdev->info.physfn.devfn : 0;
... if I end up committing this and if I don't forget, I'll likely take the
liberty to remove the pointless parentheses here.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v10] VT-d: use correct BDF for VF to search VT-d unit
2017-08-28 7:26 ` Chao Gao
@ 2017-08-31 0:51 ` Crawford, Eric R
0 siblings, 0 replies; 5+ messages in thread
From: Crawford, Eric R @ 2017-08-31 0:51 UTC (permalink / raw)
To: Gao, Chao
Cc: Tim Deegan, Tian, Kevin, Stefano Stabellini, Wei Liu,
George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
Jan Beulich, Roger Pau Monné
Sorry for the delay, it looks like this patch resolves the issue.
Tested-by: Crawford, Eric R <Eric.R.Crawford@intel.com>
-Eric
-----Original Message-----
From: Gao, Chao
Sent: Monday, August 28, 2017 12:27 AM
To: Crawford, Eric R <eric.r.crawford@intel.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@eu.citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Crawford, Eric R <eric.r.crawford@intel.com>; Tian, Kevin <kevin.tian@intel.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-devel@lists.xen.org; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim Deegan <tim@xen.org>; Jan Beulich <JBeulich@suse.com>
Subject: Re: [PATCH v10] VT-d: use correct BDF for VF to search VT-d unit
On Mon, Aug 28, 2017 at 02:16:18AM -0600, Jan Beulich wrote:
>>>> On 28.08.17 at 04:42, <chao.gao@intel.com> wrote:
>> When SR-IOV is enabled, 'Virtual Functions' of a 'Physical Function'
>> are under the scope of the same VT-d unit as the 'Physical Function'.
>> A 'Physical Function' can be a 'Traditional Function' or an ARI
>> 'Extended Function'. And furthermore, 'Extended Functions' on an
>> endpoint are under the scope of the same VT-d unit as the
>> 'Traditional Functions' on the endpoint. To search VT-d unit for a
>> VF, if its PF isn't an extended function, the BDF of PF should be
>> used. Otherwise the BDF of a traditional function in the same device
>> with the PF should be used.
>>
>> Current code uses PCI_SLOT() to recognize an ARI 'Extended Funcion'.
>> But it is conceptually wrong w/o checking whether PF is an extended
>> function and would lead to match VFs of a RC integrated PF to a wrong
>> VT-d unit.
>>
>> This patch overrides VF 'is_extfn' field and uses this field to
>> indicate whether the PF of this VF is an extended function. The field
>> helps to use correct BDF to search VT-d unit.
>>
>> Reported-by: Crawford, Eric R <Eric.R.Crawford@intel.com>
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>
>Acked-by: Jan Beulich <jbeulich@suse.com> albeit ...
>
>> --- a/xen/drivers/passthrough/vtd/dmar.c
>> +++ b/xen/drivers/passthrough/vtd/dmar.c
>> @@ -211,15 +211,15 @@ struct acpi_drhd_unit *acpi_find_matched_drhd_unit(const struct pci_dev *pdev)
>> if ( pdev == NULL )
>> return NULL;
>>
>> - if ( pdev->info.is_extfn )
>> + if ( pdev->info.is_virtfn )
>> {
>> - bus = pdev->bus;
>> - devfn = 0;
>> + bus = pdev->info.physfn.bus;
>> + devfn = (!pdev->info.is_extfn) ? pdev->info.physfn.devfn :
>> + 0;
>
>... if I end up committing this and if I don't forget, I'll likely take
>the liberty to remove the pointless parentheses here.
>
Hi, Eric.
Could you test this patch again and give your Tested-by if it fixes the problem you reported?
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-08-31 0:51 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-28 2:42 [PATCH v10] VT-d: use correct BDF for VF to search VT-d unit Chao Gao
2017-08-28 5:56 ` Tian, Kevin
2017-08-28 8:16 ` Jan Beulich
2017-08-28 7:26 ` Chao Gao
2017-08-31 0:51 ` Crawford, Eric R
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.