[PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Chao Gao]

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, the BDF of PF or the BDF of a traditional function may be used. And
it depends on whether the PF is an extended function or not.

Current code uses PCI_SLOT() to recognize an ARI 'Extended Funcion'. But it
is problematic for a corner case (a RC endpoint with SRIOV capability
and has its own VT-d unit), leading to matching to a wrong VT-d unit.

This patch reuses 'is_extfn' field in VF's struct pci_dev_info 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>
---
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      | 6 ++++++
 xen/drivers/passthrough/vtd/dmar.c | 2 +-
 xen/include/xen/pci.h              | 4 ++++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 27bdb71..2a91320 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";
@@ -608,6 +609,8 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
     {
         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,
@@ -707,6 +710,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
                    seg, bus, slot, func, ctrl);
     }
 
+    /* VF's 'is_extfn' is used to indicate whether PF is an extended function */
+    if ( pdev->info.is_virtfn )
+        pdev->info.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..83ce5d4 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.is_extfn ? 0 : pdev->info.physfn.devfn;
     }
     else
     {
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 59b6e8a..3b0da66 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 {
+    /*
+     * When 'is_virtfn' is set, 'is_extfn' is re-used to indicate whether
+     * the PF of this VF 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

Re: [PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Roger Pau Monné]

On Tue, Aug 22, 2017 at 05:52:04AM +0800, Chao Gao 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, the BDF of PF or the BDF of a traditional function may be used. And
> it depends on whether the PF is an extended function or not.
> 
> Current code uses PCI_SLOT() to recognize an ARI 'Extended Funcion'. But it
> is problematic for a corner case (a RC endpoint with SRIOV capability
> and has its own VT-d unit), leading to matching to a wrong VT-d unit.
> 
> This patch reuses 'is_extfn' field in VF's struct pci_dev_info 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>

This looks fine to me:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Given the issues we had before with this commit, could we please have
a Tested-by by someone? I saw that you dropped Eric's, and I would
like to have it again.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Chao Gao]

On Tue, Aug 22, 2017 at 08:29:58AM +0100, Roger Pau Monné wrote:
>On Tue, Aug 22, 2017 at 05:52:04AM +0800, Chao Gao 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, the BDF of PF or the BDF of a traditional function may be used. And
>> it depends on whether the PF is an extended function or not.
>> 
>> Current code uses PCI_SLOT() to recognize an ARI 'Extended Funcion'. But it
>> is problematic for a corner case (a RC endpoint with SRIOV capability
>> and has its own VT-d unit), leading to matching to a wrong VT-d unit.
>> 
>> This patch reuses 'is_extfn' field in VF's struct pci_dev_info 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>
>
>This looks fine to me:
>
>Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>

Thank you, Roger.

>Given the issues we had before with this commit, could we please have
>a Tested-by by someone? I saw that you dropped Eric's, and I would
>like to have it again.

Hi, Eric.

Could you test this patch again and give this patch your Tested-by if it
passes your test?

Thanks
Chao


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Jan Beulich]

>>> On 21.08.17 at 23:52, <chao.gao@intel.com> wrote:
> --- 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 {
> +    /*
> +     * When 'is_virtfn' is set, 'is_extfn' is re-used to indicate whether
> +     * the PF of this VF is an extended function.
> +     */

I'd be inclined to extend the comment by appending ", as a VF itself
can never be an extended function." Is that correct? If so, would
you agree this being added while committing (once the requested
Tested-by is in place)? In that case
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Chao Gao]

On Tue, Aug 22, 2017 at 06:43:49AM -0600, Jan Beulich wrote:
>>>> On 21.08.17 at 23:52, <chao.gao@intel.com> wrote:
>> --- 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 {
>> +    /*
>> +     * When 'is_virtfn' is set, 'is_extfn' is re-used to indicate whether
>> +     * the PF of this VF is an extended function.
>> +     */
>
>I'd be inclined to extend the comment by appending ", as a VF itself
>can never be an extended function." Is that correct? If so, would

Hi, Jan and Roger.

Strictly speaking, the VF can be an extended function. The definition is
within ARI device (in this kind of device, device field is treated as an
extension of function number) and function number is greater than 7. But
this field isn't used as we don't care about whether a VF is or not an
extended function (at least at present).

Eric reviewed this patch and told me we may match
'if ( pdev->info.is_extfn )' in acpi_find_matched_drhd_unit.
So we may introduce a new field like what I do in v6 or check
'pdev->info.is_virtfn' first in acpi_find_matched_drhd_unit (maybe other
places we check pdev->info.is_extfn).

Which one do you prefer?

Thanks
Chao

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Roger Pau Monné]

On Wed, Aug 23, 2017 at 09:05:14AM +0800, Chao Gao wrote:
> On Tue, Aug 22, 2017 at 06:43:49AM -0600, Jan Beulich wrote:
> >>>> On 21.08.17 at 23:52, <chao.gao@intel.com> wrote:
> >> --- 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 {
> >> +    /*
> >> +     * When 'is_virtfn' is set, 'is_extfn' is re-used to indicate whether
> >> +     * the PF of this VF is an extended function.
> >> +     */
> >
> >I'd be inclined to extend the comment by appending ", as a VF itself
> >can never be an extended function." Is that correct? If so, would
> 
> Hi, Jan and Roger.
> 
> Strictly speaking, the VF can be an extended function. The definition is
> within ARI device (in this kind of device, device field is treated as an
> extension of function number) and function number is greater than 7. But
> this field isn't used as we don't care about whether a VF is or not an
> extended function (at least at present).
> 
> Eric reviewed this patch and told me we may match
> 'if ( pdev->info.is_extfn )' in acpi_find_matched_drhd_unit.
> So we may introduce a new field like what I do in v6 or check
> 'pdev->info.is_virtfn' first in acpi_find_matched_drhd_unit (maybe other
> places we check pdev->info.is_extfn).
> 
> Which one do you prefer?

Looking at this again I'm not sure why you need any modifications to
acpi_find_matched_drhd_unit. If the virtual function is an extended
function pdev->bus should be equal to pdev->info.physfn.bus, in which
case the already existing is_extfn check will already DTRT?

Ie: an extended VF should always have the same bus as the PF it
belongs to, unless I'm missing something.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Jan Beulich]

>>> On 23.08.17 at 09:16, <roger.pau@citrix.com> wrote:
> On Wed, Aug 23, 2017 at 09:05:14AM +0800, Chao Gao wrote:
>> On Tue, Aug 22, 2017 at 06:43:49AM -0600, Jan Beulich wrote:
>> >>>> On 21.08.17 at 23:52, <chao.gao@intel.com> wrote:
>> >> --- 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 {
>> >> +    /*
>> >> +     * When 'is_virtfn' is set, 'is_extfn' is re-used to indicate whether
>> >> +     * the PF of this VF is an extended function.
>> >> +     */
>> >
>> >I'd be inclined to extend the comment by appending ", as a VF itself
>> >can never be an extended function." Is that correct? If so, would
>> 
>> Hi, Jan and Roger.
>> 
>> Strictly speaking, the VF can be an extended function. The definition is
>> within ARI device (in this kind of device, device field is treated as an
>> extension of function number) and function number is greater than 7. But
>> this field isn't used as we don't care about whether a VF is or not an
>> extended function (at least at present).
>> 
>> Eric reviewed this patch and told me we may match
>> 'if ( pdev->info.is_extfn )' in acpi_find_matched_drhd_unit.
>> So we may introduce a new field like what I do in v6 or check
>> 'pdev->info.is_virtfn' first in acpi_find_matched_drhd_unit (maybe other
>> places we check pdev->info.is_extfn).
>> 
>> Which one do you prefer?
> 
> Looking at this again I'm not sure why you need any modifications to
> acpi_find_matched_drhd_unit. If the virtual function is an extended
> function pdev->bus should be equal to pdev->info.physfn.bus, in which
> case the already existing is_extfn check will already DTRT?
> 
> Ie: an extended VF should always have the same bus as the PF it
> belongs to, unless I'm missing something.

Why would that be?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Roger Pau Monné]

On Wed, Aug 23, 2017 at 01:20:13AM -0600, Jan Beulich wrote:
> >>> On 23.08.17 at 09:16, <roger.pau@citrix.com> wrote:
> > On Wed, Aug 23, 2017 at 09:05:14AM +0800, Chao Gao wrote:
> >> On Tue, Aug 22, 2017 at 06:43:49AM -0600, Jan Beulich wrote:
> >> >>>> On 21.08.17 at 23:52, <chao.gao@intel.com> wrote:
> >> >> --- 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 {
> >> >> +    /*
> >> >> +     * When 'is_virtfn' is set, 'is_extfn' is re-used to indicate whether
> >> >> +     * the PF of this VF is an extended function.
> >> >> +     */
> >> >
> >> >I'd be inclined to extend the comment by appending ", as a VF itself
> >> >can never be an extended function." Is that correct? If so, would
> >> 
> >> Hi, Jan and Roger.
> >> 
> >> Strictly speaking, the VF can be an extended function. The definition is
> >> within ARI device (in this kind of device, device field is treated as an
> >> extension of function number) and function number is greater than 7. But
> >> this field isn't used as we don't care about whether a VF is or not an
> >> extended function (at least at present).
> >> 
> >> Eric reviewed this patch and told me we may match
> >> 'if ( pdev->info.is_extfn )' in acpi_find_matched_drhd_unit.
> >> So we may introduce a new field like what I do in v6 or check
> >> 'pdev->info.is_virtfn' first in acpi_find_matched_drhd_unit (maybe other
> >> places we check pdev->info.is_extfn).
> >> 
> >> Which one do you prefer?
> > 
> > Looking at this again I'm not sure why you need any modifications to
> > acpi_find_matched_drhd_unit. If the virtual function is an extended
> > function pdev->bus should be equal to pdev->info.physfn.bus, in which
> > case the already existing is_extfn check will already DTRT?
> > 
> > Ie: an extended VF should always have the same bus as the PF it
> > belongs to, unless I'm missing something.
> 
> Why would that be?

It is my understanding (which might be wrong), that an extended
function simply uses 8 bits for the function number, which on a
traditional device would be used for both the slot and the function
number.

So extended functions have no slot, but the bus number is the same for
all of them, or else they would belong to different devices due to the
difference in the bus numbers.

Maybe what I'm missing is whether it is possible to have a device with
virtual functions that expand across several buses?

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Chao Gao]

On Wed, Aug 23, 2017 at 08:31:51AM +0100, Roger Pau Monné wrote:
>On Wed, Aug 23, 2017 at 01:20:13AM -0600, Jan Beulich wrote:
>> >>> On 23.08.17 at 09:16, <roger.pau@citrix.com> wrote:
>> > On Wed, Aug 23, 2017 at 09:05:14AM +0800, Chao Gao wrote:
>> >> On Tue, Aug 22, 2017 at 06:43:49AM -0600, Jan Beulich wrote:
>> >> >>>> On 21.08.17 at 23:52, <chao.gao@intel.com> wrote:
>> >> >> --- 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 {
>> >> >> +    /*
>> >> >> +     * When 'is_virtfn' is set, 'is_extfn' is re-used to indicate whether
>> >> >> +     * the PF of this VF is an extended function.
>> >> >> +     */
>> >> >
>> >> >I'd be inclined to extend the comment by appending ", as a VF itself
>> >> >can never be an extended function." Is that correct? If so, would
>> >> 
>> >> Hi, Jan and Roger.
>> >> 
>> >> Strictly speaking, the VF can be an extended function. The definition is
>> >> within ARI device (in this kind of device, device field is treated as an
>> >> extension of function number) and function number is greater than 7. But
>> >> this field isn't used as we don't care about whether a VF is or not an
>> >> extended function (at least at present).
>> >> 
>> >> Eric reviewed this patch and told me we may match
>> >> 'if ( pdev->info.is_extfn )' in acpi_find_matched_drhd_unit.
>> >> So we may introduce a new field like what I do in v6 or check
>> >> 'pdev->info.is_virtfn' first in acpi_find_matched_drhd_unit (maybe other
>> >> places we check pdev->info.is_extfn).
>> >> 
>> >> Which one do you prefer?
>> > 
>> > Looking at this again I'm not sure why you need any modifications to
>> > acpi_find_matched_drhd_unit. If the virtual function is an extended
>> > function pdev->bus should be equal to pdev->info.physfn.bus, in which
>> > case the already existing is_extfn check will already DTRT?
>> > 
>> > Ie: an extended VF should always have the same bus as the PF it
>> > belongs to, unless I'm missing something.
>> 
>> Why would that be?
>
>It is my understanding (which might be wrong), that an extended
>function simply uses 8 bits for the function number, which on a
>traditional device would be used for both the slot and the function
>number.
>
>So extended functions have no slot, but the bus number is the same for
>all of them, or else they would belong to different devices due to the
>difference in the bus numbers.
>
>Maybe what I'm missing is whether it is possible to have a device with
>virtual functions that expand across several buses?

It is not true. Please refer to the 2.1.2 VF Discovery of SR-IOV spec.
The numbers of VF can be larger than 256 and so it is definite that
sometimes VF's bus number would be different from the PF's.

Thanks
Chao

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Roger Pau Monné]

On Wed, Aug 23, 2017 at 02:46:08PM +0800, Chao Gao wrote:
> On Wed, Aug 23, 2017 at 08:31:51AM +0100, Roger Pau Monné wrote:
> >On Wed, Aug 23, 2017 at 01:20:13AM -0600, Jan Beulich wrote:
> >> >>> On 23.08.17 at 09:16, <roger.pau@citrix.com> wrote:
> >> > On Wed, Aug 23, 2017 at 09:05:14AM +0800, Chao Gao wrote:
> >> >> On Tue, Aug 22, 2017 at 06:43:49AM -0600, Jan Beulich wrote:
> >> >> >>>> On 21.08.17 at 23:52, <chao.gao@intel.com> wrote:
> >> >> >> --- 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 {
> >> >> >> +    /*
> >> >> >> +     * When 'is_virtfn' is set, 'is_extfn' is re-used to indicate whether
> >> >> >> +     * the PF of this VF is an extended function.
> >> >> >> +     */
> >> >> >
> >> >> >I'd be inclined to extend the comment by appending ", as a VF itself
> >> >> >can never be an extended function." Is that correct? If so, would
> >> >> 
> >> >> Hi, Jan and Roger.
> >> >> 
> >> >> Strictly speaking, the VF can be an extended function. The definition is
> >> >> within ARI device (in this kind of device, device field is treated as an
> >> >> extension of function number) and function number is greater than 7. But
> >> >> this field isn't used as we don't care about whether a VF is or not an
> >> >> extended function (at least at present).
> >> >> 
> >> >> Eric reviewed this patch and told me we may match
> >> >> 'if ( pdev->info.is_extfn )' in acpi_find_matched_drhd_unit.
> >> >> So we may introduce a new field like what I do in v6 or check
> >> >> 'pdev->info.is_virtfn' first in acpi_find_matched_drhd_unit (maybe other
> >> >> places we check pdev->info.is_extfn).
> >> >> 
> >> >> Which one do you prefer?
> >> > 
> >> > Looking at this again I'm not sure why you need any modifications to
> >> > acpi_find_matched_drhd_unit. If the virtual function is an extended
> >> > function pdev->bus should be equal to pdev->info.physfn.bus, in which
> >> > case the already existing is_extfn check will already DTRT?
> >> > 
> >> > Ie: an extended VF should always have the same bus as the PF it
> >> > belongs to, unless I'm missing something.
> >> 
> >> Why would that be?
> >
> >It is my understanding (which might be wrong), that an extended
> >function simply uses 8 bits for the function number, which on a
> >traditional device would be used for both the slot and the function
> >number.
> >
> >So extended functions have no slot, but the bus number is the same for
> >all of them, or else they would belong to different devices due to the
> >difference in the bus numbers.
> >
> >Maybe what I'm missing is whether it is possible to have a device with
> >virtual functions that expand across several buses?
> 
> It is not true. Please refer to the 2.1.2 VF Discovery of SR-IOV spec.
> The numbers of VF can be larger than 256 and so it is definite that
> sometimes VF's bus number would be different from the PF's.

So that's what I was missing, thanks.

Then I would modify acpi_find_matched_drhd_unit so it's:

    if ( pdev->info.is_extfn )
    {
        bus = pdev->info.is_virtfn ? pdev->info.physfn.bus : pdev->bus;
        devfn = 0;
    }

AFAICT that should work?

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Chao Gao]

On Wed, Aug 23, 2017 at 09:01:07AM +0100, Roger Pau Monné wrote:
>On Wed, Aug 23, 2017 at 02:46:08PM +0800, Chao Gao wrote:
>> On Wed, Aug 23, 2017 at 08:31:51AM +0100, Roger Pau Monné wrote:
>> >On Wed, Aug 23, 2017 at 01:20:13AM -0600, Jan Beulich wrote:
>> >> >>> On 23.08.17 at 09:16, <roger.pau@citrix.com> wrote:
>> >> > On Wed, Aug 23, 2017 at 09:05:14AM +0800, Chao Gao wrote:
>> >> >> On Tue, Aug 22, 2017 at 06:43:49AM -0600, Jan Beulich wrote:
>> >> >> >>>> On 21.08.17 at 23:52, <chao.gao@intel.com> wrote:
>> >> >> >> --- 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 {
>> >> >> >> +    /*
>> >> >> >> +     * When 'is_virtfn' is set, 'is_extfn' is re-used to indicate whether
>> >> >> >> +     * the PF of this VF is an extended function.
>> >> >> >> +     */
>> >> >> >
>> >> >> >I'd be inclined to extend the comment by appending ", as a VF itself
>> >> >> >can never be an extended function." Is that correct? If so, would
>> >> >> 
>> >> >> Hi, Jan and Roger.
>> >> >> 
>> >> >> Strictly speaking, the VF can be an extended function. The definition is
>> >> >> within ARI device (in this kind of device, device field is treated as an
>> >> >> extension of function number) and function number is greater than 7. But
>> >> >> this field isn't used as we don't care about whether a VF is or not an
>> >> >> extended function (at least at present).
>> >> >> 
>> >> >> Eric reviewed this patch and told me we may match
>> >> >> 'if ( pdev->info.is_extfn )' in acpi_find_matched_drhd_unit.
>> >> >> So we may introduce a new field like what I do in v6 or check
>> >> >> 'pdev->info.is_virtfn' first in acpi_find_matched_drhd_unit (maybe other
>> >> >> places we check pdev->info.is_extfn).
>> >> >> 
>> >> >> Which one do you prefer?
>> >> > 
>> >> > Looking at this again I'm not sure why you need any modifications to
>> >> > acpi_find_matched_drhd_unit. If the virtual function is an extended
>> >> > function pdev->bus should be equal to pdev->info.physfn.bus, in which
>> >> > case the already existing is_extfn check will already DTRT?
>> >> > 
>> >> > Ie: an extended VF should always have the same bus as the PF it
>> >> > belongs to, unless I'm missing something.
>> >> 
>> >> Why would that be?
>> >
>> >It is my understanding (which might be wrong), that an extended
>> >function simply uses 8 bits for the function number, which on a
>> >traditional device would be used for both the slot and the function
>> >number.
>> >
>> >So extended functions have no slot, but the bus number is the same for
>> >all of them, or else they would belong to different devices due to the
>> >difference in the bus numbers.
>> >
>> >Maybe what I'm missing is whether it is possible to have a device with
>> >virtual functions that expand across several buses?
>> 
>> It is not true. Please refer to the 2.1.2 VF Discovery of SR-IOV spec.
>> The numbers of VF can be larger than 256 and so it is definite that
>> sometimes VF's bus number would be different from the PF's.
>
>So that's what I was missing, thanks.
>
>Then I would modify acpi_find_matched_drhd_unit so it's:
>
>    if ( pdev->info.is_extfn )
>    {
>        bus = pdev->info.is_virtfn ? pdev->info.physfn.bus : pdev->bus;
>        devfn = 0;
>    }
>
>AFAICT that should work?

Fine to me.

Jan, What your opinion on this piece of code?

Thanks
Chao

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Jan Beulich]

>>> On 23.08.17 at 09:42, <chao.gao@intel.com> wrote:
> On Wed, Aug 23, 2017 at 09:01:07AM +0100, Roger Pau Monné wrote:
>>On Wed, Aug 23, 2017 at 02:46:08PM +0800, Chao Gao wrote:
>>> On Wed, Aug 23, 2017 at 08:31:51AM +0100, Roger Pau Monné wrote:
>>> >On Wed, Aug 23, 2017 at 01:20:13AM -0600, Jan Beulich wrote:
>>> >> >>> On 23.08.17 at 09:16, <roger.pau@citrix.com> wrote:
>>> >> > On Wed, Aug 23, 2017 at 09:05:14AM +0800, Chao Gao wrote:
>>> >> >> On Tue, Aug 22, 2017 at 06:43:49AM -0600, Jan Beulich wrote:
>>> >> >> >>>> On 21.08.17 at 23:52, <chao.gao@intel.com> wrote:
>>> >> >> >> --- 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 {
>>> >> >> >> +    /*
>>> >> >> >> +     * When 'is_virtfn' is set, 'is_extfn' is re-used to indicate whether
>>> >> >> >> +     * the PF of this VF is an extended function.
>>> >> >> >> +     */
>>> >> >> >
>>> >> >> >I'd be inclined to extend the comment by appending ", as a VF itself
>>> >> >> >can never be an extended function." Is that correct? If so, would
>>> >> >> 
>>> >> >> Hi, Jan and Roger.
>>> >> >> 
>>> >> >> Strictly speaking, the VF can be an extended function. The definition is
>>> >> >> within ARI device (in this kind of device, device field is treated as an
>>> >> >> extension of function number) and function number is greater than 7. But
>>> >> >> this field isn't used as we don't care about whether a VF is or not an
>>> >> >> extended function (at least at present).
>>> >> >> 
>>> >> >> Eric reviewed this patch and told me we may match
>>> >> >> 'if ( pdev->info.is_extfn )' in acpi_find_matched_drhd_unit.
>>> >> >> So we may introduce a new field like what I do in v6 or check
>>> >> >> 'pdev->info.is_virtfn' first in acpi_find_matched_drhd_unit (maybe other
>>> >> >> places we check pdev->info.is_extfn).
>>> >> >> 
>>> >> >> Which one do you prefer?
>>> >> > 
>>> >> > Looking at this again I'm not sure why you need any modifications to
>>> >> > acpi_find_matched_drhd_unit. If the virtual function is an extended
>>> >> > function pdev->bus should be equal to pdev->info.physfn.bus, in which
>>> >> > case the already existing is_extfn check will already DTRT?
>>> >> > 
>>> >> > Ie: an extended VF should always have the same bus as the PF it
>>> >> > belongs to, unless I'm missing something.
>>> >> 
>>> >> Why would that be?
>>> >
>>> >It is my understanding (which might be wrong), that an extended
>>> >function simply uses 8 bits for the function number, which on a
>>> >traditional device would be used for both the slot and the function
>>> >number.
>>> >
>>> >So extended functions have no slot, but the bus number is the same for
>>> >all of them, or else they would belong to different devices due to the
>>> >difference in the bus numbers.
>>> >
>>> >Maybe what I'm missing is whether it is possible to have a device with
>>> >virtual functions that expand across several buses?
>>> 
>>> It is not true. Please refer to the 2.1.2 VF Discovery of SR-IOV spec.
>>> The numbers of VF can be larger than 256 and so it is definite that
>>> sometimes VF's bus number would be different from the PF's.
>>
>>So that's what I was missing, thanks.
>>
>>Then I would modify acpi_find_matched_drhd_unit so it's:
>>
>>    if ( pdev->info.is_extfn )
>>    {
>>        bus = pdev->info.is_virtfn ? pdev->info.physfn.bus : pdev->bus;
>>        devfn = 0;
>>    }
>>
>>AFAICT that should work?
> 
> Fine to me.
> 
> Jan, What your opinion on this piece of code?

Looks fine to me, but you'll rather need Kevin's input here, as he'd
the VT-d maintainer.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, August 23, 2017 4:53 PM
> 
> >>> On 23.08.17 at 09:42, <chao.gao@intel.com> wrote:
> > On Wed, Aug 23, 2017 at 09:01:07AM +0100, Roger Pau Monné wrote:
> >>On Wed, Aug 23, 2017 at 02:46:08PM +0800, Chao Gao wrote:
> >>> On Wed, Aug 23, 2017 at 08:31:51AM +0100, Roger Pau Monné wrote:
> >>> >On Wed, Aug 23, 2017 at 01:20:13AM -0600, Jan Beulich wrote:
> >>> >> >>> On 23.08.17 at 09:16, <roger.pau@citrix.com> wrote:
> >>> >> > On Wed, Aug 23, 2017 at 09:05:14AM +0800, Chao Gao wrote:
> >>> >> >> On Tue, Aug 22, 2017 at 06:43:49AM -0600, Jan Beulich wrote:
> >>> >> >> >>>> On 21.08.17 at 23:52, <chao.gao@intel.com> wrote:
> >>> >> >> >> --- 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 {
> >>> >> >> >> +    /*
> >>> >> >> >> +     * When 'is_virtfn' is set, 'is_extfn' is re-used to indicate
> whether
> >>> >> >> >> +     * the PF of this VF is an extended function.
> >>> >> >> >> +     */
> >>> >> >> >
> >>> >> >> >I'd be inclined to extend the comment by appending ", as a VF
> itself
> >>> >> >> >can never be an extended function." Is that correct? If so, would
> >>> >> >>
> >>> >> >> Hi, Jan and Roger.
> >>> >> >>
> >>> >> >> Strictly speaking, the VF can be an extended function. The
> definition is
> >>> >> >> within ARI device (in this kind of device, device field is treated as
> an
> >>> >> >> extension of function number) and function number is greater
> than 7. But
> >>> >> >> this field isn't used as we don't care about whether a VF is or not
> an
> >>> >> >> extended function (at least at present).
> >>> >> >>
> >>> >> >> Eric reviewed this patch and told me we may match
> >>> >> >> 'if ( pdev->info.is_extfn )' in acpi_find_matched_drhd_unit.
> >>> >> >> So we may introduce a new field like what I do in v6 or check
> >>> >> >> 'pdev->info.is_virtfn' first in acpi_find_matched_drhd_unit
> (maybe other
> >>> >> >> places we check pdev->info.is_extfn).
> >>> >> >>
> >>> >> >> Which one do you prefer?
> >>> >> >
> >>> >> > Looking at this again I'm not sure why you need any modifications
> to
> >>> >> > acpi_find_matched_drhd_unit. If the virtual function is an
> extended
> >>> >> > function pdev->bus should be equal to pdev->info.physfn.bus, in
> which
> >>> >> > case the already existing is_extfn check will already DTRT?
> >>> >> >
> >>> >> > Ie: an extended VF should always have the same bus as the PF it
> >>> >> > belongs to, unless I'm missing something.
> >>> >>
> >>> >> Why would that be?
> >>> >
> >>> >It is my understanding (which might be wrong), that an extended
> >>> >function simply uses 8 bits for the function number, which on a
> >>> >traditional device would be used for both the slot and the function
> >>> >number.
> >>> >
> >>> >So extended functions have no slot, but the bus number is the same
> for
> >>> >all of them, or else they would belong to different devices due to the
> >>> >difference in the bus numbers.
> >>> >
> >>> >Maybe what I'm missing is whether it is possible to have a device with
> >>> >virtual functions that expand across several buses?
> >>>
> >>> It is not true. Please refer to the 2.1.2 VF Discovery of SR-IOV spec.
> >>> The numbers of VF can be larger than 256 and so it is definite that
> >>> sometimes VF's bus number would be different from the PF's.
> >>
> >>So that's what I was missing, thanks.
> >>
> >>Then I would modify acpi_find_matched_drhd_unit so it's:
> >>
> >>    if ( pdev->info.is_extfn )
> >>    {
> >>        bus = pdev->info.is_virtfn ? pdev->info.physfn.bus : pdev->bus;
> >>        devfn = 0;
> >>    }
> >>
> >>AFAICT that should work?
> >
> > Fine to me.
> >
> > Jan, What your opinion on this piece of code?
> 
> Looks fine to me, but you'll rather need Kevin's input here, as he'd
> the VT-d maintainer.
> 

yes, above is what I'm looking for. Don't forget to also fix is_virtfn
branch:

    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.phsfn.devfn;
    }

Thanks
Kevin
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Jan Beulich]

>>> On 23.08.17 at 09:31, <roger.pau@citrix.com> wrote:
> Maybe what I'm missing is whether it is possible to have a device with
> virtual functions that expand across several buses?

The typical (non-ARI) arrangement I've seen is for all VFs to always
live on buses different from the PF (which is quite obvious, as each
function of each device on a given bus may be a PF, and the max
number of VFs is quite a bit higher than 256 this way). I don't see
why ARI would change that.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Jan Beulich]

>>> On 23.08.17 at 03:05, <chao.gao@intel.com> wrote:
> Strictly speaking, the VF can be an extended function. The definition is
> within ARI device (in this kind of device, device field is treated as an
> extension of function number) and function number is greater than 7. But
> this field isn't used as we don't care about whether a VF is or not an
> extended function (at least at present).

Hmm, that's not in line with what Linux'es xen_add_device() does:

#ifdef CONFIG_PCI_IOV
		if (pci_dev->is_virtfn) {
			add->flags = XEN_PCI_DEV_VIRTFN;
			add->physfn.bus = physfn->bus->number;
			add->physfn.devfn = physfn->devfn;
		} else
#endif
		if (pci_ari_enabled(pci_dev->bus) && PCI_SLOT(pci_dev->devfn))
			add->flags = XEN_PCI_DEV_EXTFN;

Note the "else" in there. Are you saying this is actually wrong? (I
indeed do see ARI capability structures in the VFs of the one
SR-IOV capable system I have direct access to.)

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Chao Gao]

On Wed, Aug 23, 2017 at 02:04:24AM -0600, Jan Beulich wrote:
>>>> On 23.08.17 at 03:05, <chao.gao@intel.com> wrote:
>> Strictly speaking, the VF can be an extended function. The definition is
>> within ARI device (in this kind of device, device field is treated as an
>> extension of function number) and function number is greater than 7. But
>> this field isn't used as we don't care about whether a VF is or not an
>> extended function (at least at present).
>
>Hmm, that's not in line with what Linux'es xen_add_device() does:
>
>#ifdef CONFIG_PCI_IOV
>		if (pci_dev->is_virtfn) {
>			add->flags = XEN_PCI_DEV_VIRTFN;
>			add->physfn.bus = physfn->bus->number;
>			add->physfn.devfn = physfn->devfn;
>		} else
>#endif
>		if (pci_ari_enabled(pci_dev->bus) && PCI_SLOT(pci_dev->devfn))
>			add->flags = XEN_PCI_DEV_EXTFN;
>
>Note the "else" in there. Are you saying this is actually wrong? (I
>indeed do see ARI capability structures in the VFs of the one
>SR-IOV capable system I have direct access to.)

Yes. I think it is wrong. Considering no one in Xen needs this
information, don't set XEN_PCI_DEV_EXTFN for VF is acceptable.

Thanks
Chao

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Jan Beulich]

>>> On 23.08.17 at 09:39, <chao.gao@intel.com> wrote:
> On Wed, Aug 23, 2017 at 02:04:24AM -0600, Jan Beulich wrote:
>>>>> On 23.08.17 at 03:05, <chao.gao@intel.com> wrote:
>>> Strictly speaking, the VF can be an extended function. The definition is
>>> within ARI device (in this kind of device, device field is treated as an
>>> extension of function number) and function number is greater than 7. But
>>> this field isn't used as we don't care about whether a VF is or not an
>>> extended function (at least at present).
>>
>>Hmm, that's not in line with what Linux'es xen_add_device() does:
>>
>>#ifdef CONFIG_PCI_IOV
>>		if (pci_dev->is_virtfn) {
>>			add->flags = XEN_PCI_DEV_VIRTFN;
>>			add->physfn.bus = physfn->bus->number;
>>			add->physfn.devfn = physfn->devfn;
>>		} else
>>#endif
>>		if (pci_ari_enabled(pci_dev->bus) && PCI_SLOT(pci_dev->devfn))
>>			add->flags = XEN_PCI_DEV_EXTFN;
>>
>>Note the "else" in there. Are you saying this is actually wrong? (I
>>indeed do see ARI capability structures in the VFs of the one
>>SR-IOV capable system I have direct access to.)
> 
> Yes. I think it is wrong. Considering no one in Xen needs this
> information, don't set XEN_PCI_DEV_EXTFN for VF is acceptable.

Well, that's the current situation. However, the information Dom0
reports to Xen should be correct, i.e. someone may at some point
decide to fix the code above. That would then collide with your
re-use of the is_extfn field in Xen, which then suddenly would be
set by other than what your patch arranges for.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Tian, Kevin]

> From: Gao, Chao
> Sent: Tuesday, August 22, 2017 5:52 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, the BDF of PF or the BDF of a traditional function may be used.
> And
> it depends on whether the PF is an extended function or not.
> 
> Current code uses PCI_SLOT() to recognize an ARI 'Extended Funcion'. But it
> is problematic for a corner case (a RC endpoint with SRIOV capability

it's not a corner case. It's "conceptually wrong" w/o checking is_extfn.

> and has its own VT-d unit), leading to matching to a wrong VT-d unit.
> 
> This patch reuses 'is_extfn' field in VF's struct pci_dev_info to indicate
> whether the PF of this VF is an extended function. The field helps to use
> correct BDF to search VT-d unit.

We should directly call "whether this VF is an extended function".

SR-IOV spec clearly says:

--
The ARI capability enables a Device to support up to 256 Functions - 
Functions, PFs, or VFs in any combination - associated with the 
captured Bus Number.
--

So a VF with function number >7 is also an extended function.

> 
> Reported-by: Crawford, Eric R <Eric.R.Crawford@intel.com>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> ---
> 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      | 6 ++++++
>  xen/drivers/passthrough/vtd/dmar.c | 2 +-
>  xen/include/xen/pci.h              | 4 ++++
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 27bdb71..2a91320 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";
> @@ -608,6 +609,8 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>      {
>          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,
> @@ -707,6 +710,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>                     seg, bus, slot, func, ctrl);
>      }
> 
> +    /* VF's 'is_extfn' is used to indicate whether PF is an extended function
> */
> +    if ( pdev->info.is_virtfn )
> +        pdev->info.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..83ce5d4 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.is_extfn ? 0 : pdev->info.physfn.devfn;
>      }

If you looked at Linux side code, XEN_PCI_DEV_EXTFN is set
for both PF/VF, so you don't need check is_extfn specifically
within is_virtfn branch. checks of extended functions should
be constrained within is_extfn branch

>      else
>      {
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index 59b6e8a..3b0da66 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 {
> +    /*
> +     * When 'is_virtfn' is set, 'is_extfn' is re-used to indicate whether
> +     * the PF of this VF is an extended function.
> +     */

this comment is meaningless then, since it does indicate whether
VF is 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

Re: [PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Tian, Kevin]

> From: Tian, Kevin
> Sent: Thursday, August 24, 2017 3:22 PM
> 
> > From: Gao, Chao
> > Sent: Tuesday, August 22, 2017 5:52 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, the BDF of PF or the BDF of a traditional function may be used.
> > And
> > it depends on whether the PF is an extended function or not.
> >
> > Current code uses PCI_SLOT() to recognize an ARI 'Extended Funcion'. But
> it
> > is problematic for a corner case (a RC endpoint with SRIOV capability
> 
> it's not a corner case. It's "conceptually wrong" w/o checking is_extfn.
> 
> > and has its own VT-d unit), leading to matching to a wrong VT-d unit.
> >
> > This patch reuses 'is_extfn' field in VF's struct pci_dev_info to indicate
> > whether the PF of this VF is an extended function. The field helps to use
> > correct BDF to search VT-d unit.
> 
> We should directly call "whether this VF is an extended function".
> 
> SR-IOV spec clearly says:
> 
> --
> The ARI capability enables a Device to support up to 256 Functions -
> Functions, PFs, or VFs in any combination - associated with the
> captured Bus Number.
> --
> 
> So a VF with function number >7 is also an extended function.
> 

Had a discussion with Chao. My previous understanding looks
not accurate. From VT-d spec:

1) VF is under the scope of the same VT-d as the PF

2) if PF is extended function, it is under the scope of the same
VT-d as the traditional functions on the endpoint.

Above applies to any VF requestor ID (including <=7), so when setting
is_extfn for a VF, it really doesn't mean VF is an extended function.
Instead it always refers to the PF attribute. Then let's still add the
original comment to mark it out.

Based on that, possibly below logic can better match above policy:

if ( pdev->info.is_virtfn )
{
	bus = pdev->info.physfn.bus;
	devfn = pdev->info.is_extfn ? 0 : pdev->info.physfn.devfn;
}
else if ( pdev->info.is_extfn )
{
	bus = pdev->bus;
	devfn = 0;
}

Thanks
Kevin

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Jan Beulich]

>>> On 24.08.17 at 10:01, <kevin.tian@intel.com> wrote:
>>  From: Tian, Kevin
>> Sent: Thursday, August 24, 2017 3:22 PM
>> 
>> > From: Gao, Chao
>> > Sent: Tuesday, August 22, 2017 5:52 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, the BDF of PF or the BDF of a traditional function may be used.
>> > And
>> > it depends on whether the PF is an extended function or not.
>> >
>> > Current code uses PCI_SLOT() to recognize an ARI 'Extended Funcion'. But
>> it
>> > is problematic for a corner case (a RC endpoint with SRIOV capability
>> 
>> it's not a corner case. It's "conceptually wrong" w/o checking is_extfn.
>> 
>> > and has its own VT-d unit), leading to matching to a wrong VT-d unit.
>> >
>> > This patch reuses 'is_extfn' field in VF's struct pci_dev_info to indicate
>> > whether the PF of this VF is an extended function. The field helps to use
>> > correct BDF to search VT-d unit.
>> 
>> We should directly call "whether this VF is an extended function".
>> 
>> SR-IOV spec clearly says:
>> 
>> --
>> The ARI capability enables a Device to support up to 256 Functions -
>> Functions, PFs, or VFs in any combination - associated with the
>> captured Bus Number.
>> --
>> 
>> So a VF with function number >7 is also an extended function.
>> 
> 
> Had a discussion with Chao. My previous understanding looks
> not accurate. From VT-d spec:
> 
> 1) VF is under the scope of the same VT-d as the PF
> 
> 2) if PF is extended function, it is under the scope of the same
> VT-d as the traditional functions on the endpoint.
> 
> Above applies to any VF requestor ID (including <=7), so when setting
> is_extfn for a VF, it really doesn't mean VF is an extended function.
> Instead it always refers to the PF attribute. Then let's still add the
> original comment to mark it out.
> 
> Based on that, possibly below logic can better match above policy:
> 
> if ( pdev->info.is_virtfn )
> {
> 	bus = pdev->info.physfn.bus;
> 	devfn = pdev->info.is_extfn ? 0 : pdev->info.physfn.devfn;

But that's not in line with what you say above: You look at the
VF's is_extfn here instead of at the PF's one. I.e. that would
only be correct if the PF's flag got propagated to all its VFs,
which I think earlier discussion had ruled out as an option (as
that would depend on the current, assumed buggy, behavior
of the corresponding Linux code to remain unchanged). Or the
sequence of checks in pci_add_device() would need to be
changed, such that is_virtfn is being checked before is_extfn.

Jan

> }
> else if ( pdev->info.is_extfn )
> {
> 	bus = pdev->bus;
> 	devfn = 0;
> }
> 
> Thanks
> Kevin




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v7] VT-d: use correct BDF for VF to search VT-d unit

[Chao Gao]

On Thu, Aug 24, 2017 at 02:22:47AM -0600, Jan Beulich wrote:
>>>> On 24.08.17 at 10:01, <kevin.tian@intel.com> wrote:
>>>  From: Tian, Kevin
>>> Sent: Thursday, August 24, 2017 3:22 PM
>>> 
>>> > From: Gao, Chao
>>> > Sent: Tuesday, August 22, 2017 5:52 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, the BDF of PF or the BDF of a traditional function may be used.
>>> > And
>>> > it depends on whether the PF is an extended function or not.
>>> >
>>> > Current code uses PCI_SLOT() to recognize an ARI 'Extended Funcion'. But
>>> it
>>> > is problematic for a corner case (a RC endpoint with SRIOV capability
>>> 
>>> it's not a corner case. It's "conceptually wrong" w/o checking is_extfn.
>>> 
>>> > and has its own VT-d unit), leading to matching to a wrong VT-d unit.
>>> >
>>> > This patch reuses 'is_extfn' field in VF's struct pci_dev_info to indicate
>>> > whether the PF of this VF is an extended function. The field helps to use
>>> > correct BDF to search VT-d unit.
>>> 
>>> We should directly call "whether this VF is an extended function".
>>> 
>>> SR-IOV spec clearly says:
>>> 
>>> --
>>> The ARI capability enables a Device to support up to 256 Functions -
>>> Functions, PFs, or VFs in any combination - associated with the
>>> captured Bus Number.
>>> --
>>> 
>>> So a VF with function number >7 is also an extended function.
>>> 
>> 
>> Had a discussion with Chao. My previous understanding looks
>> not accurate. From VT-d spec:
>> 
>> 1) VF is under the scope of the same VT-d as the PF
>> 
>> 2) if PF is extended function, it is under the scope of the same
>> VT-d as the traditional functions on the endpoint.
>> 
>> Above applies to any VF requestor ID (including <=7), so when setting
>> is_extfn for a VF, it really doesn't mean VF is an extended function.
>> Instead it always refers to the PF attribute. Then let's still add the
>> original comment to mark it out.
>> 
>> Based on that, possibly below logic can better match above policy:
>> 
>> if ( pdev->info.is_virtfn )
>> {
>> 	bus = pdev->info.physfn.bus;
>> 	devfn = pdev->info.is_extfn ? 0 : pdev->info.physfn.devfn;
>
>But that's not in line with what you say above: You look at the
>VF's is_extfn here instead of at the PF's one. I.e. that would
>only be correct if the PF's flag got propagated to all its VFs,
>which I think earlier discussion had ruled out as an option (as
>that would depend on the current, assumed buggy, behavior
>of the corresponding Linux code to remain unchanged). Or the

I think Kevin did agree to this solution: propageting PF's is_extfn to
all its VF (namely, reuse VF's is_extfn to show whether PF is an
extended function or not). And the sample code may be more
straightforward than Roger's proposal as it can be easily matched to the
rules metioned above.

Thanks
Chao

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel