From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by mx.groups.io with SMTP id smtpd.web11.8408.1576660228207744954 for ; Wed, 18 Dec 2019 01:10:28 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.65, mailfrom: ray.ni@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Dec 2019 01:10:27 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,328,1571727600"; d="scan'208";a="205779855" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga007.jf.intel.com with ESMTP; 18 Dec 2019 01:10:27 -0800 Received: from FMSMSX109.amr.corp.intel.com (10.18.116.9) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 18 Dec 2019 01:10:23 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx109.amr.corp.intel.com (10.18.116.9) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 18 Dec 2019 01:10:23 -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; Wed, 18 Dec 2019 17:10:21 +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 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: AQHVmdK5Hh4LyD94NUChTjCCT7Tp3Ke+eYzQgAFXgdA= Date: Wed, 18 Dec 2019 09:10:21 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C3A19C4@SHSMSX104.ccr.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> In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C3A189E@SHSMSX104.ccr.corp.intel.com> Accept-Language: en-US, zh-CN 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.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 Ashraf, Can ProcessMaxPayloadSize() get the minimum payload size for all devices u= nder a certain root port? I can understand that the payload size stored in the PCI features configur= ation table is the minimum value. But the value stored in each PciDevice->SetupMPS is not the minimum value. So OverrideMaxPayloadSize() should use the value stored in the PCI feature= s configuration table instead of the value stored in PciDevice->SetupMPS. Thanks, Ray > -----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 >=20 > > > + UINT8 SetupMPS; > 1. Can it be "MaxPayloadSize"? > > > + > > > + if (PciConfigPhase =3D=3D PciFeatureGetDevicePolicy) { > > > + if (SetupMpsAsPerDeviceCapability (PciDevice->SetupMPS)) { >=20 > 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. >=20 > > > + MpsValue =3D (UINT8)PciDeviceCap.Bits.MaxPayloadSize; > > > + // > > > + // no change to PCI Root ports without any endpoint device > > > + // > > > + if (IS_PCI_BRIDGE (&PciDevice->Pci) && PciDeviceCap.Bits.MaxP= ayloadSize) > > > { > > > + if (IsPciRootPortEmpty (PciDevice)) { > > > + MpsValue =3D PCIE_MAX_PAYLOAD_SIZE_128B; > > > + } > > > + } >=20 > 3. Above two if-s can be simplified as below? and please also copy the s= pec requirements here as comments. > if (IsListEmpty (&PciDevice->ChildList)) { > MpsValue =3D PCIE_MAX_PAYLOAD_SIZE_128B; > } >=20 >=20 >=20 > > > + } else { > > > + MpsValue =3D TranslateMpsSetupValueToPci (PciDevice->SetupMPS= ); >=20 > 4. The function name can be "UefiToPciMaxPayloadSize()". And I suggest t= he > value stored in PciDevice->SetupMPS (MaxPayloadSize) is the macro value > defined in PciExpress21.h. We could do the conversion just after the Get= DevicePolicy() call. >=20 > > > + } > > > + // > > > + // discard device policy override request if greater than PCI d= evice capability > > > + // > > > + PciDevice->SetupMPS =3D MIN ((UINT8)PciDeviceCap.Bits.MaxPayloa= dSize, > > > + MpsValue); } > > > + > > > + // > > > + // align the MPS of the tree to the HCF with this device // if > > > + (PciFeaturesConfigurationTable) { > > > + MpsValue =3D PciFeaturesConfigurationTable->Max_Payload_Size; >=20 > 5. Max_Payload_Size can be "MaxPayloadSize". > MpsValue can be "MaxPayloadSize". >=20 > > > + > > > + MpsValue =3D MIN (PciDevice->SetupMPS, MpsValue); > > > + PciDevice->SetupMPS =3D MIN (PciDevice->SetupMPS, MpsValue); > > > + > > > + if (MpsValue !=3D PciFeaturesConfigurationTable->Max_Payload_Si= ze) { > > > + PciFeaturesConfigurationTable->Max_Payload_Size =3D MpsValue; > > > + } > > > + } >=20 > 6. Can you simplify the above logic? >=20 > > > + > > > + 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 inst= ance. > > > + > > > + @retval EFI_SUCCESS The data was read from or written t= o the PCI > > > device. > > > + @retval EFI_UNSUPPORTED The address range specified by Offs= et, Width, > > > and Count is not > > > + valid for the PCI configuration hea= der of the PCI controller. > > > + @retval EFI_INVALID_PARAMETER Buffer is NULL or Width is invalid. > > > + > > > +**/ > > > +EFI_STATUS > > > +OverrideMaxPayloadSize ( > > > + IN PCI_IO_DEVICE *PciDevice > > > + ) >=20 > 7. Can this name be "ProgramMaxPayloadSize" because the function does > the register programming? >=20 > > > +{ > > > + 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 > > > + ); >=20 > 8. The PciExp is cached in PciExp field in the PciDevice structure. Why = do you need > to read it from HW again? >=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.Uint= 16; > > > + } else { > > > + DEBUG (( DEBUG_ERROR, "Unexpected DeviceControl register (0x%= x) write > > > error!", > > > + Offset > > > + )); >=20 > 9. We can use ASSERT_EFI_ERROR() here. Failure of register writing is a = fatal error. >=20 > > > + } > > > + } else { > > > + DEBUG (( DEBUG_INFO, "No write of MPS=3D%d,", PciDevice->SetupM= PS)); >=20 > 10. Can we skip this debug message? >=20 > > > + } > > > + > > > + return Status; > > > +} > > > > > > /** > > > helper routine to dump the PCIe Device Port Type @@ -669,6 +809,1= 8 @@ > > > SetupDevicePciFeatures ( > > > } > > > } > > > > > > + DEBUG ((DEBUG_INFO, "[")); > > > + // > > > + // process the PCI device Max_Payload_Size feature // if > > > + (SetupMaxPayloadSize ()) { > > > + Status =3D ProcessMaxPayloadSize ( > > > + PciDevice, > > > + PciConfigPhase, > > > + OtherPciFeaturesConfigTable > > > + ); >=20 > 11. Can this function be "CalculatemaxPayloadSize"? Process is too gener= al. >=20 > > > + } > > > + 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 PciConfigTa= ble; > > > > > > 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 hierarch= y > > > + // > > > + 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 o= f 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 fr= om > > > +type > > > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE to HW-specific value, as per PCI Ba= se > > > +Specification > > > + Revision 4.0; for the PCI feature Max_Payload_Size. > > > + > > > + @param MPS Input device-specific policy should be in terms o= f type > > > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE > > > + > > > + @retval Range values for the Max_Payload_Size as defined = 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 predetermine= d 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 device > > > // > > > + PciIoDevice->SetupMPS =3D PciPlatformExtendedPolicy.DeviceCtl= MPS; > > > > > > 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 o= f 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 fr= om > > > +type > > > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE to HW-specific value, as per PCI Ba= se > > > +Specification > > > + Revision 4.0; for the PCI feature Max_Payload_Size. > > > + > > > + @param MPS Input device-specific policy should be in terms o= f type > > > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE > > > + > > > + @retval Range values for the Max_Payload_Size as defined = in the PCI > > > + Base Specification 4.0 **/ > > > +UINT8 > > > +TranslateMpsSetupValueToPci ( > > > + IN UINT8 MPS > > > +); > > > #endif > > > -- > > > 2.21.0.windows.1 > > > > > > > > >=20