From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by mx.groups.io with SMTP id smtpd.web12.11263.1576679738068151244 for ; Wed, 18 Dec 2019 06:35:38 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.43, mailfrom: ashraf.javeed@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Dec 2019 06:35:37 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,329,1571727600"; d="scan'208";a="365762186" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga004.jf.intel.com with ESMTP; 18 Dec 2019 06:35:37 -0800 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 18 Dec 2019 06:35:37 -0800 Received: from bgsmsx110.gar.corp.intel.com (10.223.4.212) by fmsmsx110.amr.corp.intel.com (10.18.116.10) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 18 Dec 2019 06:35:36 -0800 Received: from bgsmsx101.gar.corp.intel.com ([169.254.1.143]) by BGSMSX110.gar.corp.intel.com ([169.254.11.84]) with mapi id 14.03.0439.000; Wed, 18 Dec 2019 20:05:34 +0530 From: "Javeed, Ashraf" To: "Ni, Ray" , "'devel@edk2.groups.io'" CC: "Wang, Jian J" , "Wu, Hao A" Subject: Re: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 08/12] PciBusDxe: New PCI feature Max_Payload_Size Thread-Topic: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 08/12] PciBusDxe: New PCI feature Max_Payload_Size Thread-Index: AQHVkMaEjCtDWRlzIEyNU1vql5bY7KeIg7nQgDb7g4CAAAj2gIAAl75w Date: Wed, 18 Dec 2019 14:35:32 +0000 Message-ID: <95C5C2B113DE604FB208120C742E98245797C412@BGSMSX101.gar.corp.intel.com> References: <20191101150952.3340-1-ashraf.javeed@intel.com> <15D3127D273722D4.32624@groups.io> <95C5C2B113DE604FB208120C742E9824579172AC@BGSMSX101.gar.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C3A189E@SHSMSX104.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C3A19C4@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C3A19C4@SHSMSX104.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNjQ5N2Q2MjAtOTI1My00YWQzLWE1YWQtMGJlZTUzYTQ1OTdkIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiekZPM2NVVnRCZjE0aXh1R2dNenNaK094cVRUbit6aENWWWxoNnFLVFRtRVYwdUFVMjNIckt2bURpQzV4Y25XSSJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.223.10.10] MIME-Version: 1.0 Return-Path: ashraf.javeed@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Ray, As discussed, the ProcessMaxPayloadSize() gets the minimum payload size fo= r all devices under a certain root port, at the end of PciFeatureSetupPhase= . The value from the PCI features configuration table is aligned to each of = PciDevice->SetupMPS, under a root port.=20 The PciDevice->SetupMPS is used for its corresponding device's MaxReadRequ= estSize feature conditionally, since there is no PCIe capability for this f= eature. Thus, as per the implementation design formulated here, all the PciDevice-= >SetupXXX has to be finalized, considering the device policy, during the ph= ases - PciFeatureGetDevicePolicy & PciFeatureSetupPhase. Finally, it shall = be programmed during the phase PciFeatureConfigurationPhase. The OverrideMaxPayloadSize() does not have to use the PCI features configu= ration table, as that table is temporarily meant for aligning a feature val= ue among all devices of a root port. Responding to your other comments below inline. Thanks Ashraf > -----Original Message----- > From: Ni, Ray > Sent: Wednesday, December 18, 2019 2:40 PM > 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 08/12] > PciBusDxe: New PCI feature Max_Payload_Size >=20 > Ashraf, > Can ProcessMaxPayloadSize() get the minimum payload size for all devices= under > a certain root port? >=20 > I can understand that the payload size stored in the PCI features config= uration > table is the minimum value. > But the value stored in each PciDevice->SetupMPS is not the minimum valu= e. >=20 > So OverrideMaxPayloadSize() should use the value stored in the PCI featu= res > configuration table instead of the value stored in PciDevice->SetupMPS. >=20 > Thanks, > Ray >=20 > > -----Original Message----- > > From: Ni, Ray > > Sent: Wednesday, December 18, 2019 4:38 PM > > 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 > > 08/12] PciBusDxe: New PCI feature Max_Payload_Size > > > > > > + UINT8 SetupMPS; > > 1. Can it be "MaxPayloadSize"? I thought prefixing the name with "Setup" can imply that this data member = of PCI_IO_DEVICE would help to indicate it takes the device policy, transfo= rms into the final raw value after calculation, which shall be programmed i= n to hardware control register. I have used same style for all other featur= es, and I have to rename all those....I don't see any conflict in the name = SetupMPS as we know that MPS is shorthand for the MaxPayLoadSize... > > > > + > > > > + if (PciConfigPhase =3D=3D PciFeatureGetDevicePolicy) { > > > > + if (SetupMpsAsPerDeviceCapability (PciDevice->SetupMPS)) { > > > > 2. Can you replace " SetupMpsAsPerDeviceCapability > > (PciDevice->SetupMPS" with "PciDevice->MaxPayloadSize =3D=3D > EFI_PCI_CONF_MAX_PAYLOAD_SIZE_AUTO"? > > This makes the code more readable. > > I thought the routine name itself informally indicate what type of action = it has to take based on *_AUTO EFI encoding....I shall reconsider this. > > > > + MpsValue =3D (UINT8)PciDeviceCap.Bits.MaxPayloadSize; > > > > + // > > > > + // no change to PCI Root ports without any endpoint device > > > > + // > > > > + if (IS_PCI_BRIDGE (&PciDevice->Pci) && > > > > + PciDeviceCap.Bits.MaxPayloadSize) > > > > { > > > > + if (IsPciRootPortEmpty (PciDevice)) { > > > > + MpsValue =3D PCIE_MAX_PAYLOAD_SIZE_128B; > > > > + } > > > > + } > > > > 3. Above two if-s can be simplified as below? and please also copy the= spec > requirements here as comments. > > if (IsListEmpty (&PciDevice->ChildList)) { > > MpsValue =3D PCIE_MAX_PAYLOAD_SIZE_128B; } > > OK > > > > > > > > + } else { > > > > + MpsValue =3D TranslateMpsSetupValueToPci > > > > + (PciDevice->SetupMPS); > > > > 4. The function name can be "UefiToPciMaxPayloadSize()". And I suggest > > the value stored in PciDevice->SetupMPS (MaxPayloadSize) is the macro > > value defined in PciExpress21.h. We could do the conversion just after= the > GetDevicePolicy() call. > > Ok, I can rename to the suggested name for the translation routine. But, I= cannot use that immediately after the GetDevicePolicy() because some of th= e PCIe features are defined in the capability register and its *_AUTO can b= e used in co-relation to that; like in case of MaxPayloadSize feature; but = in case of PCIe feature like RelaxOrdering & NoSnoop, the PCI Base Specific= ation has not defined any corresponding capability or hardware reference va= lue for look-up, thus *_AUTO for these kind of features would be to just ig= nore it. > > > > + } > > > > + // > > > > + // discard device policy override request if greater than PCI= device > capability > > > > + // > > > > + PciDevice->SetupMPS =3D MIN > > > > + ((UINT8)PciDeviceCap.Bits.MaxPayloadSize, > > > > + MpsValue); } > > > > + > > > > + // > > > > + // align the MPS of the tree to the HCF with this device // > > > > + if > > > > + (PciFeaturesConfigurationTable) { > > > > + MpsValue =3D PciFeaturesConfigurationTable->Max_Payload_Size; > > > > 5. Max_Payload_Size can be "MaxPayloadSize". > > MpsValue can be "MaxPayloadSize". > > OK > > > > + > > > > + MpsValue =3D MIN (PciDevice->SetupMPS, MpsValue); > > > > + PciDevice->SetupMPS =3D MIN (PciDevice->SetupMPS, MpsValue); > > > > + > > > > + if (MpsValue !=3D PciFeaturesConfigurationTable->Max_Payload_= Size) { > > > > + PciFeaturesConfigurationTable->Max_Payload_Size =3D MpsValu= e; > > > > + } > > > > + } > > > > 6. Can you simplify the above logic? > > Will check on this > > > > + > > > > + DEBUG (( DEBUG_INFO, > > > > + "MPS: %d [DevCap:%d],", > > > > + PciDevice->SetupMPS, PciDeviceCap.Bits.MaxPayloadSize > > > > + )); > > > > + return EFI_SUCCESS; > > > > +} > > > > + > > > > +/** > > > > + Overrides the PCI Device Control register MaxPayloadSize > > > > +register field; if > > > > + the hardware value is different than the intended value. > > > > + > > > > + @param PciDevice A pointer to the PCI_IO_DEVICE in= stance. > > > > + > > > > + @retval EFI_SUCCESS The data was read from or written= to the PCI > > > > device. > > > > + @retval EFI_UNSUPPORTED The address range specified by Of= fset, > Width, > > > > and Count is not > > > > + valid for the PCI configuration h= eader of the PCI > controller. > > > > + @retval EFI_INVALID_PARAMETER Buffer is NULL or Width is invali= d. > > > > + > > > > +**/ > > > > +EFI_STATUS > > > > +OverrideMaxPayloadSize ( > > > > + IN PCI_IO_DEVICE *PciDevice > > > > + ) > > > > 7. Can this name be "ProgramMaxPayloadSize" because the function does > > the register programming? > > OK > > > > +{ > > > > + PCI_REG_PCIE_DEVICE_CONTROL PcieDev; > > > > + UINT32 Offset; > > > > + EFI_STATUS Status; > > > > + EFI_TPL OldTpl; > > > > + > > > > + PcieDev.Uint16 =3D 0; > > > > + Offset =3D PciDevice->PciExpressCapabilityOffset + > > > > + OFFSET_OF (PCI_CAPABILITY_PCIEXP, DeviceControl); > > > > + Status =3D PciDevice->PciIo.Pci.Read ( > > > > + &PciDevice->PciIo, > > > > + EfiPciIoWidthUint16, > > > > + Offset, > > > > + 1, > > > > + &PcieDev.Uint16 > > > > + ); > > > > 8. The PciExp is cached in PciExp field in the PciDevice structure. > > Why do you need to read it from HW again? > > Just for the sake of defensive programming; in case platform has done some= overrides in between; since numerous callbacks are going to platform code = of PCI Host Bridge Resource Allocation Protocol driver, and PCI Platform P= rotocol driver.=20 > > > > + if (EFI_ERROR(Status)){ > > > > + DEBUG (( DEBUG_ERROR, "Unexpected DeviceControl register > > > > + (0x%x) read > > > > error!", > > > > + Offset > > > > + )); > > > > + return Status; > > > > + } > > > > + if (PcieDev.Bits.MaxPayloadSize !=3D PciDevice->SetupMPS) { > > > > + PcieDev.Bits.MaxPayloadSize =3D PciDevice->SetupMPS; > > > > + DEBUG (( DEBUG_INFO, "MPS=3D%d,", PciDevice->SetupMPS)); > > > > + > > > > + // > > > > + // Raise TPL to high level to disable timer interrupt while > > > > + the write operation > > > > completes > > > > + // > > > > + OldTpl =3D gBS->RaiseTPL (TPL_HIGH_LEVEL); > > > > + > > > > + Status =3D PciDevice->PciIo.Pci.Write ( > > > > + &PciDevice->PciIo, > > > > + EfiPciIoWidthUint16, > > > > + Offset, > > > > + 1, > > > > + &PcieDev.Uint16 > > > > + ); > > > > + // > > > > + // Restore TPL to its original level > > > > + // > > > > + gBS->RestoreTPL (OldTpl); > > > > + > > > > + if (!EFI_ERROR(Status)) { > > > > + PciDevice->PciExpStruct.DeviceControl.Uint16 =3D PcieDev.Ui= nt16; > > > > + } else { > > > > + DEBUG (( DEBUG_ERROR, "Unexpected DeviceControl register > > > > + (0x%x) write > > > > error!", > > > > + Offset > > > > + )); > > > > 9. We can use ASSERT_EFI_ERROR() here. Failure of register writing is = a fatal > error. > > OK > > > > + } > > > > + } else { > > > > + DEBUG (( DEBUG_INFO, "No write of MPS=3D%d,", > > > > + PciDevice->SetupMPS)); > > > > 10. Can we skip this debug message? > > OK > > > > + } > > > > + > > > > + return Status; > > > > +} > > > > > > > > /** > > > > helper routine to dump the PCIe Device Port Type @@ -669,6 > > > > +809,18 @@ SetupDevicePciFeatures ( > > > > } > > > > } > > > > > > > > + DEBUG ((DEBUG_INFO, "[")); > > > > + // > > > > + // process the PCI device Max_Payload_Size feature // if > > > > + (SetupMaxPayloadSize ()) { > > > > + Status =3D ProcessMaxPayloadSize ( > > > > + PciDevice, > > > > + PciConfigPhase, > > > > + OtherPciFeaturesConfigTable > > > > + ); > > > > 11. Can this function be "CalculatemaxPayloadSize"? Process is too gen= eral. > > OK > > > > + } > > > > + DEBUG ((DEBUG_INFO, "]\n")); > > > > return Status; > > > > } > > > > > > > > @@ -765,6 +917,10 @@ ProgramDevicePciFeatures ( { > > > > EFI_STATUS Status =3D EFI_SUCCESS; > > > > > > > > + if (SetupMaxPayloadSize ()) { > > > > + Status =3D OverrideMaxPayloadSize (PciDevice); } DEBUG (( > > > > + DEBUG_INFO, "\n")); > > > > return Status; > > > > } > > > > > > > > @@ -878,6 +1034,7 @@ AddPrimaryRootPortNode ( > > > > ); > > > > if (PciConfigTable) { > > > > PciConfigTable->ID =3D PortNumber; > > > > + PciConfigTable->Max_Payload_Size =3D > > > > PCIE_MAX_PAYLOAD_SIZE_4096B; > > > > } > > > > RootPortNode->OtherPciFeaturesConfigurationTable =3D > > > > PciConfigTable; > > > > > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h > > > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h > > > > index f92d008..e5ac2a3 100644 > > > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h > > > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h > > > > @@ -79,6 +79,11 @@ struct > _OTHER_PCI_FEATURES_CONFIGURATION_TABLE > > > > { > > > > // Configuration Table ID > > > > // > > > > UINTN ID; > > > > + // > > > > + // to configure the PCI feature Maximum payload size to > > > > + maintain the data packet // size among all the PCI devices in > > > > + the PCI hierarchy // > > > > + UINT8 Max_Payload_Size; > > > > }; > > > > > > > > > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c > > > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c > > > > index 238959e..99badd6 100644 > > > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c > > > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c > > > > @@ -356,6 +356,63 @@ GetPlatformPciOptionRom ( > > > > return Status; > > > > } > > > > > > > > +/** > > > > + Helper routine to indicate whether the given PCI device > > > > +specific policy value > > > > + dictates to override the Max_Payload_Size to a particular > > > > +value, or set as per > > > > + device capability. > > > > + > > > > + @param MPS Input device-specific policy should be in terms= of type > > > > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE > > > > + > > > > + @retval TRUE Setup Max_Payload_Size as per device capability > > > > + FALSE override as per device-specific platform policy > > > > +**/ > > > > +BOOLEAN > > > > +SetupMpsAsPerDeviceCapability ( > > > > + IN UINT8 MPS > > > > +) > > > > +{ > > > > + if (MPS =3D=3D EFI_PCI_CONF_MAX_PAYLOAD_SIZE_AUTO) { > > > > + return TRUE; > > > > + } else { > > > > + return FALSE; > > > > + } > > > > +} > > > > + > > > > +/** > > > > + Routine to translate the given device-specific platform policy > > > > +from type > > > > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE to HW-specific value, as per PCI > > > > +Base Specification > > > > + Revision 4.0; for the PCI feature Max_Payload_Size. > > > > + > > > > + @param MPS Input device-specific policy should be in terms= of type > > > > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE > > > > + > > > > + @retval Range values for the Max_Payload_Size as define= d in the > PCI > > > > + Base Specification 4.0 **/ > > > > +UINT8 > > > > +TranslateMpsSetupValueToPci ( > > > > + IN UINT8 MPS > > > > +) > > > > +{ > > > > + switch (MPS) { > > > > + case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_128B: > > > > + return PCIE_MAX_PAYLOAD_SIZE_128B; > > > > + case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_256B: > > > > + return PCIE_MAX_PAYLOAD_SIZE_256B; > > > > + case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_512B: > > > > + return PCIE_MAX_PAYLOAD_SIZE_512B; > > > > + case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_1024B: > > > > + return PCIE_MAX_PAYLOAD_SIZE_1024B; > > > > + case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_2048B: > > > > + return PCIE_MAX_PAYLOAD_SIZE_2048B; > > > > + case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_4096B: > > > > + return PCIE_MAX_PAYLOAD_SIZE_4096B; > > > > + default: > > > > + return PCIE_MAX_PAYLOAD_SIZE_128B; > > > > + } > > > > +} > > > > + > > > > /** > > > > Generic routine to setup the PCI features as per its predetermi= ned > defaults. > > > > **/ > > > > @@ -364,6 +421,7 @@ SetupDefaultsDevicePlatformPolicy ( > > > > IN PCI_IO_DEVICE *PciDevice > > > > ) > > > > { > > > > + PciDevice->SetupMPS =3D EFI_PCI_CONF_MAX_PAYLOAD_SIZE_AUTO; > > > > } > > > > > > > > /** > > > > @@ -399,6 +457,7 @@ GetPciDevicePlatformPolicyEx ( > > > > // > > > > // platform chipset policies are returned for this PCI devi= ce > > > > // > > > > + PciIoDevice->SetupMPS =3D > > > > + PciPlatformExtendedPolicy.DeviceCtlMPS; > > > > > > > > DEBUG (( > > > > DEBUG_INFO, "[device policy: platform]" > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h > > > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h > > > > index a13131c..786c00d 100644 > > > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h > > > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h > > > > @@ -124,4 +124,36 @@ EFI_STATUS > > > > GetPciDevicePlatformPolicy ( > > > > IN PCI_IO_DEVICE *PciDevice > > > > ); > > > > + > > > > +/** > > > > + Helper routine to indicate whether the given PCI device > > > > +specific policy value > > > > + dictates to override the Max_Payload_Size to a particular > > > > +value, or set as per > > > > + device capability. > > > > + > > > > + @param MPS Input device-specific policy should be in terms= of type > > > > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE > > > > + > > > > + @retval TRUE Setup Max_Payload_Size as per device capability > > > > + FALSE override as per device-specific platform policy > > > > +**/ > > > > +BOOLEAN > > > > +SetupMpsAsPerDeviceCapability ( > > > > + IN UINT8 MPS > > > > +); > > > > + > > > > +/** > > > > + Routine to translate the given device-specific platform policy > > > > +from type > > > > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE to HW-specific value, as per PCI > > > > +Base Specification > > > > + Revision 4.0; for the PCI feature Max_Payload_Size. > > > > + > > > > + @param MPS Input device-specific policy should be in terms= of type > > > > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE > > > > + > > > > + @retval Range values for the Max_Payload_Size as define= d in the > PCI > > > > + Base Specification 4.0 **/ > > > > +UINT8 > > > > +TranslateMpsSetupValueToPci ( > > > > + IN UINT8 MPS > > > > +); > > > > #endif > > > > -- > > > > 2.21.0.windows.1 > > > > > > > > > > > >=20