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

[Chao Gao <chao.gao@xxxxxxxxx>]

On Thu, Aug 24, 2017 at 02:22:47AM -0600, Jan Beulich wrote:
>>>> On 24.08.17 at 10:01, <kevin.tian@xxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel