| Message ID | 1503352324-13467-1-git-send-email-chao.gao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
>>> 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
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
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
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.
>>> 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
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.
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
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
>>> 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
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.
>>> 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
>>> 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
>>> 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
> 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
> 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
> 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
>>> 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
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
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 {
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(-)