[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RESEND v9] VT-d: use correct BDF for VF to search VT-d unit
On Fri, Aug 25, 2017 at 09:20:23AM -0600, Jan Beulich wrote: >>>> On 25.08.17 at 15:51, <chao.gao@xxxxxxxxx> wrote: >> On Fri, Aug 25, 2017 at 03:39:38AM -0600, Jan Beulich wrote: >>>>>> On 25.08.17 at 07:27, <chao.gao@xxxxxxxxx> 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 conceptually wrong w/o checking whether PF is an extended function and >>>> would lead to match VFs of a RC endpoint to a wrong VT-d unit. >>>> >>>> This patch uses VF's 'is_extfn' field to indicate whether the PF of this >>>> VF >>>> is >>>> an extended function. The field helps to use correct BDF to search VT-d >>>> unit. >>>> >>>> Reported-by: Crawford, Eric R <Eric.R.Crawford@xxxxxxxxx> >>>> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx> >>>> --- >>>> - RESEND for the previous email has no subject. >>>> >>>> v9: >>>> - check 'is_virtfn' first in pci_add_device() to avoid potential error if >>>> linux side sets VF's 'is_extfn' >>>> - comments changes to make it clear that we use VF's 'is_extfn' >>>> intentionally >>>> otherwise the patch seems like a workaround. >>>> >>>> v8: >>>> - use "conceptually wrong", instead of "a corner case" in commit message >>>> - check 'is_virtfn' first in acpi_find_matched_drhd_unit() >>>> >>>> v7: >>>> - Drop Eric's tested-by >>>> - Change commit message to be clearer >>>> - Re-use VF's is_extfn field >>>> - access PF's is_extfn field in locked area >>>> >>>> --- >>>> xen/drivers/passthrough/pci.c | 14 ++++++++++---- >>>> xen/drivers/passthrough/vtd/dmar.c | 12 ++++++------ >>>> xen/include/xen/pci.h | 1 + >>>> 3 files changed, 17 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c >>>> index 27bdb71..0e27e29 100644 >>>> --- a/xen/drivers/passthrough/pci.c >>>> +++ b/xen/drivers/passthrough/pci.c >>>> @@ -599,21 +599,24 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, >>>> unsigned int slot = PCI_SLOT(devfn), func = PCI_FUNC(devfn); >>>> const char *pdev_type; >>>> int ret; >>>> + bool pf_is_extfn = false; >>>> >>>> - if (!info) >>>> + if ( !info ) >>>> pdev_type = "device"; >>>> - else if (info->is_extfn) >>>> - pdev_type = "extended function"; >>>> - else if (info->is_virtfn) >>>> + else if ( info->is_virtfn ) >>>> { >>>> pcidevs_lock(); >>>> pdev = pci_get_pdev(seg, info->physfn.bus, info->physfn.devfn); >>>> + if ( pdev ) >>>> + pf_is_extfn = pdev->info.is_extfn; >>>> pcidevs_unlock(); >>>> if ( !pdev ) >>>> pci_add_device(seg, info->physfn.bus, info->physfn.devfn, >>>> NULL, node); >>>> pdev_type = "virtual function"; >>>> } >>>> + else if ( info->is_extfn ) >>>> + pdev_type = "extended function"; >>>> else >>>> { >>>> info = NULL; >>>> @@ -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); >>> >>>Can this please be moved up right next to >>> >>> pdev->info = *info; >>> >>>, so that information is right from the point it is being stored? And >> >> Yes. I will. >> >>>looking at that code I can't really work out why the SR-IOV device >>>handling is in an "else if" to that path. I can't check that case >>>myself, as by box'es root ports don't support ARI forwarding, so >>>despite PF and VF being ARI-capable it can't be enabled, and >>>hence I'm not seeing the devices reported as extended functions. >> >> Yeah. I think we should remove "else if" for it is the only place >> where vf_rlen[] is set, otherwise extended PF's vf_rlen[] won't be >> initialized. I think we don't have extended PF at present, so the bug >> isn't triggered. > >So none of your chipsets implement ARI forwarding? I would have >hoped you could test this somewhere. No. I mean PF always has function number <= 7 at present. Thus, it can't be extended function even ARI forwarding enabled in upstream bridge. > >> Currently, VF won't implement SRIOV feature, seeing >> SRIOV specv1.1 chapter 3.7 PCI Express Extended Capabilities. Even VF >> will implement SRIOV later, I think as long as a function is SRIOV >> capable, we can initialize vf_rlen[] here. > >How could a VF itself implement SR-IOV? I don't know. Thanks Chao _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |