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

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

Thanks.

Please also help review [PATCH] IntelSiliconPkg IntelVTdDxe: Check HeaderType if func 0 is implemented.


Star
-----Original Message-----
From: Yao, Jiewen 
Sent: Thursday, September 13, 2018 2:42 PM
To: Zeng, Star <star.zeng@intel.com>; Ni, Ruiyu <ruiyu.ni@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

Sounds good.

Reviewed-by: Jiewen.yao@intel.com

> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, September 13, 2018 2:30 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu 
> <ruiyu.ni@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>; Zeng, Star 
> <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Optimize when 
> func
> 0 is not implemented
> 
> 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