From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by mx.groups.io with SMTP id smtpd.web11.2602.1576584215701135004 for ; Tue, 17 Dec 2019 04:03:35 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.115, mailfrom: ray.ni@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Dec 2019 04:03:35 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,325,1571727600"; d="scan'208";a="247441697" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga002.fm.intel.com with ESMTP; 17 Dec 2019 04:03:35 -0800 Received: from fmsmsx112.amr.corp.intel.com (10.18.116.6) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 17 Dec 2019 04:03:34 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX112.amr.corp.intel.com (10.18.116.6) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 17 Dec 2019 04:03:34 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.90]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.222]) with mapi id 14.03.0439.000; Tue, 17 Dec 2019 20:03:32 +0800 From: "Ni, Ray" To: "Javeed, Ashraf" , "devel@edk2.groups.io" CC: "Wang, Jian J" , "Wu, Hao A" Subject: Re: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 07/12] PciBusDxe: Record the PCI-Express Capability Structure Thread-Topic: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 07/12] PciBusDxe: Record the PCI-Express Capability Structure Thread-Index: AQHVmdKXRQH/ws/yR0CQahFyU1GqEqe+b3/Q Date: Tue, 17 Dec 2019 12:03:32 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C3A05E7@SHSMSX104.ccr.corp.intel.com> References: <20191101150952.3340-1-ashraf.javeed@intel.com> <15D3127C6DFCD4A7.12315@groups.io> <95C5C2B113DE604FB208120C742E982457917240@BGSMSX101.gar.corp.intel.com> In-Reply-To: <95C5C2B113DE604FB208120C742E982457917240@BGSMSX101.gar.corp.intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMWM2ZmIzNTgtZDY3ZS00OTVkLWE5YmQtMjA2N2M4NDUxMzA1IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoid1wvTGkwQWduMlNBSm81MGg1SFd4cHJwWVRFMXBzOUZpaGt3VDk2WjZZczd4OFBYYjVUcFwvZVpiSWJORTZkbVwvZyJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 2 minor comments. And I suggest you don't change EFI_D_xxx in this patch. Or you could chang= e in a separate patch. > > + PCI_CAPABILITY_PCIEXP PciExpStruct; 1. to align "Pci" which buffers the PCI config space, this can be simply "= PciExp". > > + // > > + // For SR-IOV > > + // > > UINT32 AriCapabilityOffset; > > UINT32 SrIovCapabilityOffset; > > UINT32 MrIovCapabilityOffset; > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > index c7eafff..2343702 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > @@ -230,7 +230,7 @@ PciSearchDevice ( > > PciIoDevice =3D NULL; > > > > DEBUG (( > > - EFI_D_INFO, > > + DEBUG_INFO, > > "PciBus: Discovered %s @ [%02x|%02x|%02x]\n", > > IS_PCI_BRIDGE (Pci) ? L"PPB" : > > IS_CARDBUS_BRIDGE (Pci) ? L"P2C" : > > @@ -397,7 +397,7 @@ DumpPpbPaddingResource ( > > > > if ((Type !=3D PciBarTypeUnknown) && ((ResourceType =3D=3D PciBar= TypeUnknown) > > || (ResourceType =3D=3D Type))) { > > DEBUG (( > > - EFI_D_INFO, > > + DEBUG_INFO, > > " Padding: Type =3D %s; Alignment =3D 0x%lx;\tLength =3D 0x= %lx\n", > > mBarTypeStr[Type], Descriptor->AddrRangeMax, Descriptor->Addr= Len > > )); > > @@ -424,7 +424,7 @@ DumpPciBars ( > > } > > > > DEBUG (( > > - EFI_D_INFO, > > + DEBUG_INFO, > > " BAR[%d]: Type =3D %s; Alignment =3D 0x%lx;\tLength =3D 0x%l= x;\tOffset =3D > > 0x%02x\n", > > Index, mBarTypeStr[MIN (PciIoDevice->PciBar[Index].BarType, > > PciBarTypeMaxType)], > > PciIoDevice->PciBar[Index].Alignment, PciIoDevice->PciBar[Index= ].Length, > > PciIoDevice->PciBar[Index].Offset @@ -437,13 +437,13 @@ DumpPciBars ( > > } > > > > DEBUG (( > > - EFI_D_INFO, > > + DEBUG_INFO, > > " VFBAR[%d]: Type =3D %s; Alignment =3D 0x%lx;\tLength =3D 0x%l= x;\tOffset =3D > > 0x%02x\n", > > Index, mBarTypeStr[MIN (PciIoDevice->VfPciBar[Index].BarType, > > PciBarTypeMaxType)], > > PciIoDevice->VfPciBar[Index].Alignment, PciIoDevice- > > >VfPciBar[Index].Length, PciIoDevice->VfPciBar[Index].Offset > > )); > > } > > - DEBUG ((EFI_D_INFO, "\n")); > > + DEBUG ((DEBUG_INFO, "\n")); > > } > > > > /** > > @@ -1903,7 +1903,7 @@ PciParseBar ( > > // Fix the length to support some special 64 bit BAR > > // > > if (Value =3D=3D 0) { > > - DEBUG ((EFI_D_INFO, "[PciBus]BAR probing for upper 32bit of M= EM64 BAR > > returns 0, change to 0xFFFFFFFF.\n")); > > + DEBUG ((DEBUG_INFO, "[PciBus]BAR probing for upper 32bit of > > + MEM64 BAR returns 0, change to 0xFFFFFFFF.\n")); > > Value =3D (UINT32) -1; > > } else { > > Value |=3D ((UINT32)(-1) << HighBitSet32 (Value)); @@ -2153,7= +2153,17 @@ > > CreatePciIoDevice ( > > NULL > > ); > > if (!EFI_ERROR (Status)) { > > - PciIoDevice->IsPciExp =3D TRUE; > > + PciIoDevice->IsPciExp =3D TRUE; > > + // > > + // read the PCI device's entire PCI Express Capability structure > > + // > > + PciIo->Pci.Read ( > > + PciIo, > > + EfiPciIoWidthUint8, > > + PciIoDevice->PciExpressCapabilityOffset, > > + sizeof (PCI_CAPABILITY_PCIEXP) / sizeof (UINT8), > > + &PciIoDevice->PciExpStruct > > + ); 2. Please follow EDKII coding style document regarding the indent. > > } > > > > if (PcdGetBool (PcdAriSupport)) { > > @@ -2206,7 +2216,7 @@ CreatePciIoDevice ( > > &Data32 > > ); > > DEBUG (( > > - EFI_D_INFO, > > + DEBUG_INFO, > > " ARI: forwarding enabled for PPB[%02x:%02x:%02x]\n", > > Bridge->BusNumber, > > Bridge->DeviceNumber, > > @@ -2215,7 +2225,7 @@ CreatePciIoDevice ( > > } > > } > > > > - DEBUG ((EFI_D_INFO, " ARI: CapOffset =3D 0x%x\n", PciIoDevice- > > >AriCapabilityOffset)); > > + DEBUG ((DEBUG_INFO, " ARI: CapOffset =3D 0x%x\n", > > + PciIoDevice->AriCapabilityOffset)); > > } > > } > > > > @@ -2325,12 +2335,12 @@ CreatePciIoDevice ( > > PciIoDevice->ReservedBusNum =3D (UINT16)(EFI_PCI_BUS_OF_RID (La= stVF) - > > Bus + 1); > > > > DEBUG (( > > - EFI_D_INFO, > > + DEBUG_INFO, > > " SR-IOV: SupportedPageSize =3D 0x%x; SystemPageSize =3D 0x%x= ; FirstVFOffset > > =3D 0x%x;\n", > > SupportedPageSize, PciIoDevice->SystemPageSize >> 12, FirstVF= Offset > > )); > > DEBUG (( > > - EFI_D_INFO, > > + DEBUG_INFO, > > " InitialVFs =3D 0x%x; ReservedBusNum =3D 0x%x; CapOf= fset =3D 0x%x\n", > > PciIoDevice->InitialVFs, PciIoDevice->ReservedBusNum, PciIoDe= vice- > > >SrIovCapabilityOffset > > )); > > @@ -2345,7 +2355,7 @@ CreatePciIoDevice ( > > NULL > > ); > > if (!EFI_ERROR (Status)) { > > - DEBUG ((EFI_D_INFO, " MR-IOV: CapOffset =3D 0x%x\n", PciIoDevic= e- > > >MrIovCapabilityOffset)); > > + DEBUG ((DEBUG_INFO, " MR-IOV: CapOffset =3D 0x%x\n", > > + PciIoDevice->MrIovCapabilityOffset)); > > } > > } > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.c > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.c > > index 9e6671d..df9e696 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.c > > @@ -467,6 +467,14 @@ GetPciFeaturesConfigurationTable ( > > return EFI_SUCCESS; > > } > > > > + // > > + // The PCI features configuration table is not built for RCiEP, > > + return NULL // if > > + (PciDevice->PciExpStruct.Capability.Bits.DevicePortType =3D=3D \ > > + PCIE_DEVICE_PORT_TYPE_ROOT_COMPLEX_INTEGRATED_ENDPOINT) { > > + *PciFeaturesConfigTable =3D NULL; > > + return EFI_SUCCESS; > > + } > > > > if (IsDevicePathEnd (PciDevice->DevicePath)){ > > // > > @@ -575,6 +583,45 @@ IsPciRootPortEmpty ( } > > > > > > +/** > > + helper routine to dump the PCIe Device Port Type **/ VOID > > +DumpDevicePortType ( > > + IN UINT8 DevicePortType > > + ) > > +{ > > + switch (DevicePortType){ > > + case PCIE_DEVICE_PORT_TYPE_PCIE_ENDPOINT: > > + DEBUG (( DEBUG_INFO, "PCIe endpoint found\n")); > > + break; > > + case PCIE_DEVICE_PORT_TYPE_LEGACY_PCIE_ENDPOINT: > > + DEBUG (( DEBUG_INFO, "legacy PCI endpoint found\n")); > > + break; > > + case PCIE_DEVICE_PORT_TYPE_ROOT_PORT: > > + DEBUG (( DEBUG_INFO, "PCIe Root Port found\n")); > > + break; > > + case PCIE_DEVICE_PORT_TYPE_UPSTREAM_PORT: > > + DEBUG (( DEBUG_INFO, "PCI switch upstream port found\n")); > > + break; > > + case PCIE_DEVICE_PORT_TYPE_DOWNSTREAM_PORT: > > + DEBUG (( DEBUG_INFO, "PCI switch downstream port found\n")); > > + break; > > + case PCIE_DEVICE_PORT_TYPE_PCIE_TO_PCI_BRIDGE: > > + DEBUG (( DEBUG_INFO, "PCIe-PCI bridge found\n")); > > + break; > > + case PCIE_DEVICE_PORT_TYPE_PCI_TO_PCIE_BRIDGE: > > + DEBUG (( DEBUG_INFO, "PCI-PCIe bridge found\n")); > > + break; > > + case PCIE_DEVICE_PORT_TYPE_ROOT_COMPLEX_INTEGRATED_ENDPOINT: > > + DEBUG (( DEBUG_INFO, "RCiEP found\n")); > > + break; > > + case PCIE_DEVICE_PORT_TYPE_ROOT_COMPLEX_EVENT_COLLECTOR: > > + DEBUG (( DEBUG_INFO, "RC Event Collector found\n")); > > + break; > > + } > > +} > > + > > /** > > Process each PCI device as per the pltaform and device-specific po= licy. > > > > @@ -590,8 +637,12 @@ SetupDevicePciFeatures ( > > ) > > { > > EFI_STATUS Status; > > + PCI_REG_PCIE_CAPABILITY PcieCap; > > OTHER_PCI_FEATURES_CONFIGURATION_TABLE > > *OtherPciFeaturesConfigTable; > > > > + PcieCap.Uint16 =3D PciDevice->PciExpStruct.Capability.Uint16; > > + DumpDevicePortType ((UINT8)PcieCap.Bits.DevicePortType); > > + > > OtherPciFeaturesConfigTable =3D NULL; > > Status =3D GetPciFeaturesConfigurationTable (PciDevice, > > &OtherPciFeaturesConfigTable); > > if (EFI_ERROR( Status)) { > > -- > > 2.21.0.windows.1 > > > > > >=20