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

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

> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Wednesday, August 23, 2017 4:53 PM
> 
> >>> On 23.08.17 at 09:42, <chao.gao@xxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel