Re: [PATCH] IntelSiliconPkg IntelVTdDxe: Optimize when func 0 is not implemented

["Zeng, Star" <star.zeng@intel.com>]

Good information. :)
The UEFI shell implementation also has the code below.
                if (PciHeader.VendorId == 0xffff && Func == 0) {
                  break;
                }

                if (PciHeader.VendorId != 0xffff) {


The ScanPciBus() has no functional issue, but has another optimization point.

Current code checks HeaderType of Function 0 even Function 0 is not
implemented. HeaderType value will be 0xFF if Function 0 is not
implemented, then MaxFunction will be set to PCI_MAX_FUNC + 1.

The code can be optimized to only check HeaderType if Function 0 is
implemented.

I just sent another patch for it at https://lists.01.org/pipermail/edk2-devel/2018-September/029636.html.


Thanks,
Star
-----Original Message-----
From: Yao, Jiewen 
Sent: Thursday, September 13, 2018 1:29 PM
To: Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Chang, Tomson <tomson.chang@intel.com>; Huang, Jenny <jenny.huang@intel.com>; Chan, Amy <amy.chan@intel.com>
Subject: RE: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Optimize when func 0 is not implemented

I checked the UEFI shell implementation. It uses below:

                  //
                  // If this is not a multi-function device, we can leave the loop
                  // to deal with the next device.
                  //
                  if (Func == 0 && ((PciHeader.HeaderType & HEADER_TYPE_MULTI_FUNCTION) == 0x00)) {
                    break;
                  }

Thank you
Yao Jiewen

> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Thursday, September 13, 2018 11:11 AM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Chang, Tomson <tomson.chang@intel.com>; Yao, Jiewen 
> <jiewen.yao@intel.com>; Huang, Jenny <jenny.huang@intel.com>; Chan, 
> Amy <amy.chan@intel.com>
> Subject: Re: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Optimize when 
> func
> 0 is not implemented
> 
> On 9/13/2018 10:10 AM, Star Zeng wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1169
> >
> > PCI spec:
> > They are also required to always implement function 0 in the device.
> > Implementing other functions is optional and may be assigned in any 
> > order (i.e., a two-function device must respond to function 0 but 
> > can choose any of the other possible function numbers (1-7) for the 
> > second function).
> >
> > This patch updates ScanPciBus() to not scan other functions if 
> > function 0 is not implemented.
> >
> > Test done:
> > Added debug code below in the second loop of ScanPciBus(), compared 
> > the debug logs with and without this patch, many
> > non-0 unimplemented functions are skipped correctly.
> >
> >    DEBUG ((
> >      DEBUG_INFO,
> >      "%a() B%02xD%02xF%02x VendorId: %04x DeviceId: %04x\n",
> >      __FUNCTION__,
> >      Bus,
> >      Device,
> >      Function,
> >      VendorID,
> >      DeviceID
> >      ));
> >
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
> > Cc: Tomson Chang <tomson.chang@intel.com>
> > Cc: Jenny Huang <jenny.huang@intel.com>
> > Cc: Amy Chan <amy.chan@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Star Zeng <star.zeng@intel.com>
> > ---
> >   IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c | 8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
> b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
> > index 36750b3f1d9c..305995de032c 100644
> > --- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
> > +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
> > @@ -1,6 +1,6 @@
> >   /** @file
> >
> > -  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> > +  Copyright (c) 2017 - 2018, Intel Corporation. All rights 
> > + reserved.<BR>
> >     This program and the accompanying materials
> >     are licensed and made available under the terms and conditions 
> > of
> the BSD License
> >     which accompanies this distribution.  The full text of the 
> > license may
> be found at
> > @@ -247,6 +247,12 @@ ScanPciBus (
> >         VendorID  = PciSegmentRead16
> (PCI_SEGMENT_LIB_ADDRESS(Segment, Bus, Device, Function, 
> PCI_VENDOR_ID_OFFSET));
> >         DeviceID  = PciSegmentRead16
> (PCI_SEGMENT_LIB_ADDRESS(Segment, Bus, Device, Function, 
> PCI_DEVICE_ID_OFFSET));
> >         if (VendorID == 0xFFFF && DeviceID == 0xFFFF) {
> > +        if (Function == 0) {
> > +          //
> > +          // If function 0 is not implemented, do not scan other
> functions.
> > +          //
> > +          break;
> > +        }
> >           continue;
> >         }
> >
> >
> Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
> 
> --
> Thanks,
> Ray