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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel